Illuminate contest - 0xkowloon's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L297

Vulnerability details

Impact

Fees would not have been charged to msg.sender, while the fee can still be withdrawn through withdrawFee, causing missing funds.

Proof of Concept

The function lend only transfers lent amount of underlying tokens from msg.sender to the Lender contract, which lent is the sum of amountLent and each amountLent is equal to amount - fee. Meanwhile, fees[u] is increased by totalFee, which is the sum of fee calculated by calculateFee(amount). The contract records an intake of lent + totalFee, but has only charged lent, causing an accounting error and a loss of funds.

Recommended Mitigation Steps

The function should instead transfer lent + totalFee.

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

Summary: typo Location: https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/ERC5095.sol#L129 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/ERC5095.sol#L58 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/MarketPlace.sol#L11 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L22 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/redeemer/Redeemer.sol#L125 Issue: recieving/redeemption/avaialable/withdrawl/prinicipal

Summary: Redeem event was never used Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/marketplace/Interfaces.sol#L142 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Interfaces.sol#L146 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Interfaces.sol#L146 Issue: IERC5095’s Redeem event is not used and it should be removed. ERC5095 has the function redeem that calls Redeemer’s function authRedeem, which has its own Redeem event.

Summary: (u,m); appearing twice in the same line Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L627 Issue: To retrieve illuminateToken, only the method call principalToken(u, m); is required. The extra (u, m); looks like a careless error that should be removed. It doesn’t do anything.

Summary: setAdmin should be a 2-step function to prevent setting the wrong address. Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L129 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L62 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/MarketPlace.sol#L109 Issue: setAdmin is recommended to follow a 2-step approach: 1) The current privileged role proposes a new address for the change 2) The newly proposed address then claims the privileged role in a separate transaction. This prevents setting the wrong admin address and thus losing admin control of the contracts forever.

See Uniswap’s governance contract for some examples. https://github.com/Uniswap/governance/blob/master/contracts/Timelock.sol#L53 https://github.com/Uniswap/governance/blob/master/contracts/Timelock.sol#L45

Also, the current setAdmin function is missing an event log. For functions that change critical contract parameters, it is important to emit an event so that system monitors can pick up the change and adjust their engagement with the system accordingly.

Summary: onlyAdmin is not indented properly Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/marketplace/ERC5095.sol#L139 Issue: The function is not indented properly.

Summary: mint/mintWithUnderlying Natspec are wrong Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/marketplace/MarketPlace.sol#L200 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/marketplace/MarketPlace.sol#L217 Issue: Both functions return (uint256, uint256, uint256), while the Natspec declare only the return of a single uint256. It is missing documentation for the rest of the uint256.

Summary: All “setX” functions are missing event logs Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/marketplace/MarketPlace.sol#L98 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/marketplace/MarketPlace.sol#L119 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L137 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L145 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L156 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/redeemer/Redeemer.sol#L70 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/redeemer/Redeemer.sol#L81 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/redeemer/Redeemer.sol#L92 Issue: Similar to the issue mentioned earlier on setAdmin, for functions that change critical contract parameters, it is important to emit an event so that system monitors can pick up the change and adjust their engagement with the system accordingly.

Summary: Redeemer.redeem should check o’s token balance instead of msg.sender’s. Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/redeemer/Redeemer.sol#L120 Issue: Since the function calls token.burn(o, amount); to burn principal tokens from o, it does not make sense to get the amount from token.balanceOf(msg.sender). It should instead get it from token.balanceOf(“o”).

Summary: redeem function should not prematurely get msg.sender token balance Location: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/redeemer/Redeemer.sol#L122 Issue: The function gets msg.sender’s balance before checking the token’s maturity. It would have been gas wasted if the token has not reached maturity as the “amount” variable is not used. Move uint256 amount = token.balanceOf(msg.sender) to after the maturity check.