IllIllI - Users can accidentally lose funds during redemption

[github-actions[bot]]

IllIllI

medium

Users can accidentally lose funds during redemption

Summary

Users can accidentally lose funds during redemption

Vulnerability Detail

The redeem() function for iPTs does not ensure that each of the backing PTs have been redeemed for underlying yet.

Impact

A user may redeem before all backing PTs are redeemed (e.g. due to them being paused, or the keeper not having completed its work), thinking everything is done since maturity has passed, and will not get the full amount of underlying they are owed.

Code Snippet

The amount redeemed is a fraction of holdings, which may not have been increased to the full amount yet:

// File: src/Redeemer.sol : Redeemer.redeem()   #1

508        function redeem(address u, uint256 m) external unpaused(u, m) {
509            // Get Illuminate's principal token for this market
510            IERC5095 token = IERC5095(
511                IMarketPlace(marketPlace).markets(
512                    u,
513                    m,
514                    uint8(MarketPlace.Principals.Illuminate)
515                )
516            );
517    
518            // Verify the token has matured
519            if (block.timestamp < token.maturity()) {
520                revert Exception(7, block.timestamp, m, address(0), address(0));
521            }
522    
523            // Get the amount of tokens to be redeemed from the sender
524            uint256 amount = token.balanceOf(msg.sender);
525    
526            // Calculate how many tokens the user should receive
527            uint256 redeemed = (amount * holdings[u][m]) / token.totalSupply();
528    
529            // Update holdings of underlying
530            holdings[u][m] = holdings[u][m] - redeemed;
531    
532            // Burn the user's principal tokens
533            token.authBurn(msg.sender, amount);
534    
535            // Transfer the original underlying token back to the user
536            Safe.transfer(IERC20(u), msg.sender, redeemed);
537    
538            emit Redeem(0, u, m, redeemed, amount, msg.sender);
539:       }

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

Tool used

Manual Review

Recommendation

This is hard to solve without missing corner cases, because each external PT may have its own idosyncratic reasons for delays, and there may be losses/slippage involved when redeeming for underlying. I believe the only way that wouldn't allow griefing, would be to track the number of external PTs of each type that were deposited for minting Illuminate PTs on a per-market basis, and require() that the number of each that have been redeemed equals the minting count, before allowing the redemption of any Illuminate PTs for that market. You would also need an administrator override that bypasses this check for specific external PTs of specific maturities.

This is the same issue as was found in the prior contest, and has not been addressed. Note that in the escallation, Evert stated: Escalation accepted, enforcing a specific order of contract calls off-chain is not secure. Rewarding a medium severity.

[JTraversa]

So this is very similar to a couple tickets in our initial audit that were rejected.

The concept suggested is that external PTs may not be properly redeemed in time for the redemption of iPTs. (With #32 representing a subset of redemptions in autoredemptions)

That said, there is primarily a need in protocol design for the iPT to have a later maturity than each external PT in order to ensure efficient and timely iPT redemption.

That said, given our current solution of ensuring excess time for external PT redemption seems sufficient for time based concerns, and the suggested remediation simply breaks the protocol upon any slight external issue (preventing redemption completely) we dont see this as a reasonable report.

[IllIllI000]

The Illuminate team is on the same plane to a conference. Your keepers start to have issues and while you're debugging in-flight, the plane crashes. Everyone not on the plane is very distracted by funerals etc, and nobody thought to check the keepers. In the meantime a PT is paused to prevent an attack, and when the iPTs mature, people lose funds because the PTs couldn't be redeemed. If you have checks to ensure that redemption are completed, then it's a temporary inaccessibility of funds rather than loss of funds.

[sourabhmarathe]

In the meantime a PT is paused to prevent an attack, and when the iPTs mature, people lose funds because the PTs couldn't be redeemed. If you have checks to ensure that redemption are completed, then it's a temporary inaccessibility of funds rather than loss of funds.

I agree that a check would prevent users from prematurely redeeming funds, but in this case, wouldn't users also be locked out of redeeming funds altogether? If we force all redemptions to occur, and then somehow redemptions are not possible (in this case due to the admin key being lost), we would lock users out of all of their funds completely.

As a result, I think we're comfortable with the status quo while acknowledging that there are some cases where things can go wrong, we believe the protections we will put in place using other mechanisms will be sufficient to protect users from premature redemptions.

[JTraversa]

The Illuminate team is on the same plane to a conference. Your keepers start to have issues and while you're debugging in-flight, the plane crashes. Everyone not on the plane is very distracted by funerals etc, and nobody thought to check the keepers. In the meantime a PT is paused to prevent an attack, and when the iPTs mature, people lose funds because the PTs couldn't be redeemed. If you have checks to ensure that redemption are completed, then it's a temporary inaccessibility of funds rather than loss of funds.

This all assumes that we are the only one that can call the methods.

The methods are clearly public right?

Would it be valid to say that if there is monetary benefit to run the method, someone will do so?

[hrishibhat]

This issue was rewarded in a previous contest (https://github.com/sherlock-audit/2022-10-illuminate-judging/issues/81#issuecomment-1333467883)

The decisions made at the time were based on the context we had at the time. However, recent comments & further discussions have provided much more context around the issue, and we have decided to consider this issue a low as it's clear to us that the issue is keeper/admin related.