IllIllI - Wrong Illuminate PT allowance checks lead to loss of principal
Opened this issue · 10 comments
IllIllI
high
Wrong Illuminate PT allowance checks lead to loss of principal
Summary
Wrong Illuminate PT allowance checks lead to loss of principal
Vulnerability Detail
The ERC5095.withdraw() function, when called after maturity by a user with an allowance, incorrectly uses the amount of underlying rather than the number of shares the underlying is worth, when adjusting the allowance.
Impact
Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
If each underlying is worth less than a share (e.g. if there were losses due to Lido slashing, or the external PT's protocol is paused), then a user will be allowed to take out more shares than they have been given allowance for. If the user granting the approval had minted the Illuminate PT by providing a external PT, in order to become an LP in a pool, the loss of shares is a principal loss.
Code Snippet
The amount of underlying is being subtracted from the allowance, rather than the number of shares required to retrieve that amount of underlying:
// File: src/tokens/ERC5095.sol : ERC5095.withdraw() #1
262 uint256 allowance = _allowance[o][msg.sender];
263 @> if (allowance < a) {
264 revert Exception(20, allowance, a, address(0), address(0));
265 }
266 @> _allowance[o][msg.sender] = allowance - a;
267 return
268 IRedeemer(redeemer).authRedeem(
269 underlying,
270 maturity,
271 o,
272 r,
273 a
274: );https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L262-L274
Redemptions of Illuminate PTs for underlyings is based on shares of each Illuminate PT's totalSupply() of the available underlying, not the expect underlying total, and there is no way for an admin to pause this withdrawal since the authRedeem() function does not use the unpaused modifier:
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L464
Tool used
Manual Review
Recommendation
Calculate how many shares the the amount of underlying is worth (e.g. call previewWithdraw()) and use that amount when adjusting the allowance
This report will be addressed. The documentation should be updated to reflect what withdraw will do for the user after maturity. Given the nature of the redemption process post-maturity, we'll instead have the user specify how many PTs they will burn post-maturity.
This is intended given post maturity previewWithdraw returns a 1:1 amount.
You can utilize the two interchangably, and the suggested remediation actually returns the same thing as the current code.
After discussing very briefly with the watson, he does have a rare edge case pointed out, where the heuristic of 1:1 redemptions may not be correct and will lead to slightly incorrect calculation of approvals.
That said.... this is definitely minimally impactful (especially given the design of PTs / lack of approval usage post maturity), so likely a low-med?
To be honest we could/should? have used a binary yes/no approval process for auto redemption but wanted to utilize storage slots already available
Escalate for 1 USDC
Reminder @Evert0x
You've created a valid escalation for 1 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 50 USDC
See sponsor's comments.
To my understanding, as mentioned, first of all the issue necessitates unusual external conditions.
Furthermore, as the sponsor wrote, the protocol is working on the assumption that shares and underlying are 1:1 (see previewWithdraw). Because of that, the issue's fix practically makes no difference, as previewWithdraw anyway returns a 1:1 rate for shares-underlying. Therefore it seems to me that if the issue is valid (which I'm not sure), it should be not higher than a medium.
Escalate for 50 USDC
See sponsor's comments.
To my understanding, as mentioned, first of all the issue necessitates unusual external conditions.
Furthermore, as the sponsor wrote, the protocol is working on the assumption that shares and underlying are 1:1 (seepreviewWithdraw). Because of that, the issue's fix practically makes no difference, aspreviewWithdrawanyway returns a 1:1 rate for shares-underlying. Therefore it seems to me that if the issue is valid (which I'm not sure), it should be not higher than a medium.
You've created a valid escalation for 50 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.
Escalation accepted
Escalation accepted
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.