IllIllI - Changing the Illuminate PT will make users of the old iPT lose their funds

[github-actions[bot]]

IllIllI

medium

Changing the Illuminate PT will make users of the old iPT lose their funds

Summary

Changing the Illuminate PT will make users of the old iPT lose their funds

Vulnerability Detail

The setPrincipal() function allows the admin to change the iPT to another contract (e.g. due to a bug that needs to be worked around), without checking that there are no shares of the old one.

Impact

If the admin changes the iPT once one is already in use, holders of the old iPT won't be able to redeem, and will have their funds lost to holders of the new iPTs.

Code Snippet

There are no checks whether one is currently in use:

// File: src/MarketPlace.sol : MarketPlace.setPrincipal()   #1

224        function setPrincipal(
225            uint8 p,
226            address u,
227            uint256 m,
228            address a,
229            address h,
230            address sensePeriphery
231        ) external authorized(admin) returns (bool) {
232            // Set the principal token in the markets mapping
233:@>         markets[u][m][p] = a;

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/MarketPlace.sol#L224-L243

Redemption of the old one relies on the Redeemer:

// File: src/tokens/ERC5095.sol : ERC5095.redeem()    #2

420:                return
421:                    IRedeemer(redeemer).authRedeem(
422:                        underlying,
423:                        maturity,
424:                        msg.sender,
425:                        r,
426:                        s
427:                    );

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L420-L427

Which will fail since the modifier only knows about the new token that has been set:

// File: src/Redeemer.sol : Redeemer.authRedeem()   #3

548        function authRedeem(
549            address u,
550            uint256 m,
551            address f,
552            address t,
553            uint256 a
554        )
555            external
556 @>         authorized(IMarketPlace(marketPlace).markets(u, m, 0))
557            unpaused(u, m)
558            returns (uint256)
559        {
560            // Get the principal token for the given market
561:           IERC5095 pt = IERC5095(IMarketPlace(marketPlace).markets(u, m, 0));

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/Redeemer.sol#L548-L561

Tool used

Manual Review

Recommendation

Don't allow over-writing the old iPT if it exists, or introduce a migration function that will give old holders shares of the new contract

[sourabhmarathe]

This behavior is intentional. This allows us to handle cases where we create a market that needs to be relaunched for any number of reasons. In those cases, as you've noted, users would expect migrated funds, not the original funds to be redeemable.

With that said, we think that an external distribution method would be more efficient than building redistribution into the contract itself.

[IllIllI000]

how would external distribution work when the funds are locked in the lender? Are you saying that you would use the admin rescue function to pull out the PTs, or is there something else you have in mind? That would need to be done before maturity like I mention above, or funds would be taken by the new iPT holders

[sourabhmarathe]

how would external distribution work when the funds are locked in the lender?

For the new iPT, we expect it to utilize the same PTs as the previous iPT. Because the authRedeem authorized modifier would look up the new iPT, it would ensure that the old iPT would not be able to redeem funds held by the Lender contract.

Are you saying that you would use the admin rescue function to pull out the PTs, or is there something else you have in mind?

The goal here is not to remove the funds from the Lender - we would rely on the same funds, and just use a new token for redeeming those funds.

That would need to be done before maturity It would not be ideal to use this post-maturity, but I don't understand entirely what the issue is if:

1. No new iPTs can be minted after maturity
2. Holders of the previous iPT are fairly airdropped the new iPT in the relevant amounts

I think the core of the misunderstanding here is that we would not expect the old iPT to still be used to redeem funds after setting a new iPT for a market. The goal when setting a new iPT would be to ensure that the new iPT is capable of redeeming the original funds in the Lender contract while disallowing the old iPT from redeeming those funds as well.

[IllIllI000]

I agree that the redeemer will block redemptions after maturity. Prior to maturity, how do you expect liquidity providers to know that they shouldn't be providing liquidity anymore, since the tokens will be worthless once you change the iPT? If there's no way to know, and no way to migrate outside of an airdrop, I think your liquidity will remain low

[JTraversa]

This is generally the case with most any project right? If you launch, and need secondary liquidity but have an upgradable contract, LPs are at risk?

E.g. Would this be a valid report for each money market with upgradable contracts where their cToken equivalents also trade on AMMs?

I'm unsure how you could claim that manual upgradability and our emergency procedures would be any different?

[hrishibhat]

Considering this issue a low based on the comments as this function is an admin function.