Illuminate contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L167-L172 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L706-L720 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L134-L140

Vulnerability details

Impact

The pause-related functionality pauses user lending, but does not prevent them from minting, which may allow users to initiate actions while the contracts are unsafe.

The admin account is behind a three-day timelock for withdrawals, but if the account is compromised, three days is short enough that people not actively monitoring things may have their funds stolen, and the funds are sent to the same admin account that may be compromised.

The admin can change the feenominator to 100% and steal new user funds

Even if the admin is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation.

Proof of Concept

No paused modifier on mint():

File: lender/Lender.sol   #1

167       function mint(
168           uint8 p,
169           address u,
170           uint256 m,
171           uint256 a
172       ) public returns (bool) {

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

Admin withdrawals are sent to the same address as may have been compromised:

File: lender/Lender.sol   #2

706       /// @param e Address of token to withdraw
707       /// @return bool true if successful
708       function withdraw(address e) external authorized(admin) returns (bool) {
709           uint256 when = withdrawals[e];
710           require (when != 0, 'no withdrawal scheduled');
711     
712           require (block.timestamp >= when, 'withdrawal still on hold');
713     
714           withdrawals[e] = 0;
715     
716           IERC20 token = IERC20(e);
717           Safe.transfer(token, admin, token.balanceOf(address(this)));
718     
719           return true;
720       }

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

No validations on fee percentages:

File: lender/Lender.sol   #3

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       }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Add the unpaused modifier to mint(), specify the destination address for withdrawals during contract construction, and have an upper limit on the fee percentage

Summary

Low Risk Issues

Total: 8 instances over 2 issues

Non-critical Issues

Total: 59 instances over 10 issues

Low Risk Issues

1. Incorrect comments

There are 2 instances of this issue:

File: redeemer/Redeemer.sol   #1

/// @audit funds are not returned to the user - they still have to use the token like all the other methods
100      /// this redeemer contract. Consequently, only Illuminate's redeem returns funds
101:     /// to the user.

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

File: redeemer/Redeemer.sol   #2

/// @audit sets the swivelAddr, not the feenominator
89:      /// @notice sets the feenominator to the given value

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

2. 'Invalid' validations performed

The README.md states:

When it comes to input sanitization, we err on the side of having external interfaces validate their input, rather than socializing costs to do checks such as:

• Checking for address(0)
• Checking for input amounts of 0
• Checking to ensure the protocol enum matches the parameters provided
• Any similar input sanitization

These sorts of input verifications are generally considered to be of 'Low' severity. For these to be excluded, the sponsor must find that the socialization of these costs is at least of the same severity as these sort of issues. Therefore, if any of the mentioned checks are being done, those checks are valid low-severity issues. These are examples where the exact checks mentioned are being done

There are 6 instances of this issue:

File: redeemer/Redeemer.sol

166          // Make sure we have the correct principal
167          if (
168              p != uint8(MarketPlace.Principals.Swivel) &&
169              p != uint8(MarketPlace.Principals.Element) &&
170              p != uint8(MarketPlace.Principals.Yield) &&
171              p != uint8(MarketPlace.Principals.Notional)
172          ) {
173              revert Invalid('principal');
174:         }

212          // Check the principal is Pendle
213          if (p != uint8(MarketPlace.Principals.Pendle)) {
214              revert Invalid('principal');
215:         }

144              } else {
145                  revert Invalid('principal');
146:             }

247          // Check the principal is Sense
248          if (p != uint8(MarketPlace.Principals.Sense)) {
249              revert Invalid('principal');
250:         }

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

File: lender/Lender.sol

109          if (len != a.length) {
110              revert NotEqual('array length');
111:         }

199          // check the principal is illuminate or yield
200          if (p != uint8(MarketPlace.Principals.Illuminate) && p != uint8(MarketPlace.Principals.Yield)) {
201              revert Invalid('principal');
202:         }

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

Non-critical Issues

1. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 14 instances of this issue:

File: lender/Lender.sol

