IllIllI - Illuminate's PT doesn't respect users' slippage specifications
Opened this issue · 9 comments
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 requests
Impact
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:
// File: src/tokens/ERC5095.sol : ERC5095.withdraw() #1
219 uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
220 underlying,
221 maturity,
222 shares,
223 @> Cast.u128(a - (a / 100))
224 );
225 Safe.transfer(IERC20(underlying), r, returned);
226: return returned;https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L219-L226
// File: src/tokens/ERC5095.sol : ERC5095.withdraw() #2
240 uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
241 underlying,
242 maturity,
243 Cast.u128(shares),
244 @> Cast.u128(a - (a / 100))
245 );
246 Safe.transfer(IERC20(underlying), r, returned);
247: return returned;https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L240-L247
(redeem() has the same issue
and IMarketPlace.sellPrincipalToken() also considers the amount as an amount that already includes slippage:
// File: src/MarketPlace.sol : MarketPlace.a #3
279 /// @notice sells the PT for the underlying via the pool
280 /// @param u address of an underlying asset
281 /// @param m maturity (timestamp) of the market
282 /// @param a amount of PTs to sell
283 @> /// @param s slippage cap, minimum amount of underlying that must be received
284 /// @return uint128 amount of underlying bought
285 function sellPrincipalToken(
286 address u,
287 uint256 m,
288 uint128 a,
289 @> uint128 s
290: ) external returns (uint128) {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 instead
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 sellPrincipalToken. As a result, we are finding that this does not work on fork-mode tests. For now, we're going to keep the method in place with the knowledge that users should have other avenues available to them (via using the pool directly) to reduce their slippage risk.
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 withdrawPreview() tells us how many shares will be required and the fact that it could change depending on where in the block the swap occurs, I think that's the only way around this (other than providing the 1% slippage built-in value we provide)
The 5095 standard has specific arguments for withdraw(), so I don't think you should add another argument. The standard also mentions Note that some implementations will require pre-requesting to the principal token contract before a withdrawal may be performed. Those methods should be performed separately. so you could have a function that pre-specifies the slippage before each withdrawal, or you could have a completely separate pre-maturity withdrawal function, and have the normal withdraw() revert before maturity, as is done in the sample contract in the EIP
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.
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.
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.
Escalate for 10 USDC
Just to add another point: this should be open/close simultaneously with #181 as the root issue is the same.
Escalate for 10 USDC
Just to add another point: this should be open/close simultaneously with #181 as the root issue is the same.
You've created a valid escalation for 10 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.