`V3Vault` lacks slippage protection on functions `deposit()`, `mint()`, `withdraw()` and `redeem()`

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L360-L393

Vulnerability details

Impact

The functions deposit(), mint(), deposit() with permit2, mint() with permit2, withdraw(), redeem() invokes _convertToAssets() or _convertToShares which relies heavily on an exchange rate / exchangeRateX96 that can change often.

It is reasonable that as the lender deposits (sends input tokens) to the vault, he/she should receive the rightful amount of shares on that moment. Also, when the lender redeems his/her collateral he/she should receive the rightful amount of assets on that moment.

If the token amount to be received (whether that be assets or shares) is less than what he/she expects, the transaction should revert. Hence the purpose of slippage protection.

Proof of Concept

Tools Used

Manual Review

Recommended Mitigation Steps

Here's an example of redeem implementation with slippage protection.

    function redeem(uint256 shares, address receiver, address owner) external override returns (uint256) { 
        (uint256 assets,) = _withdraw(receiver, owner, shares, true);
        return assets;
    }

++  function redeem(uint256 shares, address receiver, address owner, uint256 minAssets) external override returns (uint256) { 
++      (uint256 assets,) = _withdraw(receiver, owner, shares, true);    
++      if (assets < minAssets) revert TooMuchSlippageNotAllowed();

++      return assets;
++  }

Implement this pattern to the rest of the functions: deposit(), mint(), deposit() with permit2, mint() with permit2, withdraw().

Assessed type

Other

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as primary issue

[0xEVom]

Setting as primary since this is the most comprehensive submission in listing occurrences. However, none of the duplicates demonstrate a direct loss of funds and if the shares rate is not directly manipulatable via MEV, this is likely low severity.

As pointed out in #337, the ERC-4626 standard recommends adding additional functions to account for slippage.

[0xEVom]

181 seems to provide a reasonable POC

[kalinbas]

The exchange rate of shares is only updated once per block. It can not be directly manipulated by whales depositing or withdrawing because it only is affected by the interest rate (which takes effect in the following blocks). There is no point in frontrunning a deposit or withdraw tx. So slippage control is not needed here.

[c4-sponsor]

kalinbas (sponsor) disputed

[jhsagd76]

I believe this is only a valid issue for mint, but it would only occur when there is a significant fluctuation in network gas fees causing transactions to remain unconfirmed for a long time. The change in shares rate is mostly due to interest which makes the impact of this issue very slight. Marked as low.

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[jhsagd76]

L-B

[c4-judge]

jhsagd76 marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by jhsagd76

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)