sherlock-audit / 2022-10-illuminate-judging

3 stars 0 forks source link

__141345__ - `Holdings` could be unfair for redeem amount calculation #206

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

141345

high

Holdings could be unfair for redeem amount calculation

Summary

The underlying amount users can redeem is based on the redeem amount of external markets, but not the PT balance. This rule is not consistent and not fair to the users. Some users could lose fund, some could abuse this rule to steal fund.

Vulnerability Detail

holdings will be updated when redeem() is called for the specific market. And later users can redeem for the underlying by burning their shares. However, this mechanism to calculate the underlying redeem amount based on the external market amount is not fair. Users underlying share should depend on the PT balance, other than the loan amount in some specific market and specific maturity.

Imagine the following: Some user has 10% of the PT total supply. Now 2 markets mature, Swivel with $1,000, and Sense with $200. When redeem for the underlying, if the user specify Swivel, the amount redeemed will be $100, but if use Sense as input, the user only get $20. Which is no consistent and not fair.

Impact

Code Snippet

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L422

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L464

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L517

Tool used

Manual Review

Recommendation

The redeem amount calculation should based on the PT balance, rather than the external markets amount.

sourabhmarathe commented 1 year ago

I think this report slightly misinterprets the redemption flow. Users will be made whole on a market-wide basis. That is all protocol redemptions will occur to the underlying prior to executing the user's redemption. As a result, the user will not select which protocol to redeem from and instead will always redeem completely from Illuminate.

141345 commented 1 year ago

Escalate for 11 USDC

Admit that I have some misrepresentation in the report. I intended to referring specific "underlying and maturity", but mistakenly written as the protocols. But the main idea remains. Please let me reiterate.

The point of this issue is, the amount user can redeem varies, depending on the "underlying and maturity" market size illuminate opened with all external protocols. Inevitably the sizes will be different, such as total amount of "DAI 2023/06/30" is 200M, but with "USDC 2023/06/30" is 150M, as a result, for same amount of PT, users' redemption amount will differ by 25% for DAI and USDC. And sometimes this choice is not up to the users, such as autoRedeem(), u and m are provided by the auto function caller.

I believe this is a business logic issue, which introduces potential unfair redemption amount for users.

sherlock-admin commented 1 year ago

Escalate for 11 USDC

Admit that I have some misrepresentation in the report. I intended to referring specific "underlying and maturity", but mistakenly written as the protocols. But the main idea remains. Please let me reiterate.

The point of this issue is, the amount user can redeem varies, depending on the "underlying and maturity" market size illuminate opened with all external protocols. Inevitably the sizes will be different, such as total amount of "DAI 2023/06/30" is 200M, but with "USDC 2023/06/30" is 150M, as a result, for same amount of PT, users' redemption amount will differ by 25% for DAI and USDC. And sometimes this choice is not up to the users, such as autoRedeem(), u and m are provided by the auto function caller.

I believe this is a business logic issue, which introduces potential unfair redemption amount for users.

You've created a valid escalation for 11 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.