Illuminate contest - BowTiedWardens's results

Overview

Table of Contents

• Low Risk Issues
  • 1. Check Effects Interactions pattern not respected
  • 2. Fees should be upper-bounded
  • 3. Missing events for critical parameters that change the state
• Non-Critical Issues
  • 1. Typos

Low Risk Issues

1. Check Effects Interactions pattern not respected

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.

Consider always moving the state-changes before the external calls.

Affected code:

lender/Lender.sol:
  331          Safe.transferFrom(IERC20(u), msg.sender, address(this), a);
  332  
  333          // Track the accumulated fees
  334:         fees[u] += calculateFee(a); //@audit CEIP not respected

  389          Safe.transferFrom(IERC20(u), msg.sender, address(this), a);
  390  
  391          // Add the accumulated fees to the total
  392          uint256 fee = calculateFee(a);
  393:         fees[u] += fee; //@audit CEIP not respected

  444          Safe.transferFrom(underlyingToken, msg.sender, address(this), a);
  445  
  446          // Add the accumulated fees to the total
  447          uint256 fee = calculateFee(a);
  448:         fees[u] += fee; //@audit CEIP not respected

  548          Safe.transferFrom(IERC20(u), msg.sender, address(this), a);
  549  
  550          // Determine the fee
  551          uint256 fee = calculateFee(a);
  552  
  553          // Add the accumulated fees to the total
  554:         fees[u] += fee; //@audit CEIP not respected

  598          Safe.transferFrom(IERC20(u), msg.sender, address(this), a);
  599  
  600          // Add the accumulated fees to the total
  601          uint256 fee = calculateFee(a);
  602:         fees[u] += fee; //@audit CEIP not respected

2. Fees should be upper-bounded

There should be an upper limit to reasonable fees

• https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L137-L140

File: Lender.sol
134:     /// @notice sets the feenominator to the given value
135:     /// @param f the new value of the feenominator, fees are not collected when the feenominator is 0
136:     /// @return bool true if successful
137:     function setFee(uint256 f) external authorized(admin) returns (bool) {
138:         feenominator = f;
139:         return true;
140:     }

3. Missing events for critical parameters that change the state

• lender/Lender.sol:

  129:     function setAdmin(address a) external authorized(admin) returns (bool) {
  130          admin = a;
  131          return true;
  132      }

  137:     function setFee(uint256 f) external authorized(admin) returns (bool) {
  138          feenominator = f;
  139          return true;
  140      }

  145:     function setMarketPlace(address m) external authorized(admin) returns (bool) {
  146          if (marketPlace != address(0)) {
  147              revert Exists(marketPlace);
  148          }
  149          marketPlace = m;
  150          return true;
  151      }

  156:     function setSwivel(address s) external authorized(admin) returns (bool) {
  157          swivelAddr = s;
  158          return true;
  159      }

• marketplace/MarketPlace.sol:

   98:     function setPrincipal(uint8 p, address u, uint256 m, address a) external authorized(admin) returns (bool) {
   99          if (markets[u][m][p] != address(0)) {
  100              revert Exists('Market already exists');
  101          }
  102          markets[u][m][p] = a;
  103          return true;
  104      }

  109:     function setAdmin(address a) external authorized(admin) returns (bool) {
  110          admin = a;
  111          return true;
  112      }

  119:     function setPool(
  120          address u,
  121          uint256 m,
  122          address a
  123      ) external authorized(admin) returns (bool) {
  124          if (pools[u][m] != address(0)) {
  125              revert Exists('pool already exists');
  126          }
  127          pools[u][m] = a;
  128          return true;
  129      }

• redeemer/Redeemer.sol:

  62:     function setAdmin(address a) external authorized(admin) returns (bool) {
  63          admin = a;
  64          return true;
  65      }

  70:     function setMarketPlace(address m) external authorized(admin) returns (bool) {
  71          if (marketPlace != address(0)) {
  72              revert Exists('marketplace');
  73          }
  74          marketPlace = m;
  75          return true;
  76      }

  81:     function setLender(address l) external authorized(admin) returns (bool) {
  82          if (lender != address(0)) {
  83              revert Exists('lender');
  84          }
  85          lender = l;
  86          return true;
  87      }

  92:     function setSwivel(address s) external authorized(admin) returns (bool) {
  93          swivelAddr = s;
  94          return true;
  95      }

Non-Critical Issues

1. Typos

• withdrawl

lender/Lender.sol:22:    /// @notice minimum amount of time the admin must wait before executing a withdrawl

• unsighed

lender/Lender.sol:83:        // max is the maximum integer value for a 256 unsighed integer

• gauruntee

