Illuminate contest - kenzo's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L411

Vulnerability details

To buy Pendle principal tokens, Illuminate executes a swap on a Sushiswap pool. The lend methods gets the number of tokens received using swapExactTokensForTokens's return value, but it accesses the amount of tokens sent, not received.

Impact

User will be minted wrong amount of tokens; will be minted the amountIn instead of the amountOut.

Proof of Concept

This is how the Pendle lend method exchanges underlying for Pendle principal, and mints accordingingly iPTs to the user:

            returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[0];
            address illuminateToken = principalToken(u, m);
            IERC5095(illuminateToken).mint(msg.sender, returned);

This is a call to a Sushiswap pool (discussed with dev). The problem is that swapExactTokensForTokens will return an array where the last element is the amountOut, but the lend method is accessing the first element, and minting that amount to the user. Therefore, the contract accounting will break and user will get wrong amount of tokens.

We can see in the UniswapV2Router that the last element is the amountOut:

        require(amounts[amounts.length - 1] >= amountOutMin, 'UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT');

Recommended Mitigation Steps

Access the second element of the array, not the first.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L116

Vulnerability details

The redeem function is checking wrongly whether msg.sender is allowed to spend holder's tokens.

Impact

Anybody can redeem anybody's token to himself. Therefore user funds will be lost.

Proof of Concept

The redeem function is taking principalAmount and parameter and returning underlyingAmount. It checks whether the sender is allowed to spend holder's tokens like so:

            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');

And then proceeds to send principalAmount to the Redeemer:

            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);     

Since underlyingAmount is 0, the sender can redeem anybody's tokens and set himself as the receiver of underlying.

Recommended Mitigation Steps

Check whether the sender's allowance is >= principalAmount, not underlyingAmount.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L187

Vulnerability details

When Redeemer redeems Element, it sends the underlying tokens to Marketplace.

Impact

All Element underlying will be lost.

Proof of Concept

The method is sending marketplace as parameter to Element:

            IElementToken(principal).withdrawPrincipal(amount, marketPlace);

This second parameter is the destination where the funds would be sent to.

Therefore, all Element underlying will be sent to the Marketplace, which has no function to retrieve them.

Recommended Mitigation Steps

Send the underlying to the Redeemer instead of the Marketplace.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L465:#L466

Vulnerability details

The Tempus lend method calculates the amount of tokens to mint as amountReturnedFromTempus - lenderBalanceOfMetaPrincipalToken. This seems wrong as there's no connection between the two items. Tempus has no relation to the iPT token.

Impact

Wrong amount of iPT will be minted to the user. If the Lender contract has iPT balance, the function will revert, otherwise, user will get minted 0 iPT tokes.

Proof of Concept

This is how the lend method calculates the amount of iPT tokens to mint:

        uint256 returned = ITempus(tempusAddr).depositAndFix(Any(x), Any(t), a - fee, true, r, d) -
            illuminateToken.balanceOf(address(this));
        illuminateToken.mint(msg.sender, returned);

The Tempus depositAndFix method does not return anything. Therefore this calculation will revert if illuminateToken.balanceOf(address(this)) > 0, or will return 0 if the balance is 0.

