Users can loose their Illuminate tokens if amount to redeem is greater than holdings[u][m]

Summary

When a user tries to redeem Illuminate tokens (using the Redeemer contract), the call will burn his/her illuminate tokens in exchange of zero underlying tokens if the amount to redeem exceeds the holdings value for that [underlying, maturity] pair.

Vulnerability Detail

Holdings mapping for a [underlying, maturity] pair is only increased in certain function calls. redeem method for Swivel, Yield, Element, Pendle, APWine, Tempus and Notional protocols https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L329 redeem method signature for Sense https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L394 But it is decreased in a number of other places, for instance in this function: https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L403-L434 Which burns Illuminate principal tokens and sends underlying to user. Acording to the documentation,

As an alternative to directly lending through Illuminate, users can also purchase external principal tokens and then wrap them at a 1:1 ratio into Illuminate Principal Tokens. As an example, let's say a user lends 100 USDC directly on Notional in the December 2022 market at a rate of 5% for one year. This leaves the user with 105 Notional PTs.

By then calling mint on Lender.sol, this user can then wrap their 105 Notional PTs into 105 Illuminate PTs (likely in order to perform arbitrage). Lender: holds 105 Notional (External) PTs User: holds 105 Illuminate PTs

So it could happen that a user minted Illuminate tokens, and after maturity try to redeem the underlying before any call has been made to the redeem functions above (the ones that increase the holdings). This means that holdings[u][m] would be zero and the call to redeem(address u, uint256 m) by the user would just burn their Illuminate principal in return for nothing. https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L422-L431 Note that in line 422, the redeemed amount is zero because holdings[u][m] is zero. So in line 428, the Illuminate tokens are burnt and in line 431 zero (redeemed) underlying is transferred to the user. This issue is present also in funtions autoRedeem and authRedeem because both calculate the amount of underlying to redeem as uint256 redeemed = (amount * holdings[u][m]) / pt.totalSupply();. For the sake of simplicity, I only present below a PoC of the case of the redeem(address u, uint256 m) function to prove the loss of funds.

Impact

Loss of user funds in certin scenarios.

Code Snippet

For the case of the redeem(address u, uint256 m) function of the Redeemer contract, I wrote the following test that can be included in Redeemer.t.sol.

function testIssueIlluminateRedeem() public {
    // Deploy market
    deployMarket(Contracts.USDC, 0);

    address principalToken = mp.token(Contracts.USDC, maturity, uint256(MarketPlace.Principals.Yield));
    address illuminateToken = mp.token(Contracts.USDC, maturity, uint256(MarketPlace.Principals.Illuminate));

    // Give msg.sender principal (Yield) tokens
    deal(principalToken, msg.sender, startingBalance);

    // approve lender to transfer principal tokens
    vm.startPrank(address(msg.sender));
    IERC20(principalToken).approve(address(l), startingBalance);

    // In the starting state, the balance of Yield tokens for the user is equal to startingBalance
    // and the balance of Illuminate tokens is zero
    // Both the USDC balance of the user and the holdings mapping in the Redeemer for [u][m] is zero
    assertEq(IERC20(principalToken).balanceOf(msg.sender), startingBalance);
    assertEq(IERC20(illuminateToken).balanceOf(msg.sender), 0);
    assertEq(IERC20(Contracts.USDC).balanceOf(msg.sender), 0);
    assertEq(r.holdings(Contracts.USDC, maturity), 0);

    // User mints Illuminate tokens by wrapping his/her 1_000 Yield principal tokens
    l.mint(
        uint8(MarketPlace.Principals.Yield), // Yield
        Contracts.USDC, 
        maturity, 
        1_000
    );
    vm.stopPrank();

    // After minting, the balance of Yield tokens for the user is 1_000 less than the starting balance
    // and the balance of Illuminate tokens is 1_000
    // Both the USDC balance of the user and the holdings mapping in the Redeemer for [u][m] is zero
    assertEq(IERC20(principalToken).balanceOf(msg.sender), startingBalance - 1_000);
    assertEq(IERC20(illuminateToken).balanceOf(msg.sender), 1_000);
    assertEq(IERC20(Contracts.USDC).balanceOf(msg.sender), 0);
    assertEq(r.holdings(Contracts.USDC, maturity), 0);

    assertEq(r.holdings(Contracts.USDC, maturity), 0);

    // Try to redeem the underlying as msg.sender
    vm.prank(msg.sender);
    r.redeem(Contracts.USDC, maturity);

    // After redeeming, the balance of Yield tokens for the user is 1_000 less than the starting balance
    // and the balance of Illuminate tokens is zero, they have been burnt
    // The holdings mapping in the Redeemer for [u][m] is zero
    // But the USDC balance of the user is also zero, meaning the user has received nothing in return for 
    // burning their Illuminate
    assertEq(IERC20(principalToken).balanceOf(msg.sender), startingBalance - 1_000);
    assertEq(IERC20(illuminateToken).balanceOf(msg.sender), 0);
    assertEq(IERC20(Contracts.USDC).balanceOf(msg.sender), 0);
    assertEq(r.holdings(Contracts.USDC, maturity), 0);
}

