-
Notifications
You must be signed in to change notification settings - Fork 0
IllIllI - Illuminate's PT doesn't respect users' slippage specifications #114
Comments
Unfortunately, there isn't a clean solution here for users. When using the preview methods as suggested in the recommendation, an invalid slippage is used for the fourth parameter to In addition, I would disagree with the severity of this issue on that basis as well, and I am open to hearing other ideas the judges have. |
Even if there are other paths that the user can take, the presence of a path where they lose principal means there's still a high-severity issue. I believe the problem you're facing is that convert* is using preview*, when it's supposed to be a flash-resistant method of getting the value. The ERC5095 spec specifically only requires that convertToUnderlying() only work at maturity, because according to one of the spec authors, it's an 'open question' about how to get a valid price before then. |
As an alternative solution, should we have user provide the slippage then? Given that |
The 5095 standard has specific arguments for |
Yeah unfortunately we also intended some backwards compatability with 4626 (specifically for some integrations like the aztec 4626 bridge as this product targets their sort of batched design in particular). W/ that context we cant quite remove that sort of integration compatibility with pre-maturity redemptions, nor can add any params without breaking those integrations. So its difficult to find a great solution other than writing overrides that do include additional parameters for slippage protection. IIRC we had issues with bytecode size limits and didnt want to add them but perhaps through other efforts we reduced some headroom there? |
Escalate for 10 USDC Slight-moderate incorrect slippage controls historically have been graded as medium not high. Seems like there is still ongoing discussion about this issue, but medium seems appropriate if deemed valid. |
You've created a valid escalation for 10 USDC! To remove the escalation from consideration: Delete your comment. You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final. |
Escalate for 10 USDC |
You've created a valid escalation for 10 USDC! To remove the escalation from consideration: Delete your comment. You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final. |
IllIllI
high
Illuminate's PT doesn't respect users' slippage specifications
Summary
Illuminate's PT doesn't respect users' slippage specifications, and allows more slippage than is requested
Vulnerability Detail
ERC5095.withdraw()/redeem()
's code adds extra slippage on top of what the user requestsImpact
Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Miner-extractable value (MEV)
At the end of withdrawal/redemption, the user will end up losing more underlying than they wished to, 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
The NatSpec says
Before maturity, sends 'assets' by selling shares of PT on a YieldSpace AMM.
, so it's clear that the intention is to send back the amount of tokens specified in the input argument. In spite of this, extra slippage is allowed for the amount:https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L219-L226
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L240-L247
(
redeem()
has the same issueand
IMarketPlace.sellPrincipalToken()
also considers the amount as an amount that already includes slippage:https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Marketplace.sol#L285-L298
Tool used
Manual Review
Recommendation
Pass
Cast.u128(a)
to the two calls insteadThe text was updated successfully, but these errors were encountered: