Platform: Code4rena
Start Date: 23/06/2023
Pot Size: $60,500 USDC
Total HM: 31
Participants: 132
Period: 10 days
Judge: 0xean
Total Solo HM: 10
Id: 254
League: ETH
Rank: 14/132
Findings: 3
Award: $777.04
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0xnev
Also found by: 0xRobocop, 0xbrett8571, 0xkazim, 0xnacho, 3agle, 8olidity, ABAIKUNANBAEV, Bauchibred, Co0nan, CrypticShepherd, D_Auditor, DelerRH, HE1M, Iurii3, Kaysoft, MrPotatoMagic, RedOneN, RedTiger, Rolezn, SanketKogekar, Sathish9098, Timenov, Toshii, Vagner, bart1e, bytes032, codetilda, devival, halden, hals, kutugu, m_Rassska, naman1778, nonseodion, seth_lawson, solsaver, squeaky_cactus, totomanov, y51r, yudan, zaevlad
57.9031 USDC - $57.90
| LOW COUNT | ISSUES | INSTANCES |
|---|---|---|
| [L-1] | assetAmount * 2 <= depositedAsset[onBehalfOf] check is conflict with docs | 1 |
| [L-2] | Lack of check-effects-interactions (CEI) pattern is vulnerable to reentrancy attacks | 3 |
| [L-3] | User could deposit a small amount of ether and then mint a large amount of PEUSD, which could create a security risk | 1 |
| [L-4] | Use the safeAllowance() function instead of the normal approve() function | 1 |
| [L-5] | Use call instead of transfer to send ethers | 3 |
| [L-6] | Timelocks not implemented as per documentation | - |
| [L-7] | depositAssetToMint() does not check for a maximum limit on asset deposits | 1 |
| [L-8] | Project Upgrade and Stop Scenario should be | - |
| [L-9] | Lack of nonReentrant modifiers for critical external functions | 1 |
| [L-10] | Divide by zero should be avoided | 2 |
| [L-11] | ````name maskingshould be avoided toprevent errors`` | 2 |
| [L-12] | Division before multiplication can lead to precision errors | 2 |
| [L-13] | Hardcoding addresses in smart contracts can make them more vulnerable to attack | 1 |
| [L-14] | NATSPEC comments should be increased in contracts | - |
| [L-15] | Negative values may return the unexpected values when using uint256(int)conversion | 1 |
| [L-16] | Array does not have a pop function | 1 |
| [L-17] | Setters should have initial value check | 1 |
| [L-18] | External calls in an un-bounded for-loop may result in a DOS | 2 |
assetAmount * 2 <= depositedAsset[onBehalfOf] check is conflict with docs* - collateralAmount should be less than 50% of collateral
The comment for the function states that the collateral amount should be less than 50% of the deposited asset. However, the implementation of the function actually allows for the collateral amount to be equal to 50% of the deposited asset
This is a potential security vulnerability, as it could allow a user to liquidate more collateral than they are entitled to.
FILE: 2023-06-lybra/contracts/lybra/pools/base/LybraEUSDVaultBase.sol * - collateralAmount should be less than 50% of collateral 159:require(assetAmount * 2 <= depositedAsset[onBehalfOf], "a max of 50% collateral can be liquidated");
it would be advisable to change the implementation of the function to require that the collateral amount be less than 50% of the deposited asset, as stated in the comment
check-effects-interactions (CEI) pattern is vulnerable to reentrancy attacksIn the function depositEtherToMint, the state change of minting PEUSD is performed after the external call to the IWBETH contract. This means that an attacker could call the IWBETH contract and then call the _mintPeUSD function before the state change of minting PEUSD has been completed. This could allow the attacker to mint more PEUSD than they are entitled to, which could create a security risk
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/pools/LybraWbETHVault.sol 23: IWBETH(address(collateralAsset)).deposit{value: msg.value}(address(configurator)); 24: uint256 balance = collateralAsset.balanceOf(address(this)); 25: depositedAsset[msg.sender] += balance - preBalance; 26: 27: if (mintAmount > 0) { 28: _mintPeUSD(msg.sender, msg.sender, mintAmount, getAssetPrice()); 29: }
FILE: 2023-06-lybra/contracts/lybra/pools/LybraRETHVault.sol rkPool.deposit{value: msg.value}(); uint256 balance = collateralAsset.balanceOf(address(this)); depositedAsset[msg.sender] += balance - preBalance; if (mintAmount > 0) { _mintPeUSD(msg.sender, msg.sender, mintAmount, getAssetPrice()); }
Use check-effects-interactions (CEI) pattern
ether and then mint a large amount of PEUSD, which could create a security riskif a user deposits 1 ether and mints 100,000 PEUSD, they would have a large amount of PEUSD that they could then use to buy up a large amount of esLBR. This could then give them control of a large portion of the ProtocolRewardsPoolcontract, which could allow them to manipulate the rewards distribution.
FILE: 2023-06-lybra/contracts/lybra/pools/LybraWbETHVault.sol function depositEtherToMint(uint256 mintAmount) external payable override { require(msg.value >= 1 ether, "DNL"); uint256 preBalance = collateralAsset.balanceOf(address(this)); IWBETH(address(collateralAsset)).deposit{value: msg.value}(address(configurator)); uint256 balance = collateralAsset.balanceOf(address(this)); depositedAsset[msg.sender] += balance - preBalance; if (mintAmount > 0) { _mintPeUSD(msg.sender, msg.sender, mintAmount, getAssetPrice()); }
The depositEtherToMint() function could be modified to limit the amount of PEUSD that a user can mint. The function could be modified to only allow a user to mint a certain percentage of the amount of ether that they deposit.
safeAllowance() function instead of the normal approve() functionspender has already transferred the requested amount of tokenssafeAllowance() functionThe safeAllowance() function first checks if the spender has already been approved for the requested amount of tokens. If they have not, the function approves the spender for the requested amount of tokens and then checks if the spender has already transferred the requested amount of tokens. If they have, the function reverts. This helps to prevent malicious spenders from transferring tokens that they have not been approved to transfer.
FILE: 2023-06-lybra/contracts/lybra/pools/LybraWstETHVault.sol 35: lido.approve(address(collateralAsset), msg.value);
Use safeAllowance() for better security
call instead of transfer to send ethers.transfer will relay 2300 gas and .call will relay all the gas. If the receive/fallback function from the recipient proxy contract has complex logic, using .transfer will fail, causing integration issues
FILE: 2023-06-lybra/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol 139: collateralAsset.transfer(msg.sender, reducedAsset); 142: collateralAsset.transfer(provider, reducedAsset - reward2keeper); 143: collateralAsset.transfer(msg.sender, reward2keeper);
Replace .transfer with .call. Note that the result of .call need to be checked
Timelocks not implemented as per documentation Does it use a timelock function?: True
Implement time locks as per documentations
depositAssetToMint() does not check for a maximum limit on asset depositsFILE: 2023-06-lybra/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol function depositAssetToMint(uint256 assetAmount, uint256 mintAmount) external virtual { require(assetAmount >= 1 ether, "Deposit should not be less than 1 collateral asset."); uint256 preBalance = collateralAsset.balanceOf(address(this)); collateralAsset.transferFrom(msg.sender, address(this), assetAmount); require(collateralAsset.balanceOf(address(this)) >= preBalance + assetAmount, ""); depositedAsset[msg.sender] += assetAmount; if (mintAmount > 0) { uint256 assetPrice = getAssetPrice(); _mintPeUSD(msg.sender, msg.sender, mintAmount, assetPrice); } emit DepositAsset(msg.sender, address(collateralAsset), assetAmount, block.timestamp); }
It would be advisable to add a maximum limit on asset deposits to the depositAssetToMint function
At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an ” EMERGENCY STOP (CIRCUIT BREAKER) PATTERN “.
https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol
nonReentrant modifiers for critical external functionsApply the nonReentrant modifier to critical external functions to ensure they are not susceptible to reentrancy attacks
nonReentrant modifier, you can help protect your contract from reentrancy vulnerabilities. It's important to apply this modifier to functions that involve state changes or interact with external contracts, especially if they involve transfers of Ether or tokens
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol 82: function withdraw(address onBehalfOf, uint256 amount) external virtual {
It would be advisable to add the nonReentrant modifier
Divide by zero should be avoidedDivision by zero should be avoided in Solidity and in any programming language. Division by zero is an undefined operation, and it can lead to unexpected and unpredictable results. In Solidity, division by zero will cause the contract to revert. This means that the transaction will be canceled, and the caller will not receive any funds or tokens
FILE: 2023-06-lybra/contracts/lybra/miner/esLBRBoost.sol 67: return ((boostEndTime - userUpdatedAt) * maxBoost) / (time - userUpdatedAt);
FILE: 2023-06-lybra/contracts/lybra/pools/base/LybraEUSDVaultBase.sol 156: uint256 onBehalfOfCollateralRatio = (depositedAsset[onBehalfOf] * assetPrice * 100) / borrowed[onBehalfOf];
name masking should be avoided to prevent errorsName masking can be a source of errors, so it is important to be aware of the potential for name masking when writing Solidity contracts. By giving variables unique names and avoiding name masking, you can help to prevent errors in your contracts.
If a variable is declared with the same name as a function, the variable will shadow the function. This means that the variable will be used instead of the function when the name is referenced.
FILE: 2023-06-lybra/contracts/lybra/token/PeUSD.sol 12: uint8 decimals = decimals();
FILE: 2023-06-lybra/contracts/lybra/token/LBR.sol 20: uint8 decimals = decimals();
Avoid using the same name for a variable and a function
Division before multiplication can lead to precision errorsBecause Solidity integer division may truncate, it is often preferable to do multiplication before division to prevent precision loss.
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol 142: borrowed = borrowed * (1e20 + peUSDExtraRatio) / 1e20;
142: borrowed = (borrowed * (1e20 + peUSDExtraRatio)) / 1e20;
Hardcoding addresses in smart contracts can make them more vulnerable to attackIt makes the contract more difficult to audit. This is because it is more difficult to understand how the contract works if the addresses are hardcoded.
It makes the contract more vulnerable to attack. This is because it is easier for an attacker to find a way to manipulate the values of the variables and get an incorrect result.
It makes the contract more difficult to update. This is because it is necessary to update the hardcoded addresses if the addresses of the contracts change.
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol 153: uint256 etherInLp = (IEUSD(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2).balanceOf(ethlbrLpToken) * uint(etherPrice)) / 1e8;
Use dynamic addresses. Dynamic addresses are addresses that are not hardcoded into the contract. Instead, they are generated at runtime and stored in a state variable. This makes it more difficult for an attacker to manipulate the values of the variables and get an incorrect result
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
Recommendation NatSpec comments should be increased in Contracts
Negative values may return the unexpected values when using uint256(int) conversionWhen converting a negative int256 value to a uint256 using uint256(int256), the result will be unexpected. if you try to convert a negative int256 value to a uint256, the result will be interpreted as the two's complement representation of the negative value. This means that the resulting uint256 value will be a large positive number.
PROOF:
We attempt to convert the negative value -10 to a uint256. However, the resulting value will not be the expected value of -10 but rather a large positive number.
If you execute the function, the returned value will be 115792089237316195423570985008687907853269984665640564039457584007913129639934. This result corresponds to the two's complement representation of -10 when interpreted as an unsigned uint256.
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol 212: (, int lbrPrice, , , ) = lbrPriceFeed.latestRoundData(); 213: biddingFee = biddingFee * uint256(lbrPrice) / 1e8;
Carefully handle conversions between signed and unsigned integer types and ensure that the values are within the valid ranges of the target type.
Array does not have a pop functionArrays without the pop operation in Solidity can lead to inefficient memory management and increase the likelihood of out-of-gas errors.
FILE: 2023-06-lybra/contracts/lybra/miner/esLBRBoost.sol 26: esLBRLockSettings.push(esLBRLockSetting(30 days, 20 * 1e18)); 27: esLBRLockSettings.push(esLBRLockSetting(90 days, 30 * 1e18)); 28: esLBRLockSettings.push(esLBRLockSetting(180 days, 50 * 1e18)); 29: esLBRLockSettings.push(esLBRLockSetting(365 days, 100 * 1e18)); 34: esLBRLockSettings.push(setting);
Add pop function to avoid out-of-gas errors
Setters should have initial value checkSetters should have initial value check to prevent assigning wrong value to the variable. Assginment of wrong value can lead to unexpected behavior of the contract.
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/configuration/LybraConfigurator.sol function setMintVaultMaxSupply(address pool, uint256 maxSupply) external onlyRole(DAO) { mintVaultMaxSupply[pool] = maxSupply; }
maxSupply add initial value check then assign to mintVaultMaxSupply[pool]
Consider limiting the number of iterations in for-loops that make external calls
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol for (uint i = 0; i < _pools.length; i++) { require(configurator.mintVault(_pools[i]), "NOT_VAULT"); //@audit external call } for (uint i = 0; i < pools.length; i++) { ILybra pool = ILybra(pools[i]); uint borrowed = pool.getBorrowedOf(user); //@audit external call if (pool.getVaultType() == 1) { borrowed = borrowed * (1e20 + peUSDExtraRatio) / 1e20; } amount += borrowed;
Ensure that loops are bounded and their iterations are reasonable and predictable
#0 - c4-pre-sort
2023-07-27T19:32:57Z
JeffCX marked the issue as high quality report
#1 - c4-judge
2023-07-27T23:58:12Z
0xean marked the issue as grade-a
#2 - c4-sponsor
2023-07-29T11:20:20Z
LybraFinance marked the issue as sponsor acknowledged
🌟 Selected for report: JCN
Also found by: 0xAnah, DavidGiladi, MohammedRizwan, Rageur, Raihan, ReyAdmirado, Rolezn, SAAJ, SAQ, SM3_SS, Sathish9098, ayo_dev, dharma09, fatherOfBlocks, hunter_w3b, mgf15, mrudenko, naman1778, shamsulhaq123, souilos, turvy_fuzz
80.434 USDC - $80.43
Total Gas saved from all findings - 68800 GAS
Optimizing gas consumption with tight variable packing- Saves 20000 GAS , 10 SLOTS
Structs can be packed into fewer storage slots- Saves 2000 GAS, 1 SLOT
maxSupply can be declared as constant to save large volume of gas - Saves 42000 GAS
Using storage instead of memory for structs/arrays saves gas- Saves 4100 GAS
State variables should be cached outside the loop to save gas- Saves 100 GAS per iterations
If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper
EUSDMiningIncentives.sol: duration ,finishAt, updatedAt, rewardRatio, extraRatio ,peUSDExtraRatio Variables can be uint128 instead of uint256.biddingFeeRatio can be uint94 instead of uint256 : Saves 8000 GAS , 4 SLOTduration,finishAt,updatedAt,rewardRatio can be changed from uint256 to uint128. The maximum value that a uint128 variable can store is 18,446,744,073,709,551,615 (2^128 - 1). The block timestamp will not reach this value for over 280 billion years. Therefore, a uint128 variable is enough to store the block timestamp for the foreseeable future. Saves 4000 gas , 2 SLOT
extraRatio, peUSDExtraRatio can be changed from uint256 to uint128. The values of extraRatio, peUSDExtraRatio not exceeds 1e20 as per require condition checks require(ratio <= 1e20, "BCE"), require(ratio <= 1e20, "BCE"). So uint128 alone enough instead of uin256. Saves 2000 gas , 1 SLOT
biddingFeeRatio can be uint94 instead of uitn256. biddingFeeRatio value not exceeds 8000 as per require(_biddingRatio <= 8000, "BCE") this check. Saves 2000 gas , 1 SLOT
FILE: Breadcrumbs2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol 35: // Duration of rewards to be paid out (in seconds) - 36: uint256 public duration = 2_592_000; + 36: uint128 public duration = 2_592_000; 37: // Timestamp of when the rewards finish - 38: uint256 public finishAt; + 38: uint128 public finishAt; 39: // Minimum of last updated time and reward finish time - 40: uint256 public updatedAt; + 40: uint128 public updatedAt; 41: // Reward to be paid out per second - 42: uint256 public rewardRatio; + 42: uint128 public rewardRatio; 43: // Sum of (reward ratio * dt * 1e18 / total supply) 44: uint256 public rewardPerTokenStored; 45: // User address => rewardPerTokenStored 46: mapping(address => uint256) public userRewardPerTokenPaid; 47: // User address => rewards to be claimed 48: mapping(address => uint256) public rewards; 49: mapping(address => uint256) public userUpdatedAt; - 50: uint256 public extraRatio = 50 * 1e18; + 50: uint128 public extraRatio = 50 * 1e18; - 51: uint256 public peUSDExtraRatio = 10 * 1e18; + 51: uint128 public peUSDExtraRatio = 10 * 1e18; - 52: uint256 public biddingFeeRatio = 3000; + 52: uint94 public biddingFeeRatio = 3000; 53: address public ethlbrStakePool; 54: address public ethlbrLpToken;
LybraConfigurator.sol: redemptionFee ,flashloanFee ,maxStableRatio can be uint94 instead of uint256. Saves 6000 GAS, 3 SLOTredemptionFee is not exceeds 500 because of this check require(newFee <= 500, "Max Redemption Fee is 5%");
flashloanFee is not exceeds 10_000 because of this check if (fee > 10_000) revert('EL');
maxStableRatio is not exceeds 10_000 because of this check require(_ratio <= 10_000, "The maximum value is 10000");
FILE: 2023-06-lybra/contracts/lybra/configuration/LybraConfigurator.sol - 49: uint256 public redemptionFee = 50; + 49: uint94 public redemptionFee = 50; 50: IGovernanceTimelock public GovernanceTimelock; 51: 52: IeUSDMiningIncentives public eUSDMiningIncentives; 53: IProtocolRewardsPool public lybraProtocolRewardsPool; 54: IEUSD public EUSD; 55: IEUSD public peUSD; - 56: uint256 public flashloanFee = 500; - 56: uint94 public flashloanFee = 500; 57: // Limiting the maximum percentage of eUSD that can be cross-chain transferred to L2 in relation to the total supply. - 58: uint256 maxStableRatio = 5_000; + 58: uint94 maxStableRatio = 5_000; 59: address public stableToken;
ProtocolRewardsPool.sol: grabFeeRatio can be uint94 instead of uint256 since the value not exceeds 8000 as per this check require(_ratio <= 8000, "BCE"); : Saves 2000 GAS, 1 SLODFILE: 2023-06-lybra/contracts/lybra/miner/ProtocolRewardsPool.sol 26: IesLBR public esLBR; 27: IesLBR public LBR; + 41: uint94 public grabFeeRatio = 3000; 28: IesLBRBoost public esLBRBoost; 29: 30: // Sum of (reward ratio * dt * 1e18 / total supply) 31: uint public rewardPerTokenStored; 39: uint256 immutable exitCycle = 90 days; 40: uint256 public grabableAmount; - 41: uint256 public grabFeeRatio = 3000; 42: event Restake(address indexed user, uint256 amount, uint256 time);
stakerewardV2pool.sol: duration ,finishAt, updatedAt, rewardRatio Variables can be uint128 instead of uint256: Saves 4000 Gas , 2 SLOTduration,finishAt,updatedAt,rewardRatio can be changed from uint256 to uint128. The maximum value that a uint128 variable can store is 18,446,744,073,709,551,615 (2^128 - 1). The block timestamp will not reach this value for over 280 billion years. Therefore, a uint128 variable is enough to store the block timestamp for the foreseeable future. Saves 4000 gas , 2 SLOT FILE: 2023-06-lybra/contracts/lybra/miner/stakerewardV2pool.sol - 23: uint256 public duration = 2_592_000; + 23: uint128 public duration = 2_592_000; 24: // Timestamp of when the rewards finish - 25: uint256 public finishAt; + 25: uint128 public finishAt; 26: // Minimum of last updated time and reward finish time - 27: uint256 public updatedAt; + 27: uint128 public updatedAt; 28: // Reward to be paid out per second - 29: uint256 public rewardRatio; + 29: uint128 public rewardRatio; 30: // Sum of (reward ratio * dt * 1e18 / total supply) 31: uint256 public rewardPerTokenStored;
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.
esLBRBoost.sol: unlockTime,duration can be uint128 instead of uint256 : Saves 2000 GAS, 1 SLOTThe maximum value that a uint128 variable can store is 18,446,744,073,709,551,615 (2^128 - 1). The block timestamp will not reach this value for over 280 billion years. Therefore, a uint128 variable is enough to store the block timestamp for the foreseeable future
FILE: 2023-06-lybra/contracts/lybra/miner/esLBRBoost.sol 18: struct LockStatus { - 19: uint256 unlockTime; + 19: uint128 unlockTime; - 20: uint256 duration; + 20: uint128 duration; 21: uint256 miningBoost; 22: }
maxSupply can be declared as constant to save large volume of gasThe maxSupply value not changed any where inside the contract. This value only used for checking totalSupply() + amount <= maxSupply
The maxSupply variable can be declared as a constant to save gas. This is because constants are stored in memory, which is a much cheaper operation than storing variables in storage.
The expression 100_000_000 * 1e18 evaluates to a very large number, which would require a lot of gas to store in storage. By declaring maxSupply as a constant, we can avoid this gas cost.
| Contract | Gas Cost |
|---|---|
maxSupply as a variable | 21,000 gas |
maxSupply as a constant | 1,500 gas |
esLBR.sol,LBR.sol: maxSupply should be declared as constant instead of state variable : Saves 42000 GAS, 200 GAS for each callFILE: 2023-06-lybra/contracts/lybra/token/esLBR.sol - 20: uint256 maxSupply = 100_000_000 * 1e18; + 20: uint256 public constant maxSupply = 100_000_000 * 1e18;
FILE: 2023-06-lybra/contracts/lybra/token/LBR.sol - 15: uint256 maxSupply = 100_000_000 * 1e18; + 20: uint256 public constant maxSupply = 100_000_000 * 1e18;
The constructor of the LybraRETHVault contract takes the address of the rkPool contract as a parameter. This means that the value of rkPool is already known to the contract when it is deployed. There is no need to store the value of rkPool in the state of the contract, as it can be retrieved from the parameter of the constructor
Storing the value of rkPool in the state of the contract would consume unnecessary gas. The gas cost of storing a 20-byte address in the state of a contract is 200 gas. This means that storing the value of rkPool would consume 200 gas, even though the value of rkPool is already known to the contract
FILE: 2023-06-lybra/contracts/lybra/pools/LybraRETHVault.sol 18: IRkPool rkPool = IRkPool(0xDD3f50F8A6CafbE9b31a427582963f465E745AF8);
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
esLBRBoost.sol: storage should be used instead of ```memory: Saves 4100 GAS FILE: 2023-06-lybra/contracts/lybra/miner/esLBRBoost.sol - 39: esLBRLockSetting memory _setting = esLBRLockSettings[id]; + 39: esLBRLockSetting strorage _setting = esLBRLockSettings[id];
calldata instead of memory for read-only arguments in external functions saves gasWhen 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
EUSDMiningIncentives.sol: Use calldata instead of memory: Saves 312 GAS per iterationFILE: 2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol - 93: function setPools(address[] memory _pools) external onlyOwner { + 93: function setPools(address[] calldata _pools) external onlyOwner { 94: for (uint i = 0; i < _pools.length; i++) { 95: require(configurator.mintVault(_pools[i]), "NOT_VAULT"); 96: } 97: pools = _pools; 98: }
esLBRBoost.sol: Use calldata instead of memory: Saves 312 GAS FILE: 2023-06-lybra/contracts/lybra/miner/esLBRBoost.sol - 33: function addLockSetting(esLBRLockSetting memory setting) external onlyOwner { + 33: function addLockSetting(esLBRLockSetting calldata setting) external onlyOwner { 34: esLBRLockSettings.push(setting); 35: }
State variables should be cached outside the loop to save gas. This is because the gas cost of accessing a state variable inside a loop is higher than the gas cost of accessing a state variable outside a loop
peUSDExtraRatio should be cached outside the loop: Saves 100 GAS per iterationsFILE: 2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol + uint256 _peUSDExtraRatio= peUSDExtraRatio; 138:for (uint i = 0; i < pools.length; i++) { 139: ILybra pool = ILybra(pools[i]); 140: uint borrowed = pool.getBorrowedOf(user); 141: if (pool.getVaultType() == 1) { - 142: borrowed = borrowed * (1e20 + peUSDExtraRatio) / 1e20; + 142: borrowed = borrowed * (1e20 + _peUSDExtraRatio) / 1e20; 143: }
#0 - c4-pre-sort
2023-07-13T21:00:41Z
JeffCX marked the issue as high quality report
#1 - c4-sponsor
2023-07-27T07:25:50Z
LybraFinance marked the issue as sponsor acknowledged
#2 - c4-judge
2023-07-27T23:42:22Z
0xean marked the issue as grade-a
🌟 Selected for report: Sathish9098
Also found by: 0x3b, 0xbrett8571, ABAIKUNANBAEV, K42, MrPotatoMagic, hl_, ktg, peanuts, solsaver
638.7063 USDC - $638.71
| Head | Details |
|---|---|
| Approach taken in evaluating the codebase | What is unique? How are the existing patterns used? |
| Codebase quality analysis | its structure, readability, maintainability, and adherence to best practices |
| Centralization risks | power, control, or decision-making authority is concentrated in a single entity |
| Bug Fix | process of identifying and resolving issues or errors |
| Gas Optimization | process of reducing the amount of gas consumed by a smart contract or transaction on a blockchain network |
| Other recommendations | Recommendations for improving the quality of your codebase |
| Time spent on analysis | Time detail of individual stages in my code review and analysis |
Steps:
Use a static code analysis tool: Static code analysis tools can scan the code for potential bugs and vulnerabilities. These tools can be used to identify a wide range of issues, including:
Read the documentation: The documentation for Lybra Finance should provide a detailed overview of the protocol and its codebase. This documentation can be used to understand the purpose of the code and to identify potential areas of concern.
Scope the analysis: Once you have a basic understanding of the protocol and its codebase, you can start to scope the analysis. This involves identifying the specific areas of code that you want to focus on. For example, you may want to focus on the code that handles user input, the code that interacts with external APIs, or the code that stores sensitive data.
Manually review the code: Once you have scoped the analysis, you can start to manually review the code. This involves reading the code line-by-line and looking for potential problems. Some of the things you should look for include:
Mark vulnerable code parts with @audit tags: Once you have identified any potential vulnerabilities, you should mark them with @audit tags. This will help you to identify the vulnerable code parts later on.
Dig deep into vulnerable code parts and compare with documentations: For each vulnerable code part, you should dig deep to understand how it works and why it is vulnerable. You should also compare the code with the documentation to see if there are any discrepancies.
Perform a series of tests: Once you have finished reviewing the code, you should perform a series of tests to ensure that it works as intended. These tests should cover a wide range of scenarios, including:
Report any problems: If you find any problems with the code, you should report them to the developers of Lybra Finance. The developers will then be able to fix the problems and release a new version of the protocol.
vaultBadCollateralRatio,vaultSafeCollateralRatio,redemptionProvider don't have any visibility.vaultMintPaused and vaultBurnPaused into a single mapping that stores a struct with both flagsA single point of failure is not acceptable for this project Centrality risk is high in the project, the role of 'onlyOwner' detailed below has very critical and important powers
Project and funds may be compromised by a malicious or stolen private key onlyOwner msg.sender
FILE: 2023-06-lybra/contracts/lybra/miner/EUSDMiningIncentives.sol 84: function setToken(address _lbr, address _eslbr) external onlyOwner { 89: function setLBROracle(address _lbrOracle) external onlyOwner { 93: function setPools(address[] memory _pools) external onlyOwner { 100:function setBiddingCost(uint256 _biddingRatio) external onlyOwner { 105:function setExtraRatio(uint256 ratio) external onlyOwner { 110: function setPeUSDExtraRatio(uint256 ratio) external onlyOwner { 115: function setBoost(address _boost) external onlyOwner { 119: function setRewardsDuration(uint256 _duration) external onlyOwner { 124: function setEthlbrStakeInfo(address _pool, address _lp) external onlyOwner { 128: function setEUSDBuyoutAllowed(bool _bool) external onlyOwner { FILE: 2023-06-lybra/contracts/lybra/miner/ProtocolRewardsPool.sol 52: function setTokenAddress(address _eslbr, address _lbr, address _boost) external onlyOwner { 58: function setGrabCost(uint256 _ratio) external onlyOwner { FILE: Breadcrumbs2023-06-lybra/contracts/lybra/miner/stakerewardV2pool.sol 121: function setRewardsDuration(uint256 _duration) external onlyOwner { 127: function setBoost(address _boost) external onlyOwner { 132: function notifyRewardAmount(uint256 _amount) external onlyOwner updateReward(address(0)) {
Does it use a timelock function?: True timeclock functions not implemented as per documentations
Potential reentrancy vulnerability the code includes external contract calls, such as EUSD.transfer and peUSD.transfer. If these contracts are not implemented securely and follow the checks-effects-interactions pattern, there may be a risk of reentrancy attacks.
deposit and withdraw, are not properly checked for potential integer overflow or underflow.
Should disable the renownceOwnerhip() when ever we use Ownable. Its possible onlyOwner can renounceOwner in unexpected way. So contracts become deep trouble without owner
The withdraw function allows the sender to specify an arbitrary address to send the funds. This design could be vulnerable to denial-of-servicescenario by consumingexcessive gas`` during the withdrawal process.
Consider adding event logging to important contract functions, such as deposit and withdraw important state changes
The setBiddingCost function should include a check to ensure that the _biddingRatio is within a valid range.
The contract includes some external function calls (e.g., EUSD.transferFrom, configurator.distributeRewards), but it does not handle potential exceptions that could arise from these calls.
Some state variables, such as redemptionFee, flashloanFee, and maxStableRatio, are directly modifiable by privileged roles without any additional checks or restrictions.
The code does not include mechanisms to prevent or mitigate DoS attacks, such as gas limit restrictions, rate limiting, or circuit breakers. Malicious actors could potentially exploit vulnerable areas in the code to consume excessive gas, leading to DoS attacks.
In the notifyRewardAmount function, when calculating the rewardPerTokenStored, there could be precision loss when performing division operations. The code should handle the decimal places appropriately and ensure that precision loss does not occur, especially when dealing with token amounts or ratios
Some calculations, especially in the getPreUnlockableAmount function, involve multiple operations that may consume a significant amount of gas. Optimizing these calculations can improve the contract's gas efficiency and reduce transaction costs
Review data types: Analyze the data types used in your smart contracts and consider if they can be further optimized. For example, changing uint256 to uint128 or uint94 can save gas and storage slots
Struct packing : Look for opportunities to pack structs into fewer storage slots. By carefully selecting appropriate data types for struct members, you can reduce the overall storage usage.
Use constant values : If certain values in your contracts are constant and do not change, declare them as constants rather than storing them as state variables. This can significantly save gas costs.
Avoid unnecessary storage : Examine your code and eliminate any unnecessary storage of variables or addresses that are not required for contract functionality.
Storage vs. memory usage : When working with arrays or structs, consider whether using storage instead of memory can save gas. Using storage allows direct access to the state variables and avoids unnecessary copying of data.
replacing the use of memory with calldata for read-only arguments in external functions
15 Hours
15 hours
#0 - c4-sponsor
2023-07-27T08:39:17Z
LybraFinance marked the issue as sponsor confirmed
#1 - c4-judge
2023-07-28T17:09:04Z
0xean marked the issue as grade-a
#2 - c4-judge
2023-07-28T17:10:11Z
0xean marked the issue as selected for report