You can see how the user mints Illuminate from Yield tokens, then redeems through the Redeemer and ends up with the loss of the Yield tokens he/she used to mint Illuminate.

Tool used

Forge tests and manual Review

Recommendation

Using the holdings mapping to track the redeemable Illuminate tokens in the Redeemer contract can only be done if there is no way for an address to have a positive Illuminate tokens balance without the knowledge of the Redeemer contract. I think the team should rethink the way this contract works.

Duplicate of #239

sourabhmarathe commented 1 year ago

We expect the user to confirm that the redemption process is fully complete prior to redeeming their Illuminate PTs. We believe this check is best done off-chain and as a result do not include it in Illuminate's redeem method.

neumoxx commented 1 year ago

Escalate for 100 USDC The fact that a call to redeem can leave the user without his Illuminate tokens and without his principal, and relying on doing this offchain doesn't feel right. I point out in my report the case of holdings[u][m] being equal to zero and burning the full amount the user tries to redeem, but in fact it's more general of an issue. This line (422 in Redeemer.sol)

uint256 redeemed = (amount * holdings[u][m]) / token.totalSupply();

Gets the amount to redeem. But this other line (428 in Redeemer.sol)

token.authBurn(msg.sender, amount);

burns the amount passed in. So if a user has (to make it simple) holdings[u][m] = 2 but has minted another token via lender.mint (see my report above), so totalSupply = 3 and passes an amount of 3 to the function, the value of redeemed would be 2 but the function would burn all three tokens. Please, give it a second look because it's a big issue imo.

sherlock-admin commented 1 year ago

Escalate for 100 USDC The fact that a call to redeem can leave the user without his Illuminate tokens and without his principal, and relying on doing this offchain doesn't feel right. I point out in my report the case of holdings[u][m] being equal to zero and burning the full amount the user tries to redeem, but in fact it's more general of an issue. This line (422 in Redeemer.sol)
uint256 redeemed = (amount * holdings[u][m]) / token.totalSupply();
Gets the amount to redeem. But this other line (428 in Redeemer.sol)
token.authBurn(msg.sender, amount);
burns the amount passed in. So if a user has (to make it simple) holdings[u][m] = 2 but has minted another token via lender.mint (see my report above), so totalSupply = 3 and passes an amount of 3 to the function, the value of redeemed would be 2 but the function would burn all three tokens. Please, give it a second look because it's a big issue imo.

You've created a valid escalation for 100 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 commented 1 year ago

Escalation accepted, enforcing a specific order of contract calls off-chain is not secure. Rewarding a medium severity.

sherlock-admin commented 1 year ago

Escalation accepted, enforcing a specific order of contract calls off-chain is not secure. Rewarding a medium severity.

This issue's escalations have been accepted!

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

JTraversa commented 1 year ago

I would comment that this is less of a vulnerability and more of an admin issue given it would actually rely on those creating markets to make errors in doing so, allowing no window for keepers or otherwise to themselves call redeem methods for external pts.

You should not rely on an off chain check, you should rely on keepers and sufficient time between external principal token maturity and iPT maturity to ensure users do not need to check anything whatsoever. (what we do now / a ton of mechanisms share similar time based assumptions e.g. optimistic anythings )

sherlock-audit / 2022-10-illuminate-judging

neumo - Users can loose their Illuminate tokens if amount to redeem is greater than holdings[u][m] #81