Illuminate contest - bardamu's results

Lines of code

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

Vulnerability details

Impact

As stated in the documentation:

The admin is given privilages to pause specific external principal token wrapping. This may be important in the event of external protocol insolvency or bugs. In addition, the admin is able to able to withdraw fees.

However, minting functionality in the Lender contract is impossible to pause. This would allow users to keep minting tokens corresponding to principals that have been paused due to insolvency or other issues.

Proof of Concept

Lender methods signal that they can be paused with the unpaused(p) modifier (see https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L750-L755).

/// @notice reverts on all markets where the paused mapping returns true
/// @param p principal enum value
modifier unpaused(uint8 p) {
    if (paused[p]) {
        revert Invalid('paused');
    }
    _;
}

This modifier is applied to every lend method in the contract:

• Illuminate/Yield - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L192-L198
• Swivel - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L247-L255
• Element - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L317-L326
• Pendle - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L377-L384
• Tempus - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L433-L442
• Sense - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L486-L494
• APWine - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L545-L554
• Notional - https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L597-L602

However, the mint function (https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L167-L183) does not enable the pausing feature and would allow users to continue minting Illuminate tokens for paused principals.

Tools Used

vim

Recommended Mitigation Steps

Add unpaused(p) modifier to the mint function so the administrator is able to pause it in case of protocol insolvency or bugs.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L524-L530

Vulnerability details

Impact

An attacker would be able to mint an arbitrary large amount of Illuminate tokens regardless of the provided underlying amount.

Proof of Concept

One of the parameters passed to Sense's lend method is the AMM to be used for swapping. This parameter is constructed by the caller and can therefore point to any address, including an attacker-controlled contract.

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L475-L494

[...]
/// @param x amm that is used to conduct the swap
[...]
function lend(
    uint8 p,
    address u,
    uint256 m,
    uint128 a,
    uint256 r,
    address x,
    address s
) public unpaused(p) returns (uint256) {

This method performs some fee calculations and then transfers the user specified amount of funds to the contract.

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L520-L521

// Transfer funds from user to Illuminate
Safe.transferFrom(IERC20(u), msg.sender, address(this), a);

Next step is calling the swapUnderlyingForPTs function on the provided Sense's AMM and storing the return value in the returned variable.

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L523-L524

// Swap those tokens for the principal tokens
uint256 returned = ISense(x).swapUnderlyingForPTs(s, m, lent, r);

Because x points to an arbitrary contract and not necessarily a legitimate Sense AMM, returned is completely controlled by the attacker and can be any arbitrary amount.

Finally, the contract mints returned Illuminate tokens to the caller, regardless of the original transferred amount a and relying on whatever returned quantity was returned from the external call to the attacker contract. This effectively allows the attacker to mint any desired amount of tokens.

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L526-L530

// Get the address of the ERC5095 token for this market
IERC5095 illuminateToken = IERC5095(principalToken(u, m));

// Mint the illuminate tokens based on the returned amount
illuminateToken.mint(msg.sender, returned);

Tools Used

vim

Recommended Mitigation Steps

Ensure initial transfer of funds matches minted tokens if possible, or even better, do not allow an arbitrary AMM address to indicate where to perform the swap.

Missing events for critical actions

In order to ensure proper traceability it is recommended to emit events when critical variables are updated. The following functions do not emit any events to signal value changes:

• MarketPlace.sol: setPool, setPrincipal, setAdmin
• Lender.sol: setMarketPlace, setFee, setAdmin, setSwivel, pause
• Redeemer.sol: setAdmin, setMarketPlace, setLender, setSwivel

Low liquidity could result in bad prices when selling a principal token

MarketPlace's sellPrincipalToken seems to use market value without the possibility of specifying a minimum expected amount out.

Lender does not allow resetting previous approvals

Note to reviewer: I think this one could maybe represent a MEDIUM level risk.

Lender.sol contains approve methods for single and bulk approvals. However, no way to reset these approvals is available. This means that in the case of a buggy approved redeemer or a wrong address being approved, there would be no way to revoke the approval, potentially putting funds in danger until the lender can be redeployed and funds moved to a new contract.

Multiple reentrancy vectors

Note to reviewer: I think this one could maybe represent a MEDIUM level risk.

Several functions in Lender.sol and Redeemer.sol perform external calls to user-provided contracts, therefore allowing reentrancy. While I did not manage to find a clear exploitation path, restricting reentrancy to lend() and redeem() methods would be recommended to err on the safe side.

• ISense(d).redeem(o, m, amount); where d is user-supplied
• purchased = IElement(e).swap(swap, fund, r, d); where e is user-supplied
• } else if (ISense(s).pt() != address(token)) { where s is user-supplied
• } else if (ISense(x).maturity() > m) where x is user-supplied
• uint256 returned = IAPWineRouter(pool).swapExactAmountIn(i, 1, lent, 0, r, address(this)); where pool is user-supplied
• IAave(aave).deposit(u, lent, address(this), 0); where aave is user-supplied
• uint128 returned = IYield(y).sellBasePreview(Cast.u128(a)); where y is user-supplied (see here and here for non-internal function usage)

Could use more descriptive names

Single letter variable names are used throughout the contracts. While these are in general not hard to figure out, using a more descriptive naming convention could help code readability.

> 0 is less efficient than != 0 for unsigned integers

!= 0 costs 6 less GAS compared to > 0 for unsigned integers in conditional statements with the optimizer enabled.

• Use != 0 instead of > 0 in return feenominator > 0 ? a / feenominator : 0;

++i costs less gas compared to i++

++i costs less gas compared to i++ or i += 1 for unsigned integers. This is because the pre-increment operation is cheaper (about 5 GAS per iteration).

• Use ++i in single approve
• Use ++i in bulk approve
• Use ++i in Swivel lender

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + =.) Consider replacing >= and <= with their strict counterparts > and <.

• Use > in require (block.timestamp >= when, 'withdrawal still on hold');