Illuminate contest - simon135's results

Lines of code

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

Vulnerability details

Impact

reentracy in lender.sol in lend() function an attacker can control the aave token and implent his own function doing what ever he wants if an attacker can make his own contract with his own deposit() function he can do what ever he wants like minting himself the tokens so he dosnt loose out on minting and reetner again and again also he can do the same thing with the pool.swapExactAmountIn function

proof of concept

 IAave(aave).deposit(u, lent, address(this), 0);
  uint256 returned = IAPWineRouter(pool).swapExactAmountIn(i, 1, lent, 0, r, address(this));

you can make your own pool contract with your own implenation and do what ever you want 1.attacker calls lend() with his pool address contract and aave contract 2. u= someone else address or himself and transfers himslef the funds or attacherk uses deposit to call the aave deposit but a big amount 3. same with the pool all the attacker has to do is make safe.transfer success with 0 but in the deposit function he makes a great amount and causing users lost of funds or give him self funds

Recommended Mitigation Steps

Add a require check for what pool address is and aave address is and add noreentracy or lock modifer to the functions

no events/emits after improtent variable update functions

Lender.sol:131,137,149,157 Redeemer.sol:62,74,85,93

make sure mint() is in a require statement so it does fail

Lender.sol:L#17

remove dead code its best practice

https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/MarketPlace.sol#L16

typos

instead of : avaialable use : available https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/MarketPlace.sol#L11 instead of: prinicpal use: principal https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/redeemer/Redeemer.sol#L237 https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/redeemer/Redeemer.sol#L249 instead of : withdrawl use : withdrawal https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L22 instead of : unsighed use : unsigned https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L83 instead of : gauruntee use : guarantee https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L504

unused imports makes your code cleaner

You can take out Interfaces.sol from Marketplace.sol becaue Safe.sol imports it https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/MarketPlace.sol#L5 same thing with Lender.sol https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L5

1. use uint256 instead of uint8 because in evm uses more gas for uint8

becuase each slot in evm is uint256 so the evm has to do compuation to fill up 256 bits but for 8bit number Lender.solL30

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled. i++ increments i and returns the initial value of i. Which means: uint i = 1; i++; // == 1 but i == 2 But ++i returns the actual incremented value: uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 instances include: Lender.sol:96,289

in this case you dont have to cache an calldata array length

in the if statment you dont have to read len just read the calldata array length to save gas lender.sol:108

make variables uninitlized to save gas

because variables uninitlized in evm is still 0 and saves 3 gas lender.sol:265

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source Custom Errors in Solidity: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). instances include: https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L710 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L712

you can remove token variable , just use IERC20(e) instead to save gas

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

make onlyadmin function payable to save gas

no check for msg.value equals 0

Lender.sol:131,137,149,157 Redeemer.sol:62,74,85,93

unused imports it saves gas

You can take out Interfaces.sol from Marketplace.sol becaue Safe.sol imports it https://github.com/code-423n4/2022-06-illuminate/blob/217ddfef05bc9df5c7b94f1c3226a46ee136b57d/marketplace/MarketPlace.sol#L5 same thing with Lender.sol https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L5