Illuminate contest - pashov's results

Lines of code

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

Vulnerability details

Proof of concept

Redeem method in Redeemer.sol has the following code IElementToken(principal).withdrawPrincipal(amount, marketPlace); that will send the redeemed principal tokens to marketPlace and not to the Redeemer smart contract (address(this)), as other MarketPlace.Principals do (Yield, Notional etc). In the MarketPlace smart contract the user can’t withdraw them but any user can then call sellPrincipalTokens in MarketPlace.sol which results in the ‘redeem’ method caller user’s redeemed principals being sold without their consent

Impact

This can lead to an user’s redeemed principal tokens getting stuck in the marketPlace contract or being used in the wrong way without their consent.

Recommendation

Change IElementToken(principal).withdrawPrincipal(amount, marketPlace); to IElementToken(principal).withdrawPrincipal(amount, address(this));

Lines of code

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

Vulnerability details

Proof of concept

// Transfer the original underlying token back to the user
Safe.transferFrom(IERC20(u), lender, address(this), amount);

The code should transfer the underlying token to the user (as per the NatSpec Consequently, only Illuminate's redeem returns funds to the user and the comment in the code above), but the recipient is address(this) which is the Redeemer contract.

Impact

Users are not able to get their Illuminate underlying tokens which results in a loss of funds for them.

Recommendation

In the code referenced change address(this) to msg.sender

Lines of code

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

Vulnerability details

Proof of concept

The redeeem method for Illuminate tokens gets the balance of Illuminate principle tokens from the user uint256 amount = token.balanceOf(msg.sender); but then burns this amount token.burn(o, amount); from the “o” param which is referred to in the NatSpec as “address of the controller or contract that manages the underlying token”. A user who holds any amount of Illuminate principal tokens can call the redeem method over and over again untill all of “o”’s tokens are burned but the user will still hold his Illuminate principal tokens. So now if another user tries to redeem his Illuminate underlying tokens there won’t be enough tokens to burn in “o” and the method will revert, leaving the underlying tokens stuck.

Impact

This can lead to other users not being able to redeem their Illuminate underlying token, which leads to loss of funds for them.

Recommendation

Burn the principal tokens from the msg.sender instead of from o. Or if by design they have to be burned by o still first transfer the principal tokens from the msg.sender to o, so msg.sender won’t have more Illuminate principal tokens after transaction.

Lines of code

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

Vulnerability details

Proof of concept

Anyone can call buyPrincipalToken, buyUnderlying, sellPrincipalToken and sellUnderlying since they do not have access control and are external. Those methods send tokens that MarketPlace.sol holds to some pool contract and then trades them for another token that the msg.sender receives. Basically anytime the MarketPlace contract holds funds anyone can call those methods and steal the received tokens. Even if a user intentionally makes it so that the MarketPlace contract holds his tokens and he wants to use it to trade them - his transaction can be front-run and he will lose his funds.

Impact

This can lead to user & protocol funds being stolen.

Recommendation

Implement proper access control for buyPrincipalToken, buyUnderlying, sellPrincipalToken and sellUnderlying .

Lines of code

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

Vulnerability details

Proof of concept

The approve method in Lender.sol has a parameter called r described in the NatSpec as @param r the address being approved, in this case the redeemer contract so it is assumed that this should be the address of the redeemer contract. The admin can call this method and pass his address as the value of r which will lead to him being approved to spend all of the principal tokens that the Lender contract holds.

Impact

A rug can be executed by the admin (owner) of the Lender.sol smart contract, which can happen if admin private key/multisig is compromised.

Recommendation

Do not get the redeemer contract address as a function argument, create a storage variable that is set on contract deployment in constructor that will hold the Redeemer contract address.

Lines of code

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

Vulnerability details

Proof of concept

Lend method for Illuminate and Yield in Lender.sol is missing the accumulated fee tracking code fees[u] += totalFee; that is in all other lend methods in the contract.

Impact

All fees for Illuminate and Yield lending method calls will be stuck in the contract without an option to withdraw, which is a loss of funds earned for the protocol.

Recommendation

For both Yield and Illuminate extract a totalFee variable representing the fee and ad it to the fees mapping in the same fashion as other methods fees[u] += totalFee;

Problem: the whole codebase returns bool but there is not even one place where the code says return false.

Impact: This leads to confusion from a user/integrator standpoint and wastes gas.

Solution: Remove the bool return statements everywhere in code unless it is needed to comply to a standart.

Problem: code uses floating Solidity versions like ***pragma*** solidity ^0.8.0;

Impact: code can be compiled with an unexpected Solidity compiler version

Solution: use concrete versions like ***pragma*** solidity 0.8.13;

Problem: code uses inconsistent Solidity versions, some files use 0.8.0, others 0.8.4, others 0.8.13 etc.

Impact: This can lead to unexpected bugs from some compiler versions

Solution: Use the same Solidity version in every non-external Solidity file

Problem: missing non-zero address checks for admin, setAdmin in Lender.sol MarketPlace.sol and Redeemer.sol.

Impact: If mistakenly the admin is set to zero then the protocol will lose important mechanisms and won’t be usable anymore

Solution: Consider using OpenZeppelin’s Ownable implementation

Gas optimisations

For loop gas optimisation

Proof of concept

While two of the three for loops in Lender.sol are implemented in almost the most gas efficient way there is still some more improvement to add. For example using ++i instead of i++ can save a good chunk of gas, especially if the loop is long running

Impact

Gas savings for protocol users.

Recommendations

1. Don't initialise index counter variable to zero - zero is its default value and reinitialising to zero costs extra gas
   https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L265
2. When the for loop end check expression is checking for some array's length, always cache the length of the array in a separate stack variable.
   https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L265
3. Use ++i instead of i++ in for loops (small gas saving)
   https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L96
   https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L120
   https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L289

The third loop in Lender.sol can be written this way

  255    uint256 oLength = o.length;
  256    for (uint256 i; i < oLength; ) {
             ...
  288        unchecked {
  289            ++i;
  290        }
  291    }

4. Use uint256 instead of uint8 for gas saving as the EVM's word size is bytes32 and it makes additional operation to cast to smaller types.

-  87    for (uint8 i; i < 9; )
+  87    for (uint256 i; i < 9; )

Hardcode uint256 max value calculation using type(uint256).max

Proof of concept

Storing uint256 max value as a variable in functions is unnecessary. It can be avoided by simply hardcoding it where needed.

Impact

It will reduce gas cost for the user and remove a redundant variable.

Recommendations

Hardcode uint256 max value instead of storing it in a separate variable. Use type(uint256).max as it is the cheapest way to calculate it.

  107    function approve(address[] calldata u, address[] calldata a) external authorized(admin) returns (bool) {
  108            uint256 len = u.length;
  109            if (len != a.length) {
  110                revert NotEqual('array length');
  111            }
- 112            uint256 max = 2**256 - 1;
  112
  113            for (uint256 i; i < len; ) {
  114                IERC20 uToken = IERC20(u[i]);
  115                if (address(0) != (address(uToken))) {
- 116                    Safe.approve(uToken, a[i], max);
+ 116                    Safe.approve(uToken, a[i], type(uint256).max);
  117                }
  118                unchecked {
  119                    i++;
  120                }
  121            }
  122            return true;
  123    }

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

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

Remove all require with string error expressions and use custom errors instead

Proof of concept

Using custom errors (introduced in Solidity 0.8) is more gas efficient than having require expressions with error strings.

Impact

This can be helpful for external integrations’ error handling and can save some gas for the end-user if the transaction reverts.

Recommendations

Replace require expressions in Lender.sol with custom errors

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

Example with custom errors:

+  21    error WithdrawalNotScheduled();
+  22    error WithdrawalOnHold();

  708    function withdraw(address e) external authorized(admin) returns (bool) {
  709            uint256 when = withdrawals[e];
  710            if(when == 0) revert WithdrawalNotScheduled();
  711
  712            if(block.timestamp < when) revert WithdrawalOnHold();
  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    }

Remove custom errors with string parameters and use specific custom errors instead

Proof of concept

The advantage of using custom errors is that they do not need strings to describe their purpose. Strings are gas inefficient so it is cheaper to use separate well-naimed custom errors instead of reusing one by passing it a string.

Impact

Using specific custom errors will decrease the gas cost when fuction executions fail.

Recomendation

Avoid using strings as paramenters in custom errors. Instead use separate custom errors for the different cases.

Lender.sol

-  18    error NotEqual(string);

+  18    error NotEqual_Underlying();
+  19    error NotEqual_Maturity();

- 208    if (address(pool.base()) != u) {
- 209        revert NotEqual('underlying');
- 210    } else if (pool.maturity() > m) {
- 211        revert NotEqual('maturity');
- 212    }

+ 208    if (address(pool.base()) != u) {
+ 209        revert NotEqual_Underlying();
+ 210    } else if (pool.maturity() > m) {
+ 211        revert NotEqual_Maturity();
+ 212    }

The code from the example above can be applied on the following lines in Lender.sol

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L209-L211
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L269-L271
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L332-L334
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L392-L394
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L447-L449
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L501-L505
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L558-L560

Other places where string parameters can be avoided in custom errors

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L20
https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L29
https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/MarketPlace.sol#L31
https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L14
https://github.com/code-423n4/2022-06-illuminate/blob/main/redeemer/Redeemer.sol#L16

Use immutable keyword

Proof of concept

Notice the missing immutable keyword in Reedemer.

Impact

Less gas is being payed each time referencing an immutable variable than a storage one.

Recommendations

Add the missing immutable keyword in front of apwineAddr as it is not being changed any time during the smart contract life cycle.

-  33    address public apwineAddr;
+  33    address public immutable apwineAddr;

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

X = X + Y is cheaper than X += Y

Proof of concept

Although doing the same thing the second syntax appears to cost less gas units.

Impact

Less gas being payed when increasing numbers.

Recommendations

Use x = x + y instead of x += y

Examples in function lend in Lender.sol:

- 279    totalFee += fee;
+ 279    totalFee = totalFee + fee;

- 283    lent += amountLent;
+ 283    lent = lent + amountLent;

- 285    returned += amountLent * order.premium / order.principal;
+ 285    returned = returned + amountLent * order.premium / order.principal;

- 294    fees[u] += totalFee;
+ 294    fees[u] = fees[u] + totalFee;

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L279
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L283
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L285
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L294
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L340
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L404
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L461
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L514
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L572
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L621

uint256 instead of uint8

Proof of concept

The EVM works with 256bit/32byte words. Every operation is based on these base units. If the data used is smaller, further operation are needed to downscale from 256 bits to 8 bits.

Impact

Remove the redundant gas costs of the low-level EVM convertions of uint256 units to uint8.

Recommendations

Replace each uint8 with uint256 (except the ones in structs).

Example:

All the lend functions use uint8

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

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L192 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L247 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L317 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L377 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L433 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L486 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L545 https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L597

Change visibility modifier of public functions to external

Proof of concept

External function are cheaper than public ones. Functions that are not being invoked from inside the contract have no reason to be declared as public and not external.

Impact

Save gas on each currently public function call where the function is not being used in the contract in its life cycle.

Recommendations

Change public functions visibility modifiers to external where the function have to be so.

Examples in Lender.sol:

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L172
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L198
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L255
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L326
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L384
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L442
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L494
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L554
https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#L602