IllIllI - Illuminate's PT doesn't respect users' slippage specifications for underlyings

[github-actions[bot]]

IllIllI

high

Illuminate's PT doesn't respect users' slippage specifications for underlyings

Summary

Illuminate's PT doesn't respect users' slippage specifications for underlyings, and allows more slippage than is requested

Vulnerability Detail

ERC5095.withdraw()/redeem()'s code adds extra underlying slippage on top of what the user requests

Impact

At the end of withdrawal/redemption, the user will end up receiving less underlying than they asked for, due to slippage. If the user had used a external PT to mint the Illuminate PT, they will have lost part of their principal.

Code Snippet

// File: src/tokens/ERC5095.sol : ERC5095.withdraw()   #1

271                    uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
272                        underlying,
273                        maturity,
274                        shares,
275 @>                     Cast.u128(a - (a / 100))
276:                   );

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L271-L276

// File: src/tokens/ERC5095.sol : ERC5095.withdraw()   #2

302                    uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
303                        underlying,
304                        maturity,
305                        Cast.u128(shares),
306 @>                     Cast.u128(a - (a / 100))
307:                   );

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L302-L307

redeem() has the same issue

Tool used

Manual Review

Recommendation

This is the same issue that I described in the last contest. In the original issue, the finding was disputed because there wasn't a clean solution for slippage protection on the number of shares burned in order to satisfy the input underlying amount. During the discussion of the issue, it became clear that the ERC5095 contract was supposed to be cross-compatable with the ERC4626 standard, and that standard has this to say:

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. https://eips.ethereum.org/EIPS/eip-4626

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. Note though that both standards are very clear that the exact number of underlying must be provided back, and the implmentation as it stands does not do this.

[sourabhmarathe]

We believe this issue and its duplicates are related to the same underlying problem of complying with ERC4626.

We will address this issue and its duplicates altogether in one PR.

[thereksfour]

Escalate for 5 USDC

Slight-moderate incorrect slippage controls historically have been graded as medium not high. Look at the Criteria for Issues:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

High: This vulnerability would result in a material loss of funds and the cost of the attack is low (relative to the amount of funds lost). The attack path is possible with reasonable assumptions that mimic on-chain conditions. The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

The cost to the attacker to exploit this vulnerability is not low

[sherlock-admin]

Escalate for 5 USDC

Slight-moderate incorrect slippage controls historically have been graded as medium not high. Look at the Criteria for Issues:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

High: This vulnerability would result in a material loss of funds and the cost of the attack is low (relative to the amount of funds lost). The attack path is possible with reasonable assumptions that mimic on-chain conditions. The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

The cost to the attacker to exploit this vulnerability is not low

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

The cost is extremely low for a MEV bot to trigger slippage, and the slippage amount is hard-coded, so the issue exists for all the users every time the protocol is used

[sherlock-admin]

Escalation rejected

The cost is extremely low for a MEV bot to trigger slippage, and the slippage amount is hard-coded, so the issue exists for all the users every time the protocol is used

This issue's escalations have been rejected!

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

[IllIllI000]

https://github.com/Swivel-Finance/illuminate/pull/23 PR properly removes the incorrect slippage bounds, and refactors the code so that common code can be used for alternate versions of the APIs where the user can specify exact slippage. The code also changes mint() to use buyPrincipalToken() rather than sellUnderlying(), and withdraw() to use buyUnderlying() instead of sellPrincipalToken(). buyPrincipalToken() calls pool.buyFYToken() whereas sellUnderlying() calls pool.sellBase(), and buyUnderlying() calls pool.buyBase() whereas sellPrincipalToken() calls pool.sellFYToken(). The new calls to the "buy" functions required that exactly the amount being bought is provided, even if more has been transferred, whereas the old "sell" functions allow for positive slippage in the amount receieved. Since the "preview" functions are used in order to ensure the right number of base/FYT are transferred in, in the same transaction, there is no loss due to unused base/FYT.