Illuminate contest - zer0dot's results

Lines of code

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

Vulnerability details

Impact

The y parameter, which is supposed to be a Yieldspace pool address, is not validated, thus, a malicious user-provided contract can be passed. This would allow a user to spoof the value returned by the IYield(y).sellBasePreview() function, causing the returned amount of iPTs to mint to be much larger.

The exploiter could then redeem the tokens with the public functions in the Redeemer contract, then redeem their own Illuminate tokens and drain the protocol of any redeemable unclaimed funds in a single transaction. Note that this also doesn't require any underlying as a u of zero can be passed as well.

Proof of Concept

Here's a quick example of a contract that could be passed as a y parameter:

...
    // Where "u" is the same as the expected value in the Lender contract
    function base() external returns (IERC20) {
        return u;
    }

    // Where "m" is the same as the expected value in the Lender contract
    function maturity() external returns (uint32) {
        return m;
    }

    // Where "iPT" is the Illuminate token is the same as the expected value in the Lender contract.
    // This can also be any arbitrary value. 
    function sellBasePreview(uint128) external returns (uint128) {
        return type(uint128).max - uint128(iPT.totalSupply());
    }

    function sellBase(address, uint128) external returns (uint128) {

    }
...

Tools Used

Manual review.

Recommended Mitigation Steps

It's probably best to store the appropriate Yieldspace pool addresses instead of allowing them to be passed from the user.

Lines of code

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

Vulnerability details

Impact

Incorrect transfer parameters prevent users from ever redeeming their iPTs. Funds should be transferred from the redeemer to the caller, not from the lender to the redeemer. The comment above the concerned code explains the expected behavior.

Proof of Concept

See the affected code here.

Tools Used

Manual review.

Recommended Mitigation Steps

Replace:

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

With

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

1. The admin can input any address in the Approve() function of the Lender contract, when it should only be the redeemer being approved. If the admin is compromised, they could potentially steal all funds.

2. Interface functions that are view functions are not marked as view in their interfaces (example taken from lender/interfaces). For a limited example, in the IYield interface, the functions base(), maturity() and sellBasePreview() are not marked as view. This could pose issues of unexpected reentrancy which may have been averted if the functions were called with staticcall.

3. Safe.sol contracts take IERC20 parameters when they could just take addresses without needing to cast them to IERC20 in the MarketPlace.sol contract. Shouldn't have an impact other than cleaner code.

4. Safe.sol assembly is not memory safe. Just something to keep in mind since the free memory pointer is never updated as this might cause undefined behaviour.

5. Safe.sol has both success() and didLastOptionalReturnCallSucceed() functions, when they're both essentially appear to be the same.

6. The only time the authorized() modifier is used with a different address than the admin as a parameter is in the Redeemer contract, once. It's probably better to just simplify it by removing the passed parameter altogether (and explicitly check the caller in the authRedeem() function). This improves readability and likely gas as well.

7. Lender setSwivel() function has an incorrect NatSpec comment.

8. MarketPlace SetAdmin() function is prone to accidents, consider implementing a pull pattern. This is often addressed by having the admin set as a timelock executor owned by a multisig or governance though.

9. Lender, line 55, there's a comment about a missing event.

The Lender contract uses post-increments, which take slightly more gas than pre-increments in for loops. Consider just using unchecked { ++i } instead of unchecked { i++} in these locations:
1 2 3