Illuminate contest - GimelSec's results

Lines of code

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

Vulnerability details

Impact

It doesn't spend _allowance in marketplace/ERC5095.sol. Attackers can call withdraw, redeem multiple times as the holder.

Proof of Concept

In withdraw, redeem of ERC5095.sol check the _allowance:

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

But in authRedeem it just burns tokens and it doesn't spend _allowance. If Alice approves Bob 100 amounts, Bob can spend 100 amounts again and again to call withdraw and redeem.

Tools Used

None

Recommended Mitigation Steps

Spend _allowance:

_setAllowance(owner, spender, currentAllowance - amount);

Lines of code

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

Vulnerability details

Impact

In lend, the y is fully controlled by users. Attackers can steal users' tokens if an attacker gives malicious y to users.

Proof of Concept

• Alice (attacker) creates a malicious yield pool y and gives it to Bob (victim).
• Bob calls lend of L192 with malicious y, y can easily craft values to bypass the checks in lend.
• In L219 or L229, the yield function will transfer token to y (L651): Safe.transfer(IERC20(u), y, a);

Moreover, Alice can trigger reentrancy attacks to call lend again in y (e.g. yield call IYield(y).sellBase(r, returned); to reenter lend). If Bob just set a little a (e.g. 20 amount) of lend, but Bob is very trust Illuminate and set approve max to Illuminate protocol, Alice can trigger reentrancy attack to cal lend multiple times, and amplify total amount to x10 even x100 times.

Tools Used

None

Recommended Mitigation Steps

Check yield addresses should be official, or use allowlist to prevent malicious yields.

Lines of code

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

Vulnerability details

Impact

scheduleWithdrawal should have a timeout. Admin can call scheduleWithdrawal in the beginning to bypass time lock.

Proof of Concept

In scheduleWithdrawal, it will set block.timestamp + HOLD;. But admin can call withdraw anytime by calling scheduleWithdrawal in the beginning to bypass timelock.

Tools Used

None

Recommended Mitigation Steps

scheduleWithdrawal should have a timeout:

    function scheduleWithdrawal(address e) external authorized(admin) returns (bool) {
        uint256 when = block.timestamp + HOLD;
        withdrawals[e] = when;
        timeout[e] = when + TIMEOUT;

        emit ScheduleWithdrawal(e, when);
        return true;
    }

    function blockWithdrawal(address e) external authorized(admin) returns (bool) {
        withdrawals[e] = 0;
        timeout[e] = 0;

        emit BlockWithdrawal(e);
        return true;
    }

    function withdraw(address e) external authorized(admin) returns (bool) {
        uint256 when = withdrawals[e];
        require (when != 0, 'no withdrawal scheduled');
  
        require (block.timestamp >= when, 'withdrawal still on hold');
        require (block.timestamp < timeout[e], 'timeout');
  
        withdrawals[e] = 0;
        timeout[e] = 0;
  
        IERC20 token = IERC20(e);
        Safe.transfer(token, admin, token.balanceOf(address(this)));
  
        return true;
    }