sherlock-audit / 2024-04-interest-rate-model-judging

9 stars 5 forks source link

BowTiedOriole - No slippage checks for deposit/mint/withdraw/redeem #250

Closed sherlock-admin2 closed 4 months ago

sherlock-admin2 commented 5 months ago

BowTiedOriole

medium

No slippage checks for deposit/mint/withdraw/redeem

Summary

The amount that a user receives from an ERC4626 vault is dependent upon the amount of assets and shares that currently exist. When a user submits a transaction, there is no guarantee that the exchange rate will remain the same by the time the transaction is included in a block.

Per EIP4626:

"If implementors intend to support EOA account access directly, they should consider adding an additional function call for deposit/mint/withdraw/redeem with the means to accommodate slippage loss or unexpected deposit/withdrawal limits, since they have no other means to revert the transaction if the exact output amount is not achieved."

Vulnerability Detail

Market does not include slippage functionality on deposit/mint/withdraw/redeem functions.

Impact

Users may receive less assets/shares that they expect due to changes between when they submit a transaction and when the transaction is confirmed.

Code Snippet

ERC4626#L46-L73

Market#L725-L745

  function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256 shares) {
    auditor.checkShortfall(this, owner, assets);
    RewardsController memRewardsController = rewardsController;
    if (address(memRewardsController) != address(0)) memRewardsController.handleDeposit(owner);
    shares = super.withdraw(assets, receiver, owner);
    emitMarketUpdate();
  }

  /// @notice Redeems the owner's floating pool assets to the receiver address.
  /// @dev Makes sure that the owner doesn't have shortfall after withdrawing.
  /// @param shares amount of shares to be redeemed for underlying asset.
  /// @param receiver address to which the assets will be transferred.
  /// @param owner address which owns the floating pool assets.
  /// @return assets amount of underlying asset that was withdrawn.
  function redeem(uint256 shares, address receiver, address owner) public override returns (uint256 assets) {
    auditor.checkShortfall(this, owner, previewRedeem(shares));
    RewardsController memRewardsController = rewardsController;
    if (address(memRewardsController) != address(0)) memRewardsController.handleDeposit(owner);
    assets = super.redeem(shares, receiver, owner);
    emitMarketUpdate();
  }

Tool used

Manual Review

Recommendation

Include a slippage parameter in withdraw/redeem functions. Add a new external function to deposit and mint with a minAmountOut parameter.

santipu03 commented 4 months ago

This issue is low (invalid) because the README doesn't specify that the market must comply with the ERC4626 standard.