Platform: Code4rena
Start Date: 09/12/2021
Pot Size: $50,000 USDC
Total HM: 19
Participants: 21
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 61
League: ETH
Rank: 4/21
Findings: 3
Award: $3,337.45
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: 0x0x0x
2816.7538 USDC - $2,816.75
0x0x0x
Current implementation to get the price is as follows:
(uint256 _ratioOfPrices, uint256 _decimals) = IPriceOracle(priceOracle).getLatestPrice(_borrowAsset, _collateralAsset);
But it should not consult borrowToken / collateralToken, rather it should consult the inverse of this result. As a consequence, in liquidate the liquidator/lender can lose/gain funds as a result of this miscalculation.
Replace it with
(uint256 _ratioOfPrices, uint256 _decimals) = IPriceOracle(priceOracle).getLatestPrice(_collateralAsset, _borrowAsset);
0x0x0x
When _borrowAsset == address(0), the liquidator sends ethereum via transaction value (msg.value), but _borrowTokens amount is not sent to the lender. Therefore, lender losses funds.
As seen in: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/CreditLine/CreditLine.sol#L1013-L1020
Add (bool success, ) = _lender.call{value: _borrowTokens}(''); at the end of the cited code block, so that lender receives the funds. So cited code block be replaced with the following:
uint256 _borrowTokens = _borrowTokensToLiquidate(_borrowAsset, _collateralAsset, _totalCollateralTokens); if (_borrowAsset == address(0)) { uint256 _returnETH = msg.value.sub(_borrowTokens, 'Insufficient ETH to liquidate'); if (_returnETH != 0) { (bool success, ) = msg.sender.call{value: _returnETH}(''); require(success, 'Transfer fail'); } (bool success, ) = _lender.call{value: _borrowTokens}('');
#0 - ritik99
2021-12-24T15:22:59Z
Duplicate of #90
10.9563 USDC - $10.96
0x0x0x
Solidity 0.7, does not directly receive security support from Solidity team.
As you can see in the documentation: https://docs.soliditylang.org/en/v0.8.10/
When deploying contracts, you should use the latest released version of Solidity. Apart from exceptional cases, only the latest version receives [security fixes](https://github.com/ethereum/solidity/security/policy#supported-versions). Furthermore, breaking changes as well as new features are introduced regularly.
Furthermore, with solidity 0.8, SafeMath won't be required anymore and tsafe math functionality can be used natively and more gas efficient. There are also a lot of interesting futures planned for 0.9 (better compiler etc.).
I strongly recommend using a newer solidity version for better security/gas efficiency and keeping the code base up to date for future language improvements.
#0 - ritik99
2021-12-26T17:13:38Z
Duplicate of #39
🌟 Selected for report: sirhashalot
Also found by: 0x0x0x, WatchPug, cmichel, pmerkleplant, robee
0x0x0x
When a variable is declared solidity assigns the default value. In case the contract assigns the value again, it costs extra gas.
Example:uint x = 0 costs more gas than uint x without having any different functionality.
./CreditLine/CreditLine.sol:812: uint256 _principalPaid = 0; ./Pool/Pool.sol:358: uint256 _collateralShares = 0; ./mocks/yVault/yVault.sol:281: uint256 shares = 0; ./mocks/yVault/yVault.sol:297: uint256 shares = 0;
#0 - ritik99
2022-01-07T22:50:27Z
Duplicate of #36 since both issues deal with unnecessary initializations. Hence, the issue is the same even if the instances mentioned in both places are different. Two of the instances in mocks were not part of the audit scope as mentioned in the contest readme
🌟 Selected for report: 0x0x0x
83.4956 USDC - $83.50
0x0x0x
In Verfication.sol#unlinkAddress, there is a not needed zero address check.
require(_linkedTo != address(0), 'V:UA-Address not linked'); require(_linkedTo == msg.sender, 'V:UA-Not linked to sender');
Since, msg.sender != address(0), there is no need for a zero address check here.
22.5438 USDC - $22.54
0x0x0x
uint256 _id = creditLineCounter + 1; creditLineCounter = _id;
can be replaced with
uint256 _id = ++creditLineCounter;
to save gas.
#0 - ritik99
2021-12-25T18:43:21Z
Duplicate of #22
🌟 Selected for report: 0x0x0x
Also found by: WatchPug, pmerkleplant, robee
15.2171 USDC - $15.22
0x0x0x
Example:
for (uint i = 0; i < arr.length; i++) { //Operations not effecting the length of the array. }
Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Furthermore, there is no need to assign the initial value 0. This costs extra gas.
Recommended implementation:
uint length = arr.length; for (uint i; i < length; ++i) { //Operations not effecting the length of the array. }
By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).
./CreditLine/CreditLine.sol:484: for (uint256 _index = 0; _index < _strategyList.length; _index++) { ./CreditLine/CreditLine.sol:662: for (uint256 _index = 0; _index < _strategyList.length; _index++) { ./CreditLine/CreditLine.sol:738: for (uint256 _index = 0; _index < _strategyList.length; _index++) { ./CreditLine/CreditLine.sol:892: for (uint256 index = 0; index < _strategyList.length; index++) { ./CreditLine/CreditLine.sol:959: for (uint256 index = 0; index < _strategyList.length; index++) { ./SavingsAccount/SavingsAccount.sol:289: for (uint256 i = 0; i < _strategyList.length; i++) { ./SavingsAccount/SavingsAccount.sol:467: for (uint256 i = 0; i < _strategyList.length; i++) {