167       function mint(
168           uint8 p,
169           address u,
170           uint256 m,
171           uint256 a
172:      ) public returns (bool) {

192       function lend(
193           uint8 p,
194           address u,
195           uint256 m,
196           uint256 a,
197           address y
198:      ) public unpaused(p) returns (uint256) {

247       function lend(
248           uint8 p,
249           address u,
250           uint256 m,
251           uint256[] memory a,
252           address y,
253           Swivel.Order[] calldata o,
254           Swivel.Components[] calldata s
255:      ) public unpaused(p) returns (uint256) {

317       function lend(
318           uint8 p,
319           address u,
320           uint256 m,
321           uint256 a,
322           uint256 r,
323           uint256 d,
324           address e,
325           bytes32 i
326:      ) public unpaused(p) returns (uint256) {

377       function lend(
378           uint8 p,
379           address u,
380           uint256 m,
381           uint256 a,
382           uint256 r,
383           uint256 d
384:      ) public unpaused(p) returns (uint256) {

433       function lend(
434           uint8 p,
435           address u,
436           uint256 m,
437           uint256 a,
438           uint256 r,
439           uint256 d,
440           address t,
441           address x
442:      ) public unpaused(p) returns (uint256) {

486       function lend(
487           uint8 p,
488           address u,
489           uint256 m,
490           uint128 a,
491           uint256 r,
492           address x,
493           address s
494:      ) public unpaused(p) returns (uint256) {

545       function lend(
546           uint8 p,
547           address u,
548           uint256 m,
549           uint256 a,
550           uint256 r,
551           address pool,
552           address aave,
553           uint256 i
554:      ) public unpaused(p) returns (uint256) {

597       function lend(
598           uint8 p,
599           address u,
600           uint256 m,
601           uint256 a
602:      ) public unpaused(p) returns (uint256) {

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

File: redeemer/Redeemer.sol

107       function redeem(
108           uint8 p,
109           address u,
110           uint256 m,
111           address o
112:      ) public returns (bool) {

158       function redeem(
159           uint8 p,
160           address u,
161           uint256 m
162:      ) public returns (bool) {

206       function redeem(
207           uint8 p,
208           address u,
209           uint256 m,
210           bytes32 i
211:      ) public returns (bool) {

240       function redeem(
241           uint8 p,
242           address u,
243           uint256 m,
244           address d,
245           address o
246:      ) public returns (bool) {

275       function authRedeem(
276           address u,
277           uint256 m,
278           address f,
279           address t,
280           uint256 a
281:      ) public authorized(IMarketPlace(marketPlace).markets(u, m, 0)) returns (bool) {

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

2. 2**<n> - 1 should be re-written as type(uint<n>).max

Earlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas)

There are 2 instances of this issue:

File: lender/Lender.sol   #1

84:           uint256 max = 2**256 - 1;

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

File: lender/Lender.sol   #2

112:          uint256 max = 2**256 - 1;

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

3. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 11 instances of this issue:

File: lender/Lender.sol

/// @audit 1000
70:           feenominator = 1000;

/// @audit 256
84:           uint256 max = 2**256 - 1;

/// @audit 9
87:           for (uint8 i; i < 9; ) {

/// @audit 256
112:          uint256 max = 2**256 - 1;

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

File: marketplace/MarketPlace.sol

/// @audit 8
70:           address[8] memory t,

/// @audit 9
83:           address[9] memory market = [iToken, t[0], t[1], t[2], t[3], t[4], t[5], t[6], t[7]];

/// @audit 3
83:           address[9] memory market = [iToken, t[0], t[1], t[2], t[3], t[4], t[5], t[6], t[7]];

/// @audit 4
83:           address[9] memory market = [iToken, t[0], t[1], t[2], t[3], t[4], t[5], t[6], t[7]];

/// @audit 5
83:           address[9] memory market = [iToken, t[0], t[1], t[2], t[3], t[4], t[5], t[6], t[7]];

/// @audit 6
83:           address[9] memory market = [iToken, t[0], t[1], t[2], t[3], t[4], t[5], t[6], t[7]];

/// @audit 7
83:           address[9] memory market = [iToken, t[0], t[1], t[2], t[3], t[4], t[5], t[6], t[7]];

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L70

4. Missing event and or timelock for critical parameter change

Events help non-contract tools to track changes, and events prevent users from being surprised by changes

There are 9 instances of this issue:

File: 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:      }

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

File: marketplace/MarketPlace.sol

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

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L109-L112

File: 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:       }

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

5. Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.

There are 2 instances of this issue:

File: redeemer/Redeemer.sol   #1

/// @audit seen in lender/Lender.sol 
29:       address public immutable pendleAddr;

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

File: redeemer/Redeemer.sol   #2

/// @audit seen in lender/Lender.sol 
31:       address public immutable tempusAddr;

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

6. Inconsistent spacing in comments

Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file

There are 3 instances of this issue:

File: lender/Lender.sol   #1

173:          //use market interface to fetch the market for the given market pair

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

File: lender/Lender.sol   #2

175:          //use safe transfer lib and ERC interface...

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

File: lender/Lender.sol   #3

177:          //use ERC5095 interface...

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

7. Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

There are 3 instances of this issue:

File: lender/Lender.sol   #1

161:      /// @notice mint swaps the sender's principal tokens for illuminate's ERC5095 tokens in effect, this opens a new fixed rate position for the sender on illuminate

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

File: lender/Lender.sol   #2

477:      /// @dev sense provides a [divider] contract that splits [target] assets (underlying) into PTs and YTs. Each [target] asset has a [series] of contracts, each identifiable by their [maturity].

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

File: marketplace/MarketPlace.sol   #3

218:      function mintWithUnderlying(address u, uint256 m, uint256 a, uint256 ptBought, uint256 minRatio, uint256 maxRatio) external returns (uint256, uint256, uint256) {

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L218

8. Typos

There are 10 instances of this issue:

File: lender/Lender.sol

/// @audit withdrawl
22:       /// @notice minimum amount of time the admin must wait before executing a withdrawl

/// @audit unsighed
83:           // max is the maximum integer value for a 256 unsighed integer

/// @audit gauruntee
500:          if (token.underlying() != u) { // gauruntee the input token is the right token

/// @audit gauruntee
504:          } else if (ISense(x).maturity() > m) { // gauruntee the input amm has the correct maturity

/// @audit Dont
560:          // Dont necessarily need to validate APWINE maturity (They have 1 maturity per underlying)

/// @audit remaing
650:          // send the remaing amount to the given yield pool

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

File: marketplace/MarketPlace.sol

/// @audit intializes
50:       /// @notice intializes the MarketPlace contract

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L50

File: redeemer/Redeemer.sol

/// @audit prinicipal
125:              // Burn the prinicipal token from Illuminate

/// @audit prinicipal
189:              // Redeems prinicipal tokens from yield

/// @audit prinicpal
237:      /// @param d Sense contract that splits the loan's prinicpal and yield

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

9. NatSpec is incomplete

There is 1 instance of this issue:

File: lender/Lender.sol   #1

/// @audit Missing: '@param aave'
536       /// @notice this method can be called before maturity to lend to APWine while minting Illuminate tokens
537       /// @param p value of a specific principal according to the Illuminate Principals Enum
538       /// @param u address of an underlying asset
539       /// @param m maturity (timestamp) of the market
540       /// @param a the amount of underlying tokens to lend
541       /// @param r the minimum amount of zero-coupon tokens to return accounting for slippage
542       /// @param pool the address of a given APWine pool
543       /// @param i the id of the pool
544       /// @return uint256 the amount of principal tokens lent out
545       function lend(
546           uint8 p,
547           address u,
548           uint256 m,
549           uint256 a,
550           uint256 r,
551           address pool,
552           address aave,
553           uint256 i
554:      ) public unpaused(p) returns (uint256) {

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

10. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There are 4 instances of this issue:

File: lender/Lender.sol   #1

48:       event Lend(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 returned);

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

File: lender/Lender.sol   #2

50:       event Mint(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount);

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

File: lender/Lender.sol   #3

52:       event ScheduleWithdrawal(address indexed token, uint256 hold);

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

File: redeemer/Redeemer.sol   #4

36:       event Redeem(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount);

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

Summary

Gas Optimizations

Total: 54 instances over 12 issues

Gas Optimizations

1. Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 2 instances of this issue:

File: lender/Lender.sol   #1

43        mapping(address => uint256) public fees;
44        /// @notice maps a token address to a point in time, a hold, after which a withdrawal can be made
45:       mapping (address => uint256) public withdrawals;

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

File: marketplace/MarketPlace.sol   #2

35        mapping(address => mapping(uint256 => address[9])) public markets;
36    
37        /// @notice pools map markets to their respective YieldSpace pools for the MetaPrincipal token
38:       mapping(address => mapping(uint256 => address)) public pools;

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L35-L38

2. State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There is 1 instance of this issue:

File: redeemer/Redeemer.sol   #1

33:       address public apwineAddr;

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

3. Using calldata instead of memory for read-only arguments in external functions saves gas

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 memory index. 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.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

There is 1 instance of this issue:

File: marketplace/MarketPlace.sol   #1

70:           address[8] memory t,

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L70

4. Use EIP-1167 minimal proxies for 10x cheaper contract instantiation

When new contracts have to be instantiated frequently, it's much cheaper for it to be done via minimal proxies. The only downside is that they rely on delegatecall() calls for every function, which adds an overhead of ~800 gas, but this is multiple orders of manitude less than the amount saved during deployment

There is 1 instance of this issue:

File: marketplace/MarketPlace.sol   #1

80:          address iToken = address(new ERC5095(u, m, redeemer, lender, n, s, d));

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L80

5. State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 9 instances of this issue:

File: lender/Lender.sol

/// @audit marketPlace on line 146
147:              revert Exists(marketPlace);

/// @audit feenominator on line 681
681:          return feenominator > 0 ? a / feenominator : 0;

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

File: redeemer/Redeemer.sol

/// @audit marketPlace on line 164
187:              IElementToken(principal).withdrawPrincipal(amount, marketPlace);

/// @audit marketPlace on line 283
281:      ) public authorized(IMarketPlace(marketPlace).markets(u, m, 0)) returns (bool) {

/// @audit lender on line 128
134:              uint256 amount = IERC20(principal).balanceOf(lender);

/// @audit lender on line 134
136:              Safe.transferFrom(IERC20(u), lender, address(this), amount);

/// @audit lender on line 177
180:          Safe.transferFrom(IERC20(principal), lender, address(this), amount);

/// @audit lender on line 221
224:          Safe.transferFrom(token, lender, address(this), amount);

/// @audit lender on line 256
259:          Safe.transferFrom(token, lender, address(this), amount);

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

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

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There is 1 instance of this issue:

File: lender/Lender.sol   #1

265:              for (uint256 i = 0; i < o.length; ) {

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

7. Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

There is 1 instance of this issue:

File: lender/Lender.sol   #1

30:       mapping(uint8 => bool) public paused;

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

8. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There is 1 instance of this issue:

File: lender/Lender.sol   #1

265:              for (uint256 i = 0; i < o.length; ) {

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

9. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

There are 3 instances of this issue:

File: lender/Lender.sol   #1

96:                   i++;

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

File: lender/Lender.sol   #2

120:                  i++;

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

File: lender/Lender.sol   #3

289:                      i++;

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

10. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

There are 31 instances of this issue:

File: lender/Lender.sol

48:       event Lend(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 returned);

50:       event Mint(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount);

87:           for (uint8 i; i < 9; ) {

168:          uint8 p,

193:          uint8 p,

248:          uint8 p,

318:          uint8 p,

378:          uint8 p,

434:          uint8 p,

487:          uint8 p,

490:          uint128 a,

546:          uint8 p,

598:          uint8 p,

648:          uint128 returned = IYield(y).sellBasePreview(Cast.u128(a));

734:      function pause(uint8 p, bool b) external authorized(admin) returns (bool) {

750:      modifier unpaused(uint8 p) {

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

File: marketplace/MarketPlace.sol

73:           uint8 d

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

139:          uint128 a

140:      ) external returns (uint128) {

154:          uint128 a

155:      ) external returns (uint128) {

169:          uint128 a

170:      ) external returns (uint128) {

184:          uint128 a

185:      ) external returns (uint128) {

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L73

File: redeemer/Redeemer.sol

36:       event Redeem(uint8 principal, address indexed underlying, uint256 indexed maturity, uint256 amount);

108:          uint8 p,

159:          uint8 p,

207:          uint8 p,

241:          uint8 p,

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

11. Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There is 1 instance of this issue:

File: lender/Lender.sol   #1

23:       uint256 constant public HOLD = 3 days;

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

12. Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 2 instances of this issue:

File: lender/Lender.sol   #1

710:          require (when != 0, 'no withdrawal scheduled');

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

File: lender/Lender.sol   #2

712:          require (block.timestamp >= when, 'withdrawal still on hold');

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