ck - ERC5095::deposit does not account for slippage as intended

[github-actions[bot]]

ck

high

ERC5095::deposit does not account for slippage as intended

Summary

The ERC5095::deposit has a logic flaw where instead of comparing the returned amount to the expected amount. It compares it to the deposited amount therefore failing to account for slippage.

Vulnerability Detail

The ERC5095::deposit function checks whether a user receives at least the amount desired as follows:

       // Preview how many shares are needed to withdraw the desired amount of underlying
        uint128 shares = Cast.u128(previewDeposit(a));

        // Receive the funds from the sender
        Safe.transferFrom(IERC20(underlying), msg.sender, address(this), a);

        // consider the hardcoded slippage limit, 4626 compliance requires no minimum param.
        uint128 returned = IMarketPlace(marketplace).sellUnderlying(
            underlying,
            maturity,
            Cast.u128(a),
            shares
        );

        // Ensure the user received at least the amount desired
        if (returned < a)  {
            revert Exception(16, returned, a, address(0), address(0));
        }

As can be seen, the check if (returned < a) wrongfully compares the returned amount with the deposited amount (a). Instead the comparision should be if (returned < shares) as shares is what is calculated by the previewDeposit() function as the desired amount. shares again is the value passed to sellUnderlying as the minimum number of PTs that must be received.

uint128 returned = IMarketPlace(marketplace).sellUnderlying(
            underlying,
            maturity,
            Cast.u128(a),
            shares
        );

It is expected that the returned amount of shares can be less than the deposited amount due to slippage but the function will revert due to the incorrect check.

Impact

The deposit() function will fail when slippage happens. This would break the operation of the protocol as users may be prevented from depositing funds.

Code Snippet

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L173-L202 https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/MarketPlace.sol#L396-L421

Tool used

Manual Review

Recommendation

Change the check in ERC5095::deposit from if (returned < a) to if (returned < shares).

Alternatively, the check can be omiited altogether as IMarketPlace(marketplace).sellUnderlying() will already have a slippage check correctly in the line if (expected < s).

[IllIllI000]

See the discussion in https://github.com/sherlock-audit/2023-01-illuminate-judging/issues/16 about where slippage of this sort should be. Seem invalid

[sourabhmarathe]

Duplicate of #16.

[hrishibhat]

After further discussion with the Lead Watson and the Sponsor, this issue is not considered a duplicate of issue #16 As the contract requires compliance with ERC4626 for further external integrations, the slippage in the above function is supposed to be checked by the caller. The only functions where a slippage check is expected by the ERC are withdraw/redeem as mentioned in issue 16. Hence this issue is not considered a valid medium/high.

[iamckn]

Escalate for 5 USDC According to the standard - https://eips.ethereum.org/EIPS/eip-4626

"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."

The deposit and mint functions are included.

Specifically for deposit, the standard also says:

"MUST revert if all of assets cannot be deposited (due to deposit limit being reached, slippage, the user not approving enough underlying tokens to the Vault contract, etc)."

[sherlock-admin]

Escalate for 5 USDC According to the standard - https://eips.ethereum.org/EIPS/eip-4626

"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."

The deposit and mint functions are included.

Specifically for deposit, the standard also says:

"MUST revert if all of assets cannot be deposited (due to deposit limit being reached, slippage, the user not approving enough underlying tokens to the Vault contract, etc)."

You've created a valid escalation for 5 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

Suggestion to add additional functions to handle slippage is different from adding the check in the ERC5095 deposit function.

The above-mentioned issue is not valid as the check if (returned < a) does not cause loss of funds but would just revert the deposit. Which is highly unlikely for the reason below:
comment from @IllIllI000 : iPTs have the same decimals as the underlying, so conversion is not an issue. It would happen if iPTs are found to be more expensive than the underlying itself which, logically, should never happen since you're trying to get a discount now in return for lending. so it's like a protection thing, not a slippage thing

Also, The slippage considerations are done both with respect to ERC4626/5095. Quoting from issue #16 : In other words, it's not up to the ERC4626/ERC5095 contract implementation itself to determine whether too many shares needed to be burned in order to satisfy the request for exactly the provided number of underlying - it's up to the caller to have extra code to determine whether the number is satsifactory itself.

[sherlock-admin]

Escalation rejected

Suggestion to add additional functions to handle slippage is different from adding the check in the ERC5095 deposit function.

The above-mentioned issue is not valid as the check if (returned < a) does not cause loss of funds but would just revert the deposit. Which is highly unlikely for the reason below:
comment from @IllIllI000 : iPTs have the same decimals as the underlying, so conversion is not an issue. It would happen if iPTs are found to be more expensive than the underlying itself which, logically, should never happen since you're trying to get a discount now in return for lending. so it's like a protection thing, not a slippage thing

Also, The slippage considerations are done both with respect to ERC4626/5095. Quoting from issue #16 : In other words, it's not up to the ERC4626/ERC5095 contract implementation itself to determine whether too many shares needed to be burned in order to satisfy the request for exactly the provided number of underlying - it's up to the caller to have extra code to determine whether the number is satsifactory itself.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.