Illuminate contest - StErMi's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L92-L103 https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L108-L119

Vulnerability details

Impact

When a user call Lender lend function it get back the corresponding ERC5095 iPT token, then after some time will be redeemable.

After following protocol the flow, the user must call redeem or withdraw on the ERC5095 iPT token that will call the corresponding Redeemer that will burn the specified amount of principal and send the 1:1 underlying amount to the user.

Both the redeem or withdraw function have the same signature and code. For explanation, let's take the redeem function

function redeem(
    uint256 principalAmount,
    address receiver,
    address holder
) external override returns (uint256 underlyingAmount) {
    if (block.timestamp < maturity) {
        revert Maturity(maturity);
    }
    if (holder == msg.sender) {
        return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, principalAmount);
    } else {
        require(_allowance[holder][msg.sender] >= underlyingAmount, "not enough approvals");
        return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);
    }
}

As you can see, the code is handling the case where the msg.sender is not the holder. This is the case where holder have approved some principalAmount to the msg.sender via the approve method.

In this scenario, the msg.sender can redeem the underlying amount from the holder and send them to the receiver address.

The Redeemer.authRedeem code is the following

function authRedeem(
    address u,
    uint256 m,
    address f,
    address t,
    uint256 a
) public authorized(IMarketPlace(marketPlace).markets(u, m, 0)) returns (bool) {
    // Get the principal token for the given market
    IERC5095 pt = IERC5095(IMarketPlace(marketPlace).markets(u, m, 0));

    // Make sure the market has matured
    if (block.timestamp < pt.maturity()) {
        revert Invalid("not matured");
    }

    // Burn the user's principal tokens
    pt.burn(f, a);

    // Transfer the original underlying token back to the user
    Safe.transfer(IERC20(u), t, a);
    return true;
}

The contract makes some validity check, burn the requested amount from the holder account and transfer the asked principalAmount from the holder to the receiver.

In both the ERC5095 and Redeemer contract, the allowance of the msg.sender is updated, reducing the allowance by the amount of token that have been transferred.

This mean that the spender (msg.sender) will always be able to transfer to itself _allowance[holder][msg.sender] amount of tokens until the holder balance have been drained. This would happen even in the case that the holder lend new tokens to the Illuminate that will be reedemable in the future.

Proof of Concept

Scenario:

1. Alice lend 1000 USDC to Illuminati and get back 5000 the corresponding iPT (ERC5095)
2. Alice call iPT.approve(Bob, 1000)
3. Time pass and Alice can redeem the underlying token
4. Bob call Redeemer Redeemer.redeem
5. Bob call iPT.redeem(1000, Bob, Alice) (amount, receiver, holder) redeeming 1000 USDC (his allowance)
6. Bob can call again and again the function because his allowance for the Alice balance is never updated. He can call the method until Alice has enough iPT to be redeemed. He will be able to call again that method even after draining all the Alice funds if Alice lend again to the protocol and the time to redeem again has passed

Tools Used

Manual review

Recommended Mitigation Steps

Update the redeem and withdraw function in the ERC5095 contract to update the msg.sender allowance when msg.sender != holder.

Use self-explanatory variable name instead of abbreviations to increase readability

With variable names like p, u, m, a, and so on it become really difficult to read the code for developers, auditors and users (those natspec comments will also be used by external tools/services like Etherscan).

Recommendation: rename variable names to be self-explanatory

Mixed use of require and custom error

The code seems to use a mix of require and custom errors to revert the transaction. Consider using only a pattern to be more consistent across the project. Custom error are usually gas cheaper compared to require.

Missing event emission

Event emission are important to monitor contract activity with external tool/services. Consider adding event emission to the following function:

Redeemer.sol

• setAdmin
• setMarketPlace
• setLender
• setLender
• setSwivel

MarketPlace.sol

• setPrincipal
• setAdmin
• setPool
• sellPrincipalToken
• buyPrincipalToken
• sellUnderlying
• buyUnderlying
• mint
• mintWithUnderlying
• burn
• burnForUnderlying

Lender.sol

• approve (both of them)
• setAdmin
• setFee
• setMarketPlace
• setSwivel
• withdrawFee
• pause

Consider implementing the event emission for these functions

Lender setFee allow the admin to drain all the user's lent funds

feenominator is initialized in the contract's constructor as feenominator = 1000; but the admin of the contract can update that value via setFee.

The setFee function has no check on the max value that the feenominator state variable could be updated to.

Consider adding a max value to the function.

Lender setSwivel natspec is wrong

The current natspec for the function say /// @notice sets the feenominator to the given value but the function is updating the swivelAddr state variable and not the feenominator.

Consider updating the natspec comment to correctly describe what the function does.

Lender withdraw should also reset the fees variable

When the withdraw(token) method is called, the function is transferring all the amount of token from the Lender contract to admin.

Before the transfer is called, the fees[token] state variable should also be resetted if fees[token] > 0