Omniscia Ultra Yield Audit

BaseControlledAsyncRedeem Manual Review Findings

BaseControlledAsyncRedeem Manual Review Findings

BCA-01M: Incompatibility of System with EIP-777 Assets

Description:

The BaseControlledAsyncRedeem contract is not compatible with EIP-777 style tokens as the before and after hooks invoked during several functions (i.e. BaseControlledAsyncRedeem::_performDeposit) will be vulnerable to re-entrancy attacks.

Example:

src/vaults/BaseControlledAsyncRedeem.sol
374/// @dev Internal function to process deposit and mint flows
375function _performDeposit(
376    address _asset,
377    uint256 assets,
378    uint256 shares,
379    address receiver
380) internal {
381    // Checks
382    require(assets != 0, EmptyDeposit());
383    require(shares != 0, NothingToMint());
384

385    // Pre-deposit hook - use the actual asset amount being transferred
386    beforeDeposit(_asset, assets, shares);
387

388    // Need to transfer before minting or ERC777s could reenter
389    SafeERC20.safeTransferFrom(
390        IERC20(_asset),
391        msg.sender,
392        address(this),
393        assets
394    );
395

396    // Mint shares to receiver
397    _mint(receiver, shares);
398

399    // Emit event
400    emit Deposit(msg.sender, receiver, assets, shares);
401

402    // After-deposit hook - use the actual asset amount that was transferred
403    afterDeposit(_asset, assets, shares);
404}

Recommendation:

We advise this notice to be omitted and support for non-standard EIP-20 tokens to be eliminated from the system.

Alternatively, we advise re-entrancy guards to be introduced throughout all core functions of the system as well as its derivative implementations to avoid re-entrancy complications.

Alleviation (28f2785396):

The Ultra Yield team stated that they do not wish to support EIP-777 assets yet comments remain in the code indicating otherwise.

For this item to be considered addressed, we require any comments alluding to EIP-777 support to be omitted.

Alleviation (2d9651dbd7):

Any mention of EIP-777 support has been omitted, addressing this exhibit.

BCA-02M: Improper Accounting Error Drift

Description:

The BaseControlledAsyncRedeem::_performWithdraw function is invoked with the results of share-to-asset and asset-to-share conversions that round in favour of the protocol (i.e. asset-to-share rounds upward, share-to-asset rounds downward).

Due to this approach, it is possible for a user to have a ClaimableRedeemSlot data entry with a non-zero amount of assets and a zero amount of shares rendering them permanently irredeemable.

Impact:

It is presently possible for truncation errors to result in an irredeemable balance for a user.

Example:

src/vaults/BaseControlledAsyncRedeem.sol
510/// @dev Internal function for redeeming exact number of `shares` and withdrawing
511/// @dev assets in `_asset` to `receiver`
512function _redeemAsset(
513    address _asset,
514    uint256 shares,
515    address receiver,
516    address controller
517) internal checkAccess(controller) returns (uint256 assets) {
518    require(shares != 0, NothingToRedeem());
519    
520    // Calculate assets directly in asset units
521    assets = _calculateClaimableAssetsForShares(controller, _asset, shares);
522    require(assets != 0, NothingToWithdraw());
523

524    // Execute withdraw
525    _performWithdraw(_asset, assets, shares, receiver, controller);
526}

Recommendation:

We advise the code to permit a full withdrawal of the remaining assets if 0 shares remain in a ClaimableRedeemSlot data entry, ensuring no funds can be lost due to truncations during partial withdrawals.

Alleviation (28f27853965de07fb79f4f2b5fed696d35120032):

The Ultra Yield team evaluated the behaviour described and consider it to be in-line with conventional EIP-4626 vault functionality that rounds in favour of the system.

As such, we consider this exhibit safely acknowledged.

BCA-03M: Incorrect Event Emission

Description:

The referenced event emission will yield the mutated $.rateProvider variable incorrectly.

Impact:

The data emitted through the RateProviderUpdated is incorrect.

Example:

src/vaults/BaseControlledAsyncRedeem.sol
1002/// @notice Accept proposed rate provider
1003 /// @dev Pauses vault to ensure provider setup and prevent deposits with faulty prices
1004 /// @dev Oracle must be switched before unpausing
1005 function acceptProposedRateProvider(address newRateProvider) external onlyOwner {
1006     BaseAsyncRedeemStorage storage $ = _getBaseAsyncRedeemStorage();
1007     AddressUpdateProposal memory proposal = $.proposedRateProvider;
1008 
1009     require(proposal.addr != address(0), NoRateProviderProposed());
1010     require(proposal.addr == newRateProvider, ProposedRateProviderMismatch());
1011     require(block.timestamp >= proposal.timestamp + ADDRESS_UPDATE_TIMELOCK, CannotAcceptRateProviderYet());
1012     require(block.timestamp <= proposal.timestamp + MAX_ADDRESS_UPDATE_WAIT, RateProviderUpdateExpired());
1013 
1014     $.rateProvider = IUltraVaultRateProvider(proposal.addr);
1015     delete $.proposedRateProvider;
1016     emit RateProviderUpdated(address($.rateProvider), proposal.addr);
1017 
1018     // Pause to manually check the setup by operators
1019     _pause();
1020 }

Recommendation:

We advise the event's emission to be relocated prior to the $.rateProvider assignment, ensuring the data yielded by the RateProviderUpdated event is correct.

Alleviation (28f27853965de07fb79f4f2b5fed696d35120032):

The event's emission was corrected, ensuring that the proper rate provider data points are emitted.

BCA-04M: Non-Standard Event Emission

Description:

The EIP-7540 standard mandates that the RedeemRequest event is emitted during redemption requests, however, the system uses a custom RedeemRequested event instead.

Impact:

Off-chain software meant to integrate with EIP-7540 vaults will fail to do so properly due to absence of MUST keyword events defined in the EIP-7540 standard.

Example:

src/vaults/BaseControlledAsyncRedeem.sol
782/// @dev Internal function for processing redeem requests
783function _requestRedeemOfAsset(
784    address _asset,
785    uint256 shares,
786    address controller,
787    address owner
788) internal checkAccess(owner) returns (uint256 requestId) {
789    // Checks
790    require(shares != 0, NothingToRedeem());
791    require(IERC20(address(this)).balanceOf(owner) >= shares, InsufficientBalance());
792

793    // Validate that the asset is supported by the rate provider
794    require(_asset == this.asset() || rateProvider().isSupported(_asset), AssetNotSupported());
795

796    // Call beforeRequestRedeem hook
797    beforeRequestRedeem(_asset, shares, controller, owner);
798

799    // Update pending redeem
800    _increasePendingRedeem(controller, _asset, shares);
801

802    // Transfer shares to vault for burning later
803    SafeERC20.safeTransferFrom(IERC20(this), owner, address(this), shares);
804

805    // Emit event
806    emit RedeemRequested(controller, owner, REQUEST_ID, msg.sender, shares);
807    
808    return REQUEST_ID;
809}

Recommendation:

We advise the proper event signature to be satisfied so as to be compliant with the EIP-7540 standard.

Alleviation (28f27853965de07fb79f4f2b5fed696d35120032):

The event was aptly renamed to ensure it complies with the EIP-7540 standard, alleviating this exhibit.

Prev

RedeemQueue.sol (RQE-S)

Next

UltraFeeder.sol (UFR-M)

On this page