[Note: there's another issue here where the depositAndFix sends wrong parameters - I will submit it in another issue.]

Recommended Mitigation Steps

I believe that what you intended to do is to check how many Tempus principal tokens the contract received. So you need to check Lender's x.tempusPool().principalShare() before and after the swap, and the delta is the amount received.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L262

Vulnerability details

Sense's redeem can be totally DOSd due to user supplied input.

Impact

Using this attack, Sense market can not be redeemed.

Proof of Concept

This is how Sense market is being redeemed:

        IERC20 token = IERC20(IMarketPlace(marketPlace).markets(u, m, p));
        uint256 amount = token.balanceOf(lender);
        Safe.transferFrom(token, lender, address(this), amount);
        ISense(d).redeem(o, m, amount);

The problem is that d is user supplied input and the function only tries to redeem the amount that was transferred from Lender.

A user can supply malicious d contract which does nothing on redeem(o, m, amount). The user will then call Redeemer's redeem with his malicious contract. Redeemer will transfer all the prinicipal from Lender to itself, will call d (noop), and finish. Sense market has not been redeemed.

Now if somebody tries to call Sense market's redeem again, the amount variable will be 0, and Redeemer will try to redeem 0 from Sense.

All the original principal is locked and lost in the contract, like tears in rain.

Recommended Mitigation Steps

I think you should either use a whitelisted Sense address, or send to ISense(d).redeem Redeemer's whole principal balance.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L100

Vulnerability details

ERC5095's redeem/withdraw allows an ERC20-approved account to redeem user's tokens, but does not update the allowance after burning.

Impact

User Mal can burn more tokens than Alice allowed him to. He can set himself to be the receiver of the underlying, therefore Alice will lose funds.

Proof of Concept

withdraw and redeem functions check that the msg.sender has enough approvals to redeem the tokens:

            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');

But they do not update the allowances. They then call authRedeem, which also does not update the allowances. Therefore, an approved user could "re-use his approval" again and again and redeem whole of approver's funds to himself.

Recommended Mitigation Steps

Update the allowances upon spending.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L172

Vulnerability details

Lender's mint function does not check whether the supplied market is paused.

Impact

Even if a market is paused due to insolvency/bugs, an attacker can issue iPTs. This renders the whole pause and insolvency protection mechanism ineffective. See POC.

Proof of Concept

Let's say market P has become insolvent, and Illuminate pauses that market, as it doesn't want to create further bad debt. Let's say P's principal tokens's value has declined severely in the market because of the insolvency. An attacker can buy many worthless P principal tokens for cheap, then call Lender and mint from them iPT. The attacker is now owed underlying which belongs to the legitimate users. There won't be enough funds to repay everybody.

Recommended Mitigation Steps

Check in mint that the market is not paused.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L219 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L229 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L362 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L524 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L582

Vulnerability details

Some of the lend methods accept the protocol pool to swap in as a user parameter (eg. Yield, Sense). The lend methods then calculate the amount of iPT tokens to mint based on a function call to these user supplied contracts.

Impact

User can supply his own malicious contract to these methods. This contract will tell the methods to mint 1 trillion billion iPT tokens. The user can sell those on the market or later redeem them. Therefore user funds are at risk.

Proof of Concept

Let's look at APWine's lend method. It is taking as parameter from the user pool, the address of the APWine pool. The only place this pool is used in the function is to execute a swap, and mint to the user the amount of tokens the swap returned:

        // Swap on the APWine Pool using the provided market and params
        uint256 returned = IAPWineRouter(pool).swapExactAmountIn(i, 1, lent, 0, r, address(this));
        // Mint Illuminate zero coupons
        IERC5095(principalToken(u, m)).mint(msg.sender, returned);

So the user can supply his own contract as the pool, which will return 1 trillion billion tokens when somebody calls swapExactAmountIn. This will be the returned value, so the lend method will mint 1 trillion billion tokens to the user.

This issue (user supplied malicious external contract) is present for principals Yield (malicious y), Illuminate (malicious y), Element (malicious e), Sense (malicious x), APWine (malicious pool).

Recommended Mitigation Steps

I think Illuminate should used whitelisted addresses for these pools. Either query the external protocol for the relevant pool, or query with it whether a supplied pool is indeed registered, or create a registry in Illuminate for which pools should be used per markets[u][m][p].

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L300 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L364

Vulnerability details

The Swivel and Element lend methods pull funds from the user, get the Swivel/Element principal tokens, but do not mint iPT to the user.

Impact

User will not receive his iPTs and lose his funds.

Proof of Concept

I've linked above to the Swivel and Element functions. We can see that the functions conduct the swaps but do not mint iPTs to the user, unlike other lend methods.

Recommended Mitigation Steps

Mint iPT to the user.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L128

Vulnerability details

When redeeming iPTs via redeem, Redeemer doesn't send to the user back his funds.

Impact

Main redeem function will revert. While the user can still redeem via ERC5095's redeem, this is core functionality to the project so I believe high severity is justified.

Proof of Concept

After burning user's iPTs, redeem is doing the following transfer:

            Safe.transferFrom(IERC20(u), lender, address(this), amount);

This is wrong. The lender does not hold any underlying (other than the fees). Rather, at this point the market's PTs should have already been redeemed for the underlying which should now belong to Redeemer. Plus, the transfer is transferring these funds to Redeemer, not to the user.

Recommended Mitigation Steps

Function should transfer underlying from the Redeemer to the msg.sender.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L126

Vulnerability details

Redeemer's Illuminate redeem function takes as user supplied parameter the address to burn the tokens of.

Impact

1. User does not control his own tokens, and they can be redeemed (burned) unbeknownst to him and contrary to his plans. Can also be a problem for tax purposes I believe.
2. If users supply iPTs as liquidity in a pool like Uniswap, a malicious user can burn these tokens and make the users lose their funds (liquidity provided).
3. Uniswap pools with iPTs can be manipulated.

Proof of Concept

Redeemer's Illuminate redeem method takes the account to burn from as user supplied input:

            token.burn(o, amount);

It does check that msg.sender's balance is bigger than the amount requested to burn, but it then burns these tokens from o. Therefore the attacker can supply any address as o.

If he supplies a regular user's address, the user's assets would be redeemed (+ generating a tax event).

If he supplies a Uniswap pool address (eg. iPT-yieldPrincipalToken pair), user's assets (liquidity provided) in the pool will be lost.

Additionally, the pool's price would be manipulated (as the balances changed), allowing the attacker for example to buy the yieldPrincipalToken from the pool at a miniscule price and dump it at another pool.

Recommended Mitigation Steps

Only allow to burn the msg.sender's tokens, or whoever o ERC20-approved his tokens for.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L134:#L136

Vulnerability details

When globally-redeeming Tempus and APWine, redeem is trying to pull from the Lender the underlying token instead of the principal token.

Impact

Function will fail, markets can not be redeemed.

Proof of Concept

The functions is checking Lender's balance of principal, but then proceeds to try to pull underlying:

            // Get the amount of tokens to be redeemed from the principal token
            uint256 amount = IERC20(principal).balanceOf(lender);
            // Transfer the principal token from the lender contract to here
            Safe.transferFrom(IERC20(u), lender, address(this), amount);

Therefore the function will revert due to insufficient funds.

Recommended Mitigation Steps

Pull from Lender the principal instead of the underlying.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L193

Vulnerability details

When trying to redeem Notional market, the redeemer doesn't call the correct function.

Impact

Notional market can't be redeemed.

Proof of Concept

When redeeming Notional, Redeemer calls Notional's maxRedeem:

            amount = INotional(principal).maxRedeem(address(this));

That is only a view function which does not redeem the market:

    function maxRedeem(address owner) public view override returns (uint256) {

Therefore the market can't be redeemed for the underlying.

Recommended Mitigation Steps

Call Notional's redeem instead.

Judge has assessed an item in Issue #333 as Medium risk. The relevant finding follows:

2. Marketplace assumes tokens are already sent Description: The Marketplace looks like a convienance router contract for the lenders to easily interact with Illuminate on chain. However, in methods sellPrincipalToken, buyPrincipalToken, sellUnderlying, buyUnderlying, it assumes the user has already transferred the tokens to the Marketplace. This is a bit unusual as usually router-type contracts pull the funds from the user using transferFrom. This is a valid design, but I'm just bringing it to your attention. Code reference: here Impact/Mitigation: Make sure this is documented thoroughly and users are aware they should send their funds atomically to prevent an MEVr from claiming their tokens to themselves. Also, consider changing the implementation to pull the funds from the user.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L78 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L93

Vulnerability details

Lender contract's approve method allows the owner to approve any address to transfer the principal tokens held by the Lender.

Impact

This is a rug vector - compromised admin can approve itself and steal all the tokens. Lender has a scheduleWithdrawal mechanism to allow the owner to transfer out tokens only after certain timelock. The issue I'm describing allows the owner to transfer the principal tokens (which belong to the users) immediately. Thereby rendering the timelock ineffective.

Proof of Concept

The approve method gives max allowance for the admin supplied address (r) for all the principal tokens. It is supposed to give max allowance to the Redeemer (as the comment says), but the admin can supply any address there. Therefore the admin can rug and this renders the withdrawal timelock ineffective.

Recommended Mitigation Steps

Instead of allowing any arbitrary address as input, read the Redeemer address from the saved marketplace and approve only that address.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L297

Vulnerability details

The Swivel lend method adds to fees[u] the order fee, but does not pull that fee from the user. It only pulls the order-post-fee amount.

Impact

withdrawFee will fail, as it tries to transfer more tokens than are in the contract.

Proof of Concept

The Swivel lend method sums up the fees to totalFee, and the amount to send to Swivel in lent:

                    totalFee += fee;
                    // Amount lent for this order
                    uint256 amountLent = amount - fee;
                    // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                    lent += amountLent;

It then increments fees[u] by totalFee, but only pulls from the user lent:

            fees[u] += totalFee;
            // transfer underlying tokens from user to illuminate
            Safe.transferFrom(IERC20(u), msg.sender, address(this), lent);

Therefore, totalFee has not been pulled from the user. The fees variable now includes tokens which are not in the contract, and withdrawFee will fail as it tries to transfer fees[u].

Recommended Mitigation Steps

Pull lent + totalFee from the user.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L219 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L229

Vulnerability details

The lend method for Yield and Illuminate deducts the protocol fee from the user, but does not increment the fees variable.

Impact

Wrong accounting, admin won't be able to properly pull the fee using withdrawFee. (Admin can still withdraw using scheduleWithdrawal, but obviously the fee accounting is implemented wrongly in this function.)

Proof of Concept

withdrawFee sends to the admin only whatever amount is saved in fees[u]. The Illuminate\Yield lend method deducts the fee ((1), (2)) when sending tokens to Yield, but does not increment the fees variable. Therefore the fees are not accounted for.

Recommended Mitigation Steps

Like is being done in the rest of the functions, this function should set fees[u] += calculateFee(a).

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L174

Vulnerability details

Lender's mint function allows a user to re-wrap/re-mint an already wrapped iPT.

Impact

Extra iPTs will be minted which have no corresponding underlying. The original iPT will be pulled to Lender.sol (and can be pulled by the admin). This creates "bad debt"/liabilities for iPT - the supply does not match the underlying.

Proof of Concept

This is the mint method:

    function mint(uint8 p, address u, uint256 m, uint256 a) public returns (bool) {
        address principal = IMarketPlace(marketPlace).markets(u, m, p);
        Safe.transferFrom(IERC20(principal), msg.sender, address(this), a);
        IERC5095(principalToken(u, m)).mint(msg.sender, a);
        emit Mint(p, u, m, a);
        return true;
    }

So if a user sends principal 0 (iPT), Lender will pull the user's iPT, and will mint him brand new iPT. Now iPT's supply does not match the liabilities any more.

Recommended Mitigation Steps

Do not accept principal 0 in the mint function.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L255

Vulnerability details

A user can use Swivel's lend method to issue iPTs from Swivel even if the Swivel market is paused.

Impact

Pause functionality for Swivel is ineffective and can cause for loss of funds for users in case of Swivel insolvency/bugs.

Proof of Concept

Swivel's lend function has unpaused(p) modifier, but does not use p anywhere else, and does not check whether the p supplied is Swivel. Therefore, a user could pass a different p to the function and mint iPTs through Swivel even if Swivel is set to be paused. Then when the market is matured, even if Swivel can not be fully redeemed, the user is still due his allotment of underlying from the unified iPT. So the user can redeem his iPT before other users, thereby taking their tokens. iPT's balance sheet would be off.

Recommended Mitigation Steps

Add a check that reverts if p is not Swivel.

1. Wrong log emitted

Description: Redeemer's redeem method for Tempus/APWine emits wrong prinicipal (always emits 0) on the log. Impact: Wrong log emitted, perhaps front-ends confused. Code reference: here Mitigation: emit p instead of 0.

2. Marketplace assumes tokens are already sent

Description: The Marketplace looks like a convienance router contract for the lenders to easily interact with Illuminate on chain. However, in methods sellPrincipalToken, buyPrincipalToken, sellUnderlying, buyUnderlying, it assumes the user has already transferred the tokens to the Marketplace. This is a bit unusual as usually router-type contracts pull the funds from the user using transferFrom. This is a valid design, but I'm just bringing it to your attention. Code reference: here Impact/Mitigation: Make sure this is documented thoroughly and users are aware they should send their funds atomically to prevent an MEVr from claiming their tokens to themselves. Also, consider changing the implementation to pull the funds from the user.

3. No two-step process for setting admin

Description: The various contracts can change an admin instantly, without the new account having to claim it's ownership. Impact: If there was an (admittedly unlikely) error in the setting of the new admin, this error is irreversible and parts of the protocol would be completely bricked. Code ref: Redeemer, Marketplace, Lender Mitigation: Consider changing the functions to a two-step process, where a new admin address is proposed, and then the new address has to accept the ownership transfer.

4. Lender protocol fees are not capped

Description: The admin can change instantly the fee. There is no cap on the fee amount. Impact: Admin may set the fee to be 100% and steal user's funds. Code ref: here Mitigation: Consider capping the fee to a reasonable amount, eg. 5%.