IllIllI - Sense PT redemptions do not allow for known loss scenarios

[sherlock-admin]

IllIllI

high

Sense PT redemptions do not allow for known loss scenarios

Summary

Sense PT redemptions do not allow for known loss scenarios, which will lead to principal losses

Vulnerability Detail

The Sense PT redemption code in the Redeemer expects any losses during redemption to be due to a malicious adapter, and requires that there be no losses. However, there are legitimate reasons for there to be losses which aren't accounted for, which will cause the PTs to be unredeemable. The Lido FAQ page lists two such reasons:

- Slashing risk

ETH 2.0 validators risk staking penalties, with up to 100% of staked funds at risk if validators fail. To minimise this risk, Lido stakes across multiple professional and reputable node operators with heterogeneous setups, with additional mitigation in the form of insurance that is paid from Lido fees.

- stETH price risk

Users risk an exchange price of stETH which is lower than inherent value due to withdrawal restrictions on Lido, making arbitrage and risk-free market-making impossible. 

The Lido DAO is driven to mitigate above risks and eliminate them entirely to the extent possible. Despite this, they may still exist and, as such, it is our duty to communicate them.

https://help.lido.fi/en/articles/5230603-what-are-the-risks-of-staking-with-lido

If Lido is slashed, or there are withdrawal restrictions, the Sense series sponsor will be forced to settle the series, regardless of the exchange rate (or miss out on their rewards). The Sense Divider contract anticipates and properly handles these losses, but the Illuminate code does not.

Lido is just one example of a Sense token that exists in the Illuminate code base - there may be others added in the future which also require there to be allowances for losses.

Impact

Permanent freezing of funds

There may be a malicious series sponsor that purposely triggers a loss, either by DOSing Lido validators, or by withdrawing enough to trigger withdrawal restrictions. In such a case, the exchange rate stored by Sense during the settlement will lead to losses, and users that hold Illumimate PTs (not just the users that minted Illuminate PTs with Sense PTs), will lose their principal, because Illuminate PT redemptions are an a share-of-underlying basis, not on the basis of the originally-provided token.

While the Illuminate project does have an emergency withdraw() function that would allow an admin to rescue the funds and manually distribute them, this would not be trustless and defeats the purpose of having a smart contract.

Code Snippet

The Sense adapter specifically used in the Illuminate tests is the one that corresponds to wstETH:

// File: test/fork/Contracts.sol    #1
36    // (sense adapter)
37    // NOTE for sense, we have to use the adapter contract to verify the underlying/maturity
38    // NOTE also we had to use the wsteth pools.... (maturity: 1659312000)
39:    address constant SENSE_ADAPTER = 0x880E5caBB22D24F3E278C4C760e763f239AccA95;

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/test/fork/Contracts.sol#L36-L39

The code for the redemption of the Sense PTs assumes that one PT equals at least one underlying, which may not be the case:

// File: src/Redeemer.sol : Redeemer.redeem()   #2
360            // Get the balance of tokens to be redeemed by the user
361            uint256 amount = token.balanceOf(cachedLender);
...
379            IConverter(converter).convert(
380                compounding,
381                u,
382                IERC20(compounding).balanceOf(address(this))
383            );
384    
385            // Get the amount received
386            uint256 redeemed = IERC20(u).balanceOf(address(this)) - starting;
387    
388 @>         // Verify that underlying are received 1:1 - cannot trust the adapter
389 @>         if (redeemed < amount) {
390 @>             revert Exception(13, 0, 0, address(0), address(0));
391            }
392    
393            // Update the holdings for this market
394            holdings[u][m] = holdings[u][m] + redeemed;
395    
396            emit Redeem(p, u, m, redeemed, msg.sender);
397            return true;
398:       }

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

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:
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

Allow losses during redemption if Sense's Periphery.verified() returns true

[sourabhmarathe]

I agree with the stated problem from this report, the only thing I would change about the Recommendation is that we can check is if the Lender contract has approved the periphery.

[JTraversa]

Hah i find this one kinda funny since another of your tickets recommended doing the opposite and ensuring yield can never be negative.

Buuut i would clarify here that were are not discussing the stETH <-> ETH conversion rates but strictly the actual stETH yields.

So yes, they can be negative should slashing exceed yields, but Lido has historically been nowhere close to this outside of tiny (~1 minutes?) periods. (Even then i am unsure if lido has been slashed yet / I might be confusing them with another staking service)

Given we have terms of 3+ months, it is extremely unrealistic for this to ever have any impact.

So I would vaguely accept the issue (to align with my report on the other negative yield ticket), and then definitely downgrade this to medium at most, perhaps low.

[IAm0x52]

Escalate for 1 USDC

Reminder @Evert0x

[sherlock-admin]

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.

[KenzoAgada]

Escalate for 40 USDC
See sponsor's comments. Seems like unlikely external conditions are needed so this might be better described as medium severity.

[sherlock-admin]

Escalate for 40 USDC
See sponsor's comments. Seems like unlikely external conditions are needed so this might be better described as medium severity.

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

[Evert0x]

Escalation accepted

[sherlock-admin]

Escalation accepted

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[sourabhmarathe]

https://github.com/Swivel-Finance/illumigrate/pull/270