Illuminate contest - Chom's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/ERC5095.sol#L116 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L275-L296

Vulnerability details

Impact

ERC5095 redeem must check allowance with principalAmount instead of underlyingAmount. Allowing any user to redeem token of anybody freely. This is very critical, anyone can lost their money everytime without their acknowledgement.

Proof of Concept

function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){
...
            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);

underlyingAmount is a return value which is always 0 on that require statement, so it can be simplify to _allowance[holder][msg.sender] >= 0 which means always allow every redeem request.

By calling redeem on principal token -> authRedeem on Redeemer contract. You can redeem money on behalf of anybody you want without any approval requirement.

Tools Used

Manual

Recommended Mitigation Steps

Allowance must be checked with principalAmount which is an input

            require(_allowance[holder][msg.sender] >= principalAmount, 'not enough approvals');
            return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);

Lines of code

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

Vulnerability details

Impact

User can send malicious address pool, address aave to lend function to mint unlimited principal token.

Proof of Concept

Consider these malicious AAVE pool and APWineRouter are being sent to the lend function

Malicious AAVE pool

function deposit(address u, uint256 lent, address a, uint256 x) public {}

Overwrite deposit to do nothing

Malicious APWineRouter

function swapExactAmountIn(uint256 i, uint256 i1, uint256 lent, uint256 i2, uint256 r, address a) public returns(uint256) {
    return 1000000000 ether;
}

Override swapExactAmountIn to do nothing and return a extremely high number.

There aren't any check on AAVE pool and APWineRouter

        // Instantiate market and tokens
        address principal = IMarketPlace(marketPlace).markets(u, m, p);
        if (IAPWineToken(principal).getUnderlyingOfIBTAddress() != u) {
            revert NotEqual('underlying');
        }

How the code run

1. Call deposit into malicious AAVE pool but actually do nothing (No deposit happened)
2. Call swapExactAmountIn into malicious APWineRouter but actually nothing is swapped but swapExactAmountIn return a extremely high amount which is then stored into returned
3. returned Principal token is minted to attacker. Noticed that returned is now extremely high, so it mint too much principal token to the attacker.
4. Attacker use that principal token minted from air to redeem unlimited real fund.

Tools Used

Manual

Recommended Mitigation Steps

You must whitelist valid AAVE pool and APWineRouter. And perform check to assert whether AAVE pool and APWineRouter is whitelisted before proceed to the minting logic.

Lines of code

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

Vulnerability details

Impact

Transfer the original underlying token back to the user but actually transfer from lender to redeemer. Underlying token may be locked forever and user may not receive their fund after redeeming.

Proof of Concept

            // Transfer the original underlying token back to the user
            Safe.transferFrom(IERC20(u), lender, address(this), amount);

Comment: Transfer the original underlying token back to the user Code: Obviously transfer the original underlying token to the redeemer

Tools Used

Manual

Recommended Mitigation Steps

            // Transfer the original underlying token back to the user
            Safe.transferFrom(IERC20(u), lender, msg.sender, amount);

Lines of code

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

Vulnerability details

Impact

Everyone can burn principal token of any other user. It is very critical that you may lost your principal token anytime without any permission.

Proof of Concept

            // Burn the prinicipal token from Illuminate
            token.burn(o, amount);

    function redeem(
        uint8 p,
        address u,
        uint256 m,
        address o
    ) public returns (bool) {

This is a public function without any permission check, anyone can pass arbitrary o with arbitrary amount and principal token of o will get burned by amount

Tools Used

Manual

Recommended Mitigation Steps

Should burn from msg.sender. Not o.

token.burn(msg.sender, amount);

Lines of code

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

Vulnerability details

Impact

Transfer the principal token from the lender contract to here but actually transfer underlying token from lender to redeemer.

Proof of Concept

            // Transfer the principal token from the lender contract to here
            Safe.transferFrom(IERC20(u), lender, address(this), amount);

It is transferring IERC20(u) which is underlying token instead of IERC20(principal)

Tools Used

Manual

Recommended Mitigation Steps

            // Transfer the principal token from the lender contract to here
            Safe.transferFrom(IERC20(principal), lender, address(this), amount);

Change your variable name

Currently, it is too terrible name, please change it to human readable name. For example

    function createMarket(
        address u,
        uint256 m,
        address[8] memory t,
        string calldata n,
        string calldata s,
        uint8 d
    ) 

to

    function createMarket(
        address underlying,
        uint256 maturity,
        address[8] memory principals,
        string calldata name,
        string calldata symbol,
        uint8 decimals
    ) 

That better right?

Validate valid principal token before redeem

In each redeem function in Redeemer contract

        // Get the principal token that is being redeemed by the user
        address principal = IMarketPlace(marketPlace).markets(u, m, p);

There aren't any check whether principal token is valid or not. Check can be added

        // Get the principal token that is being redeemed by the user
        address principal = IMarketPlace(marketPlace).markets(u, m, p);

        if (principal == address(0)) revert Invalid('principal');