lender/Lender.sol:487:        if (token.underlying() != u) { // gauruntee the input token is the right token
lender/Lender.sol:491:        } else if (ISense(x).maturity() > m) { // gauruntee the input amm has the correct maturity

• remaing

lender/Lender.sol:631:        // send the remaing amount to the given yield pool

• avaialable

marketplace/MarketPlace.sol:11:/// @notice This contract is in charge of managing the avaialable principals for each loan market.

• prinicpal

redeemer/Redeemer.sol:125:            // Burn the prinicipal token from Illuminate
redeemer/Redeemer.sol:189:            // Redeems prinicipal tokens from yield
redeemer/Redeemer.sol:237:    /// @param d Sense contract that splits the loan's prinicpal and yield

Overview

Table of Contents:

• 1. Unchecking arithmetics operations that can't underflow/overflow
• 2. Caching storage values in memory
• 3. Cheap Contract Deployment Through Clones
• 4. Use calldata instead of memory
• 5. <array>.length should not be looked up in every loop of a for-loop
• 6. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 7. It costs more gas to initialize variables with their default value than letting the default value be applied
• 8. Some variables should be immutable
• 9. Use Custom Errors instead of Revert Strings to save Gas

1. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

By keeping in mind the following function (calculateFee) which makes it so that a - calculateFee(a) > 0:

File: Lender.sol
661:     function calculateFee(uint256 a) internal view returns (uint256) {
662:         return feenominator > 0 ? a / feenominator : 0;
663:     }

Consider wrapping with an unchecked block here:

• File: Lender.sol

- 219:             uint256 returned = yield(u, y, a - calculateFee(a), address(this));
+ 219:             unchecked { uint256 returned = yield(u, y, a - calculateFee(a), address(this)); }

- 229:            uint256 returned = yield(u, y, a - calculateFee(a), msg.sender);
+ 229:            unchecked { uint256 returned = yield(u, y, a - calculateFee(a), msg.sender); }

- 400:        uint256 returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[0];
+ 400:       unchecked { uint256 returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[0]; }

- 502:        uint256 lent = a - fee;
+ 502:        unchecked { uint256 lent = a - fee; }

- 557:        uint256 lent = a - fee;
+ 557:        unchecked { uint256 lent = a - fee; }

- 605:        uint256 returned = token.deposit(a - fee, address(this));
+ 605:        unchecked { uint256 returned = token.deposit(a - fee, address(this)); }

2. Caching storage values in memory

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

• feenominator: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L662

File: Lender.sol
661:     function calculateFee(uint256 a) internal view returns (uint256) {
662:         return feenominator > 0 ? a / feenominator : 0;
663:     }

• lender: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L134-L136

File: Redeemer.sol
134:             uint256 amount = IERC20(principal).balanceOf(lender);
135:             // Transfer the principal token from the lender contract to here
136:             Safe.transferFrom(IERC20(u), lender, address(this), amount);

• lender: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L177-L180

File: Redeemer.sol
177:         uint256 amount = IERC20(principal).balanceOf(lender);
178: 
179:         // Transfer the principal token from the lender contract to here
180:         Safe.transferFrom(IERC20(principal), lender, address(this), amount);

• marketPlace: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L164-L187

File: Redeemer.sol
164:         address principal = IMarketPlace(marketPlace).markets(u, m, p);
...
187:             IElementToken(principal).withdrawPrincipal(amount, marketPlace);

• lender: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L221-L224

File: Redeemer.sol
221:         uint256 amount = token.balanceOf(lender);
222: 
223:         // Transfer the user's tokens to the redeem contract
224:         Safe.transferFrom(token, lender, address(this), amount);

• lender: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L256-L259

File: Redeemer.sol
256:         uint256 amount = token.balanceOf(lender);
257: 
258:         // Transfer the user's tokens to the redeem contract
259:         Safe.transferFrom(token, lender, address(this), amount);

3. Cheap Contract Deployment Through Clones

marketplace/MarketPlace.sol:80:        address iToken = address(new ERC5095(u, m, redeemer, lender, n, s, d));

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

Consider applying a similar pattern.

4. Use calldata instead of memory

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memoryindex. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Structs have the same overhead as an array of length one

When arguments are read-only on external functions, the data location should be calldata:

marketplace/MarketPlace.sol:70:        address[8] memory t,

5. <array>.length should not be looked up in every loop of a for-loop

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, Consider storing the array's length in a variable before the for-loop, and use this new variable instead:

lender/Lender.sol:265:            for (uint256 i = 0; i < o.length; ) {

6. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

lender/Lender.sol:96:                i++;
lender/Lender.sol:120:                i++;
lender/Lender.sol:283:                    i++;

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

7. It costs more gas to initialize variables with their default value than letting the default value be applied

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

lender/Lender.sol:265:            for (uint256 i = 0; i < o.length; ) {

Consider removing explicit initializations for default values.

8. Some variables should be immutable

These variables are only set in the constructor and are never edited after that:

lender/Lender.sol:26:    address public admin;
lender/Lender.sol:28:    address public marketPlace;
lender/Lender.sol:33:    address public swivelAddr;
marketplace/MarketPlace.sol:41:    address public admin;
redeemer/Redeemer.sol:19:    address public admin;
redeemer/Redeemer.sol:21:    address public marketPlace;
redeemer/Redeemer.sol:23:    address public lender;
redeemer/Redeemer.sol:27:    address public swivelAddr;
redeemer/Redeemer.sol:33:    address public apwineAddr;

Consider marking them as immutable, as it would avoid the expensive storage-writing operation (around 20 000 gas)

9. Use Custom Errors instead of Revert Strings to save Gas

As this is the case in almost the whole solution, consider also using custom errors here:

lender/Lender.sol:691:        require (when != 0, 'no withdrawal scheduled');
lender/Lender.sol:693:        require (block.timestamp >= when, 'withdrawal still on hold');