081.md

neumo

high

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.