Platform: Code4rena
Start Date: 16/02/2023
Pot Size: $144,750 USDC
Total HM: 17
Participants: 154
Period: 19 days
Judge: Trust
Total Solo HM: 5
Id: 216
League: ETH
Rank: 37/154
Findings: 2
Award: $370.05
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: GalloDaSballo
Also found by: 0x3b, 0xAgro, 0xSmartContract, 0xTheC0der, 0xackermann, 0xnev, 0xsomeone, ABA, BRONZEDISC, Bjorn_bug, Bnke0x0, Breeje, Co0nan, CodeFoxInc, CodingNameKiki, DadeKuma, DeFiHackLabs, IceBear, Josiah, Kaysoft, Lavishq, MohammedRizwan, PaludoX0, PawelK, Phantasmagoria, Raiders, RaymondFam, Rickard, Rolezn, Sathish9098, SleepingBugs, SuperRayss, UdarTeam, Udsen, Viktor_Cortess, arialblack14, ast3ros, bin2chen, brgltd, btk, catellatech, ch0bu, chaduke, chrisdior4, codeislight, cryptonue, delfin454000, descharre, dontonka, emmac002, fs0c, hacker-dom, hansfriese, imare, lukris02, luxartvinsec, martin, matrix_0wl, peanuts, rbserver, shark, tnevler, trustindistrust, tsvetanovv, vagrant, yongskiws, zzzitron
61.2601 USDC - $61.26
TYPE : LOW FINDING
Description :
Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.
Which sets the contract owner to the zero address. Once ownership has been renounced, the contract owner will no longer be able to perform any actions that require ownership, and ownership of the contract will effectively be transferred to no one
onlyOwner Functions :
File : CollateralConfig.sol
function initialize( address[] calldata _collaterals, uint256[] calldata _MCRs, uint256[] calldata _CCRs ) external override onlyOwner { function updateCollateralRatios( address _collateral, uint256 _MCR, uint256 _CCR ) external onlyOwner checkCollateral(_collateral) {
[LINK TO CODE] (https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/CollateralConfig.sol)
File : BorrowerOperations.sol
FILE : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
function setAddresses ( address _oathTokenAddress, address _stabilityPoolAddress ) external onlyOwner override
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
101 : function fund(uint amount) external onlyOwner {
120 : function updateDistributionPeriod(uint256 _newDistributionPeriod) external onlyOwner {
Recommended Mitigation Steps :
We recommend either reimplementing the function to disable it or to clearly specify if it is part of the contract design.
TYPE : LOW FINDING
Instances (12) :
All contracts in a smart contract system should ideally use the same version of the Solidity programming language in order to ensure compatibility and prevent unexpected errors.
Context: All contracts
The pragma versions used are:
pragma solidity 0.6.11 and 0.8.0
The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes
0.8.14:
ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions. 0.8.15
Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks. 0.8.16
Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array. 0.8.17
Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call. Apart from these, there are several minor bug fixes and improvements
IN ETHOS -CORE ALL CONTRACTS USING SOLIDITY VERSION 0.6.11. THE VERSION SHOULD BE UPDATED TO SOLIDITY 0.8.17
INSTANCES (8) :
3: pragma solidity 0.6.11;
IN ETHOS -VAULT ALL CONTRACTS USING SOLIDITY VERSION 0.8.0 . THE VERSION SHOULD BE UPDATED TO SOLIDITY 0.8.17
INSTANCES (4) :
File: 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
3 : pragma solidity ^0.8.0;
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
3: pragma solidity ^0.8.0;
Recommendation:
Old version of Solidity is used , newer version can be used (0.8.17)
TYPE : NON CRITICAL (NC)
The named imports only supports only from solidity version 0.8.0. So, Its possible to update this in Ethos-Vault contracts
Context: ReaperVaultERC4626.sol,ReaperBaseStrategyv4.sol,ReaperStrategyGranarySupplyOnly.sol,ReaperVaultV2.sol
Using named imports instead of plain imports can make your Solidity code more readable and reduce the risk of naming conflicts between different contracts and libraries
When you use a plain import statement in Solidity, you are importing all of the contracts and libraries defined in the imported file. This can make it difficult to determine which contracts or libraries are actually being used in your code, and can lead to naming conflicts if you import multiple files that define contracts or libraries with the same name
This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better
SOLUTIONS :
- import "./Dependencies/CheckContract.sol"; + import {CheckContract} from "./Dependencies/CheckContract.sol";
Recommended Mitigation :
USE NAMED IMPORTS FOR ALL POSSIBLE CONTRACT
TYPE : NON CRITICAL (NC)
There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values).
This will help with readability and easier maintenance for future changes. This also helps with any issues, as some of these hard-coded values are admin addresses
(https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L40-L41) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L73-L76) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L32-L35) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L42-L52)
TYPE : NON CRITICAL (NC)
CONTEXT: ALL CONTRACTS
The solidity documentation recommends a maximum of 120 characters
Consider adding a limit of 120 characters or less to prevent large lines.
(https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L637) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L645) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L675) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L697) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L538) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L528-L530) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L517) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L511) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L343) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L312) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L282) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L268-L272) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L259) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L254) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L248) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L233-L235) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L190) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L172) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L105)
TYPE : NON CRITICAL (NC)
The Solidity style guide recommends declaring modifiers before the functions. Now modifiers declared bottom of the contract .
Another recommendation is to declare internal functions below external functions
If possible, consider adding internal functions below external functions for the contract layout. So please move external function on top of the internal function.
TYPE : NON CRITICAL (NC)
File : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
function _getCollChange( uint _collReceived, uint _requestedCollWithdrawal ) internal pure returns(uint collChange, bool isCollIncrease)
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
function _liquidateNormalMode( IActivePool _activePool, IDefaultPool _defaultPool, address _collateral, address _borrower, uint _LUSDInStabPool ) internal returns (LiquidationValues memory singleLiquidation)
function _getOffsetAndRedistributionVals ( uint _debt, uint _coll, uint _LUSDInStabPool ) internal pure returns (uint debtToOffset, uint collToSendToSP, uint debtToRedistribute, uint collToRedistribute)
488 : returns (LiquidationValues memory singleLiquidation)
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
function asset() external view override returns (address assetTokenAddress) { return address(token); }
function totalAssets() external view override returns (uint256 totalManagedAssets) { return balance(); }
function convertToShares(uint256 assets) public view override returns (uint256 shares) { if (totalSupply() == 0 || _freeFunds() == 0) return assets; return (assets * totalSupply()) / _freeFunds(); }
66 : function convertToAssets(uint256 shares) public view override returns (uint256 assets) {
79 : function maxDeposit(address) external view override returns (uint256 maxAssets) {
function previewDeposit(uint256 assets) external view override returns (uint256 shares) { return convertToShares(assets); }
122 : function maxMint(address) external view override returns (uint256 maxShares) {
165 : function maxWithdraw(address owner) external view override returns (uint256 maxAssets) {
220 : function maxRedeem(address owner) external view override returns (uint256 maxShares) {
240 : function previewRedeem(uint256 shares) external view override returns (uint256 assets) {
TYPE : LOW FINDING
decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.
File : 2023-02-ethos/Ethos-Core/contracts/CollateralConfig.sol
63 : uint256 decimals = IERC20(collateral).decimals();
Recommended mitigations:
Avoids using decimals() function anywhere in the contract
TYPE : NON CRITICAL (NC)
Using scientific notation for large multiples of ten will improve code readability
FILE : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
uint256 public yieldSplitTreasury = 20_00; // amount of yield to direct to treasury in BPS uint256 public yieldSplitSP = 40_00; // amount of yield to direct to stability pool in BPS uint256 public yieldSplitStaking = 40_00; // amount of yield to direct to OATH Stakers in BPS
TYPE : LOW FINDING
Your current version of @openzeppelin/contracts is ^4.7.3 and latest version is 4.8.1
Your current version of @openzeppelin/contracts-upgradeable is ^4.7.3 and latest version is 4.8.1
Recommended Mitigation :
Use latest versions of openzeppelin instead of vulnerable versions
TYPE : NON CRITICAL (NC)
TYPE : NON CRITICAL (NC)
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
LocalVariables_InnerSingleLiquidateFunction memory vars; (singleLiquidation.entireTroveDebt, singleLiquidation.entireTroveColl, vars.pendingDebtReward, vars.pendingCollReward) = getEntireDebtAndColl(_borrower, _collateral); _movePendingTroveRewardsToActivePool(_activePool, _defaultPool, _collateral, vars.pendingDebtReward, vars.pendingCollReward); _removeStake(_borrower, _collateral); singleLiquidation.collGasCompensation = _getCollGasCompensation(singleLiquidation.entireTroveColl); singleLiquidation.LUSDGasCompensation = LUSD_GAS_COMPENSATION; uint collToLiquidate = singleLiquidation.entireTroveColl.sub(singleLiquidation.collGasCompensation);
This same codes repeated in_liquidateRecoveryMode() and _liquidateNormalMode() Functions . This line of coded can be reused instead of repeating in both functions
TYPE : LOW FINDING
it's important to properly check integer ranges to avoid unexpected behavior and vulnerabilities in your smart contract.
INTEGER CHECKS :
<= --- Upper bound check >= ---- Lower bound check >= && <= -- Range check >0 ------ Non Zero check
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
File : File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
(https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L328-L334) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L336-L342) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L320-L326) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/LUSDToken.sol#L311-L318)
Recommended Mitigation :
Perform all necessary input validations
TYPE : NON CRITICAL (NC)
Replace CONSTANT PUBLIC with PUBLIC CONSTANT
FILE : 2023-02-ethos/Ethos-Core/contracts/CollateralConfig.sol
21: uint256 constant public MIN_ALLOWED_MCR = 1.1 ether; // 110% 25: uint256 constant public MIN_ALLOWED_CCR = 1.5 ether; // 150%
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
48: uint constant public SECONDS_IN_ONE_MINUTE = 60; 53: uint constant public MINUTE_DECAY_FACTOR = 999037758833783000; 54: uint constant public override REDEMPTION_FEE_FLOOR = DECIMAL_PRECISION / 1000 * 5; // 0.5% 55: uint constant public MAX_BORROWING_FEE = DECIMAL_PRECISION / 100 * 5; // 5% 61: uint constant public BETA = 2;
FILE : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
21: string constant public NAME = "BorrowerOperations";
FILE : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
30 : string constant public NAME = "ActivePool";
FILE : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
150 : string constant public NAME = "StabilityPool";
Consider using only one approach throughout the codebase, e.g. only uint or only uint256.
TYPE : NON CRITICAL (NC)
Context: ALL CONTRACTS
(https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L48-L63) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L67-L77) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L616) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L620) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L628) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L663-L669) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L684-L691) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/BorrowerOperations.sol#L726-L732) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L130-L137) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L147-L157) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L480-L484) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L673-L679) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L870) (https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L1236)
TYPE : LOW FINDING
safeApprove() is generally considered to be a safer and more secure way of approving spenders in an ERC20 contract, as it includes additional checks and safeguards to prevent certain types of vulnerabilities that can arise from a poorly implemented approve() function
The approve() function allows an address to spend a certain amount of tokens on behalf of the token owner. One vulnerability that can arise with the approve() function is known as the "double-spending" attack, where an attacker can approve a spender to transfer tokens, then transfer all their tokens to a different address, making the previously approved transfer invalid.
To prevent this type of attack, the safeApprove() function typically includes additional checks to ensure that the current allowance for a spender is zero before setting a new allowance. This prevents an attacker from double-spending an allowance that has already been approved
File: Ethos-Core/contracts/LUSDToken.sol
231: _approve(msg.sender, spender, amount); 238: _approve(sender, msg.sender, _allowances[sender][msg.sender].sub(amount, "ERC20: transfer amount exceeds allowance")); 243: _approve(msg.sender, spender, _allowances[msg.sender][spender].add(addedValue)); 248: _approve(msg.sender, spender, _allowances[msg.sender][spender].sub(subtractedValue, "ERC20: decreased allowance below zero")); 289: _approve(owner, spender, amount);
Recommended Mitigation :
Consider safeApprove() function instead of normal approve()
TYPE : LOW FINDING
safeMint() is generally considered to be a safer and more secure way of minting new tokens in an ERC20 contract, as it includes additional checks and safeguards to prevent certain types of vulnerabilities that can arise during token minting.
File: Ethos-Core/contracts/LUSDToken.sol
188: _mint(_account, _amount);
Recommended Mitigation :
Consider safeMint() function instead of normal mint()
TYPE : NON CRITICAL (NC)
It is bad practice to use numbers directly in code without explanation
FIE : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
127 : require(_bps <= 10_000, "Invalid BPS value");
133 : require(_driftBps <= 500, "Exceeds max allowed value of 500 BPS");
145 : require(_treasurySplit + _SPSplit + _stakingSplit == 10_000, "Splits must add up to 10000 BPS");
File : 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
23: uint256 public constant PERCENT_DIVISOR = 10_000;
TYPE : LOW FINDING
As per current protocol implementation using setYieldingPercentage() function possible to set _bps 100% . A yield of 100% could be an annual percentage yield (APY), which means that a user's investment can double in value over a year.
require(_bps <= 10_000, "Invalid BPS value")
It's important to note that high yield opportunities in DeFi come with their own set of risks, including smart contract vulnerabilities, liquidity risks, and impermanent loss
FIE : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
127 : require(_bps <= 10_000, "Invalid BPS value");
Recommended mitigation :
Consider reducing _bps as per other yield farming protocols
TYPE : NON CRITICAL (NC)
Description I recommend using header for Solidity code layout and readability
(https://github.com/transmissions11/headers)
TYPE : NON CRITICAL (NC)
FILE : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
195: uint public P = DECIMAL_PRECISION;
TYPE : NON CRITICAL (NC)
Use camel case for all functions, parameters and variables and snake case for constants
Snake case examples The following variable names follow the snake case naming convention:
this_is_snake_case build_docker_image run_javascript_function call_python_method ruby_loves_snake_case
Camel case examples The following are examples of variables that use the camel case naming convention:
FileNotFoundException toString SpringBoot TomcatServer getJson
FILE : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
228 : mapping (address => uint) public lastCollateralError_Offset; 229: uint public lastLUSDLossError_Offset;
Here state variables lastCollateralError_Offset, lastLUSDLossError_Offset using Snake case instead Camel case
After Mitigation Camel Case :
lastCollateralErrorOffset, lastLUSDLossErrorOffset
File: 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
F_Collateral_Snapshot,F_LUSD_Snapshot variables using snake case instead of camel case
mapping (address => uint) F_Collateral_Snapshot; uint F_LUSD_Snapshot;
Functions are using snake case instead of camel case :
TYPE : NON CRITICAL (NC)
Consider Dependencies first, then all interfaces
TYPE : LOW FINDING
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.
FILE : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
87 : int256 endTimestamp = block.timestamp > lastDistributionTime ? lastDistributionTime : block.timestamp; 93: lastIssuanceTimestamp = block.timestamp
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
540: strategy.lastReport = block.timestamp; 541: lastReport = block.timestamp;
TYPE : LOW FINDING
It is important to check the lengths of the assets and amounts arrays before entering into the loop, as this can prevent potential vulnerabilities and errors that may occur when iterating through the arrays.
In Solidity, arrays are zero-indexed, meaning that the first element in the array is at index 0, the second element is at index 1, and so on. If the lengths of the assets and amounts arrays are not equal, it's possible that the loop may iterate over an index that is out of bounds for one of the arrays, causing an array index out of bounds error.
To prevent this issue, you can add a check at the beginning of your function to ensure that the assets and amounts arrays have the same length
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
unction _sendCollGainToUser(address[] memory assets, uint[] memory amounts) internal { uint numCollaterals = assets.length; for (uint i = 0; i < numCollaterals; i++) { if (amounts[i] != 0) { address collateral = assets[i]; emit CollateralSent(msg.sender, collateral, amounts[i]); IERC20(collateral).safeTransfer(msg.sender, amounts[i]); } } }
Recommended Mitigation step:
require(amounts.length == assets.length, "Amounts and assets arrays must have the same length");
TYPE : LOW FINDING
If we want to emit the _lusdTokenAddress, we should use the LUSDTokenAddressSet event. However, in this case, the incorrect LQTYTokenAddressSet event was used.
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
94 : emit LQTYTokenAddressSet(_lusdTokenAddress);
Recommended Mitigation :
- 85 : emit LQTYTokenAddressSet(_lusdTokenAddress); + 85 : emit LUSDTokenAddressSet (_lusdTokenAddress);
File: File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
function _burn(address account, uint256 amount) internal { assert(account != address(0)); _balances[account] = _balances[account].sub(amount, "ERC20: burn amount exceeds balance"); _totalSupply = _totalSupply.sub(amount); emit Transfer(account, address(0), amount); /// @Audit emit Burn event instead of Transfer Event }
Transfer event was used instead burn event
TYPE : LOW FINDING
In case the addresses change due to reasons such as updating their versions in the future, addresses coded as constants cannot be updated.
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
42 : bytes32 private constant _PERMIT_TYPEHASH = 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; 43: bytes32 private constant _TYPE_HASH = 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f;
File: 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
24: address public constant VELO_ROUTER = 0xa132DAB612dB5cB9fC9Ac426A0Cc215A3423F9c9; 25: ILendingPoolAddressesProvider public constant ADDRESSES_PROVIDER = ILendingPoolAddressesProvider(0xdDE5dC81e40799750B92079723Da2acAF9e1C6D6); 27: IAaveProtocolDataProvider public constant DATA_PROVIDER = IAaveProtocolDataProvider(0x9546F673eF71Ff666ae66d01Fd6E7C6Dae5a9995); 29: IRewardsController public constant REWARDER = IRewardsController(0x6A0406B8103Ec68EE9A713A073C7bD587c5e04aD);
Recommended Mitigation Steps :
So it is recommended to add the update option with the onlyOwner modifier
TYPE : NON CRITICAL (NC)
variables with underscores at the beginning of their names are treated differently from other variables in Solidity, Variables with underscores at the beginning of their names are treated as "anonymous variables" by the Solidity compiler
A common convention is to use camelCase for variable names, starting with a lowercase letter
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
54 : mapping (address => uint256) private _nonces;
57: mapping (address => uint256) private _balances;
58: mapping (address => mapping (address => uint256)) private _allowances;
TYPE : NON CRITICAL (NC)
Replace CONSTANT INTERNAL with INTERNAL CONSTANT
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
string constant internal _NAME = "LUSD Stablecoin"; string constant internal _SYMBOL = "LUSD"; string constant internal _VERSION = "1"; uint8 constant internal _DECIMALS = 18;
TYPE : LOW FINDING
The critical procedures should be two step process
See similar findings in previous Code4rena contests for reference: (https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure)
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
function updateGovernance(address _newGovernanceAddress) external { _requireCallerIsGovernance(); checkContract(_newGovernanceAddress); // must be a smart contract (multi-sig, timelock, etc.) governanceAddress = _newGovernanceAddress; emit GovernanceAddressChanged(_newGovernanceAddress); }
function updateGuardian(address _newGuardianAddress) external { _requireCallerIsGovernance(); checkContract(_newGuardianAddress); // must be a smart contract (multi-sig, timelock, etc.) guardianAddress = _newGuardianAddress; emit GuardianAddressChanged(_newGuardianAddress); }
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
function updateTreasury(address newTreasury) external { _atLeastRole(DEFAULT_ADMIN_ROLE); require(newTreasury != address(0), "Invalid address"); treasury = newTreasury; }
Recommended Mitigation Steps
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
TYPE : LOW FINDING
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
function updateGovernance(address _newGovernanceAddress) external { _requireCallerIsGovernance(); checkContract(_newGovernanceAddress); // must be a smart contract (multi-sig, timelock, etc.) governanceAddress = _newGovernanceAddress; emit GovernanceAddressChanged(_newGovernanceAddress); }
function updateGuardian(address _newGuardianAddress) external { _requireCallerIsGovernance(); checkContract(_newGuardianAddress); // must be a smart contract (multi-sig, timelock, etc.) guardianAddress = _newGuardianAddress; emit GuardianAddressChanged(_newGuardianAddress); }
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
function updateTreasury(address newTreasury) external { _atLeastRole(DEFAULT_ADMIN_ROLE); require(newTreasury != address(0), "Invalid address"); treasury = newTreasury; }
Recommended Mitigation :
require(owner != _newOwner, "The new and old owner address are same ");
TYPE : LOW FINDING
Functions may be defined as external because they need to be called from other contracts, but if they are not used anywhere, it may be safe to remove them from the contract to simplify the code and reduce the attack surface.
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
function pauseMinting() external { require( msg.sender == guardianAddress || msg.sender == governanceAddress, "LUSD: Caller is not guardian or governance" ); mintingPaused = true; } function unpauseMinting() external { _requireCallerIsGovernance(); mintingPaused = false; }
Recommended Mitigation Steps :
If the pauseMinting() and unpauseMinting() functions in a contract are not utilized by any other contracts, and there are no plans to use them in the future, then it is safe to remove them from the contract
TYPE : LOW FINDING
Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters
The events should include the new value and old value where possible
In updateGovernance(), updateGuardian() functions only new address is emitted . No events added to emit old address
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
function updateGovernance(address _newGovernanceAddress) external { _requireCallerIsGovernance(); checkContract(_newGovernanceAddress); // must be a smart contract (multi-sig, timelock, etc.) governanceAddress = _newGovernanceAddress; emit GovernanceAddressChanged(_newGovernanceAddress); }
function updateGuardian(address _newGuardianAddress) external { _requireCallerIsGovernance(); checkContract(_newGuardianAddress); // must be a smart contract (multi-sig, timelock, etc.) guardianAddress = _newGuardianAddress; emit GuardianAddressChanged(_newGuardianAddress); }
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
function updateDistributionPeriod(uint256 _newDistributionPeriod) external onlyOwner { distributionPeriod = _newDistributionPeriod; }
Recommended Mitigation Steps :
Add events for both old and new values for every critical changes
TYPE : LOW FINDING
initialize() function can be called anybody when the contract is not initialized
File: 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
function initialize( address _vault, address[] memory _strategists, address[] memory _multisigRoles, IAToken _gWant ) public initializer {
Recommended Mitigation Steps
Add a control that makes initialize() only call the Deployer Contract;
if (msg.sender != DEPLOYER_ADDRESS) { revert NotDeployer(); }
TYPE : NON CRITICAL (NC)
While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor
File: 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
49: bytes32 public constant KEEPER = keccak256("KEEPER"); 50: bytes32 public constant STRATEGIST = keccak256("STRATEGIST"); 51: bytes32 public constant GUARDIAN = keccak256("GUARDIAN"); 52: bytes32 public constant ADMIN = keccak256("ADMIN");
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
73: bytes32 public constant DEPOSITOR = keccak256("DEPOSITOR"); 74: bytes32 public constant STRATEGIST = keccak256("STRATEGIST"); 75: bytes32 public constant GUARDIAN = keccak256("GUARDIAN"); 76: bytes32 public constant ADMIN = keccak256("ADMIN");
TYPE : NON CRITICAL (NC)
The initialize() function is often declared as external instead of public or internal because it is intended to be called only once, when the contract is being deployed.
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
function initialize( address _vault, address[] memory _strategists, address[] memory _multisigRoles, IAToken _gWant ) public initializer {
TYPE : LOW FINDING
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
231: uint256 stratMaxAllocation = (strategies[stratAddr].allocBPS * balance()) / PERCENT_DIVISOR;
237: uint256 vaultMaxAllocation = (totalAllocBPS * balance()) / PERCENT_DIVISOR;
296: return totalSupply() == 0 ? 10**decimals() : (_freeFunds() * 10**decimals()) / totalSupply();
334: shares = (_amount * totalSupply()) / freeFunds;
365: value = (_freeFunds() * _shares) / totalSupply();
421 : return lockedProfit - ((lockedFundsRatio * lockedProfit) / DEGRADATION_COEFFICIENT);
440 : uint256 bpsChange = Math.min((loss * totalAllocBPS) / totalAllocated, stratParams.allocBPS);
463: uint256 performanceFee = (gain * strategies[strategy].feeBPS) / PERCENT_DIVISOR;
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
53 : return (assets * totalSupply()) / _freeFunds(); 68 : return (shares * _freeFunds()) / totalSupply();
TYPE : NON CRITICAL (NC)
internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)
roundUpDiv() internal function should be starting with underscore => _roundUpDiv()
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
269 : function roundUpDiv(uint256 x, uint256 y) internal pure returns (uint256) {
TYPE : NON CRITICAL (NC)
The parameters that are passed to the function are typically assigned to local variables within the function body. By prefixing these variables with an underscore, we can differentiate them from other variables in the contract, which can make the code more readable and easier to understand.
Solidity developers often use underscores to prefix function parameters as a way to make the code more readable and easier to understand.
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
function updateTreasury(address newTreasury) external { _atLeastRole(DEFAULT_ADMIN_ROLE); require(newTreasury != address(0), "Invalid address"); treasury = newTreasury; }
TYPE : LOW FINDING
According to the protocol descriptions, the updateCollateralRatios() function is designed to decrease the _MCR and _CCR values for a specific asset that has proven itself during tough times
To prevent the same values from being set for variables, the condition check _MCR <= config.MCR and _CCR <= config.CCR should be modified to _MCR < config.MCR and _CCR < config.CCR. If the values are the same, it is unnecessary to set them again, and this process can be skipped
File : 2023-02-ethos/Ethos-Core/contracts/CollateralConfig.sol
function updateCollateralRatios( address _collateral, uint256 _MCR, uint256 _CCR ) external onlyOwner checkCollateral(_collateral) { Config storage config = collateralConfig[_collateral]; require(_MCR <= config.MCR, "Can only walk down the MCR"); // @audit Wrong condition check require(_CCR <= config.CCR, "Can only walk down the CCR"); // @audit Wrong condition check require(_MCR >= MIN_ALLOWED_MCR, "MCR below allowed minimum"); config.MCR = _MCR; require(_CCR >= MIN_ALLOWED_CCR, "CCR below allowed minimum"); config.CCR = _CCR; emit CollateralRatiosUpdated(_collateral, _MCR, _CCR); }
Recommended Mitigation :
Use < instead of <= when check _MCR and _CCR
require(_MCR < config.MCR, "Can only walk down the MCR"); require(_CCR < config.CCR, "Can only walk down the CCR");
TYPE : NON CRITICAL (NC)
Shadowing state variable names with the same contract name can lead to unexpected behavior and make the code harder to read and maintain.
File : 2023-02-ethos/Ethos-Core/contracts/CollateralConfig.sol
35: mapping (address => Config) public collateralConfig;
Here the contract name is CollateralConfig and the state variable name also collateralConfig. This is not good code practice.
TYPE : LOW FINDING
This (currentStake != 0) already checked when calculating collGainAssets, collGainAmounts, LUSDGain values.
currentStake varibale value is not changed anywhere inside the stake() function.
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
134: if (currentStake != 0) { //@audit Unnecessary condition check 135: lusdToken.transfer(msg.sender, LUSDGain); 136: _sendCollGainToUser(collGainAssets, collGainAmounts); 137: }
Recommended Mitigation :
Remove Line 134,137. Unnecessary condition check
-134: if (currentStake != 0) { 135: lusdToken.transfer(msg.sender, LUSDGain); 136: _sendCollGainToUser(collGainAssets, collGainAmounts); -137: }
TYPE : LOW FINDING
If you don't use the emit keyword to trigger an event in Solidity, the event will not be emitted, and no event logs will be created on the blockchain
So if you forget to emit an event in your Solidity contract, it won't have any direct impact on the contract's behavior or state. However, if you intended for the event to be triggered and relied on its emission to perform some action outside of the contract, then your code may not work as expected
File : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
function decreaseLUSDDebt(address _collateral, uint _amount) external override { _requireValidCollateralAddress(_collateral); _requireCallerIsBOorTroveMorSP(); LUSDDebt[_collateral] = LUSDDebt[_collateral].sub(_amount); ActivePoolLUSDDebtUpdated(_collateral, LUSDDebt[_collateral]); /// @audit ActivePoolLUSDDebtUpdated Event called without emit keyword }
function increaseLUSDDebt(address _collateral, uint _amount) external override { _requireValidCollateralAddress(_collateral); _requireCallerIsBOorTroveM(); LUSDDebt[_collateral] = LUSDDebt[_collateral].add(_amount); ActivePoolLUSDDebtUpdated(_collateral, LUSDDebt[_collateral]); /// @audit ActivePoolLUSDDebtUpdated Event called without emit keyword }
Recommended Mitigation :
Call ActivePoolLUSDDebtUpdated event using emit keyword
emit ActivePoolLUSDDebtUpdated(_collateral, LUSDDebt[_collateral]);
TYPE : LOW FINDING
We now recommend that transfer() be avoided as gas costs can and will change.
Ethereum’s upgrade in 2019, introducing cheaper gas costs for certain SSTORE operations suffered a major setback when, as an unwanted side effect, the smart contract enabled re-entrancy attacks when using address.transfer() because lowering gas costs caused code that was previously safe from reentrancy to no longer be.
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
127 : OathToken.transfer(_account, _OathAmount);
FILE : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
135: lusdToken.transfer(msg.sender, LUSDGain);
171 : lusdToken.transfer(msg.sender, LUSDGain);
TYPE : LOW FINDING
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
function updateGovernance(address _newGovernanceAddress) external { _requireCallerIsGovernance(); checkContract(_newGovernanceAddress); // must be a smart contract (multi-sig, timelock, etc.) governanceAddress = _newGovernanceAddress; emit GovernanceAddressChanged(_newGovernanceAddress); }
function updateGuardian(address _newGuardianAddress) external { _requireCallerIsGovernance(); checkContract(_newGuardianAddress); // must be a smart contract (multi-sig, timelock, etc.) guardianAddress = _newGuardianAddress; emit GuardianAddressChanged(_newGuardianAddress); }
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
function updateTreasury(address newTreasury) external { _atLeastRole(DEFAULT_ADMIN_ROLE); require(newTreasury != address(0), "Invalid address"); treasury = newTreasury; }
TYPE : LOW FINDING
On several locations in the code precautions are not being taken for not dividing by 0, this will revert the code. These functions can be called with 0 value in the input, this value is not checked for being bigger than 0, that means in some scenarios this can potentially trigger a division by zero
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
334: shares = (_amount * totalSupply()) / freeFunds;
365: value = (_freeFunds() * _shares) / totalSupply();
440 : uint256 bpsChange = Math.min((loss * totalAllocBPS) / totalAllocated, stratParams.allocBPS);
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
53 : return (assets * totalSupply()) / _freeFunds(); 68 : return (shares * _freeFunds()) / totalSupply();
TYPE : LOW FINDING
The onlyOwner role has a single point of failure and onlyOwner can use critical a few functions.
Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.
onlyOwner Functions :
File : CollateralConfig.sol
function initialize( address[] calldata _collaterals, uint256[] calldata _MCRs, uint256[] calldata _CCRs ) external override onlyOwner { function updateCollateralRatios( address _collateral, uint256 _MCR, uint256 _CCR ) external onlyOwner checkCollateral(_collateral) {
[LINK TO CODE] (https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/CollateralConfig.sol)
File : BorrowerOperations.sol
FILE : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
function setAddresses ( address _oathTokenAddress, address _stabilityPoolAddress ) external onlyOwner override
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
101 : function fund(uint amount) external onlyOwner {
120 : function updateDistributionPeriod(uint256 _newDistributionPeriod) external onlyOwner {
Recommended Mitigation Steps
Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks.
Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.
Also detail them in documentation and NatSpec comments
TYPE : LOW FINDING
Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.
Instances (12)
File : BorrowerOperations.sol
128: assert(MIN_NET_DEBT > 0);
197: assert(vars.compositeDebt > 0);
331: assert(_collWithdrawal <= vars.coll);
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
1281 : assert(closedStatus != Status.nonExistent && closedStatus != Status.active);
1342 : assert(troveStatus != Status.nonExistent && troveStatus != Status.active); 1348 : assert(index <= idxLast);
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
312: assert(sender != address(0)); 313: assert(recipient != address(0));
321: assert(account != address(0));
329: assert(account != address(0));
337: assert(owner != address(0)); 338: assert(spender != address(0));
#0 - c4-judge
2023-03-10T16:50:39Z
trust1995 marked the issue as grade-b
#1 - trust1995
2023-03-10T16:50:50Z
Too many issues are OOS for A.
#2 - c4-sponsor
2023-03-29T01:00:16Z
0xBebis marked the issue as sponsor confirmed
🌟 Selected for report: c3phas
Also found by: 0x3b, 0x6980, 0x73696d616f, 0xSmartContract, 0xackermann, 0xhacksmithh, 0xsomeone, Bnke0x0, Bough, Budaghyan, Darshan, DeFiHackLabs, Deivitto, GalloDaSballo, JCN, LethL, Madalad, MiniGlome, Morraez, P-384, PaludoX0, Phantasmagoria, Praise, RHaO-sec, Rageur, RaymondFam, ReyAdmirado, Rickard, Rolezn, SaeedAlipoor01988, Saintcode_, Sathish9098, TheSavageTeddy, Tomio, Viktor_Cortess, abiih, arialblack14, atharvasama, banky, codeislight, cryptonue, ddimitrov22, dec3ntraliz3d, descharre, dharma09, emmac002, favelanky, hl_, hunter_w3b, kaden, kodyvim, matrix_0wl, oyc_109, pavankv, scokaf, seeu, yamapyblack
308.7866 USDC - $308.79
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
Instances (2) :
Approximate gas saved : 100000 gas for 5 SLOTS
if 4 slots 80000 gas saved
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
35: mapping(address => StrategyParams) public strategies; // (32) 42: uint256 public tvlCap; // (32) 44: uint256 public totalAllocBPS; // (32) 45: uint256 public totalAllocated; // (32) 46: uint256 public lastReport; //(32) 49: bool public emergencyShutdown; //(1) 55: uint256 public withdrawMaxLoss = 1; // (32) 56: uint256 public lockedProfitDegradation; // (32) 57: uint256 public lockedProfit; // (32) 78: address public treasury; // (20)
Variables ordering with 10 slots.
The variable withdrawMaxLoss stores a value of 1 and there is no place in the contract where a higher value is stored for this variable. Therefore, it is safe to change the type of withdrawMaxLoss to uint64 can save one slot
/// @Audit Variable ordering with 8 slots instead of the current 10 slots: /// withdrawalQueue, Mapping(32):strategies, uint256(32) : tvlCap, uint256(32) : totalAllocBPS, uint256(32) : totalAllocated, uint256(32) : lastReport,uint256(32) : lockedProfitDegradation, uint256(32) : lockedProfit, uint64 (8): withdrawMaxLoss , bool(1) : emergencyShutdown, address(20 ) : treasury address[] public withdrawalQueue; mapping(address => StrategyParams) public strategies; // (32) uint256 public tvlCap; // (32) uint256 public totalAllocBPS; // (32) uint256 public totalAllocated; // (32) uint256 public lastReport; //(32) uint256 public lockedProfitDegradation; // (32) uint256 public lockedProfit; // (32) uint64 public withdrawMaxLoss = 1; // (8) bool public emergencyShutdown; // (1) address public treasury; // (20)
So reduce 2 Gsset operations, you can save around 40,000 gas (2 x 20,000 gas).
uint256 public lockedProfit; // (32) uint256 public withdrawMaxLoss = 1; // (32) bool public emergencyShutdown; // (1) address public treasury; // (20)
File : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
49: uint256 public yieldingPercentageDrift = 100; // rebalance iff % is off by more than 100 BPS // Yield distribution params, must add up to 10k 52: uint256 public yieldSplitTreasury = 20_00; // amount of yield to direct to treasury in BPS 53: uint256 public yieldSplitSP = 40_00; // amount of yield to direct to stability pool in BPS 54: uint256 public yieldSplitStaking = 40_00;
Currently variable ordering with 4 slots
It is correct to declare the variables yieldingPercentageDrift, yieldSplitTreasury, yieldSplitSP, and yieldSplitStaking as uint64 instead of uint256 since their values are not expected to exceed the maximum value of uint64. Doing so would help save some gas costs
uint64 can store values ranging from 0 to 18,446,744,073,709,551,615
The values of the variables yieldingPercentageDrift, yieldSplitTreasury, yieldSplitSP, and yieldSplitStaking are not increased anywhere in the contract. Therefore, uint64 is more than enough to store their values. Based on the contract implementation, there are no possibilities for these variables to overflow
Recommended Mitigations :
/// @audit Variable ordering with 1 slot instead of the current 4: /// uint64(8) : yieldingPercentageDrift , uint64(8) : yieldSplitTreasury , uint64(8) : yieldSplitSP , uint64(8) : yieldSplitStaking
Instances (1) :
Total Slots Saved - 3
Approximate gas saved : 60000 gas
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
File : 2023-02-ethos/Ethos-Core/contracts/CollateralConfig.sol
DeFi tokens typically use up to 18 decimal places, which is the standard for Ethereum-based tokens such as ERC20 and ERC777. This is because using more decimal places can lead to precision issues and make calculations more complicated.
The variable decimals can be safely declared as uint128 instead of uint256 since uint128 is enough to store up to 18 decimal places. Therefore, using uint128 for decimals would be more efficient and save gas costs . The maximum value stored in uint128 is 340,282,366,920,938,463,463,374,607,431,768,211,455. This value is more than enough for decimals .
/// @audit Variable ordering with 3 slots instead of the current 4: /// uint256(32): MCR, uint256(32) : CCR, uint128(16) : decimals, bool(1) : allowed 27: struct Config { 28: bool allowed; 29: uint256 decimals; 30: uint256 MCR; 31: uint256 CCR; 32: }
FILE : https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/BorrowerOperations.sol
struct LocalVariables_adjustTrove { uint256 collCCR; //32 uint256 collMCR; //32 uint256 collDecimals; //32 uint price; //32 uint collChange; //32 uint netDebtChange; //32 bool isCollIncrease; //1 uint debt; //32 uint coll; //32 uint oldICR; //32 uint newICR; //32 uint newTCR;//32 uint LUSDFee; //32 uint newDebt; //32 uint newColl; //32 uint stake; //32 }
Currently total 16 slots
collCCR,collMCR, LUSDFee we can declare uint128 instead of uint266. The collCCR,collMCR always within the particular range. Once set values not increased as per documentations. Only possible to decrease the collCCR,collMCR values so for those situations uint128 is alone enough. There is no chance to overflow .
LUSDfee also each trove has the debt limit. The fee percent is very less which is always between 1-2%from overall debt. So uint128 is more than enough to store LUSDfee
/// @audit Variable ordering with 14 slots instead of the current 16: struct LocalVariables_adjustTrove { uint256 collDecimals; //32 uint price; //32 uint collChange; //32 uint netDebtChange; //32 uint debt; //32 uint coll; //32 uint oldICR; //32 uint newICR; //32 uint newTCR;//32 uint newDebt; //32 uint newColl; //32 uint stake; //32 uint128 collCCR; //16 uint128 collMCR; //16 uint128 LUSDFee; //16 bool isCollIncrease; //1 }
public/external function names and public member variable names can be optimized to save gas. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
Instances (7) :
File : 2023-02-ethos/Ethos-Core/contracts/CollateralConfig.sol
/// @Audit updateCollateralRatios() 15: contract CollateralConfig is ICollateralConfig, CheckContract, Ownable {
File: 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
/// @Audit
setAddresses(),setYieldingPercentage(),setYieldingPercentageDrift(),setYieldClaimThreshold(),setYieldDistributionParams(),manualRebalance(), 26 : contract ActivePool is Ownable, CheckContract, IActivePool {
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
/// @Audit fund(),updateDistributionPeriod() 14: contract CommunityIssuance is ICommunityIssuance, Ownable, CheckContract, BaseMath {
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
/// @Audit pauseMinting(),unpauseMinting(),updateGovernance(),updateGuardian(),upgradeProtocol(), 28 : contract LUSDToken is CheckContract, ILUSDToken {
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
/// @Audit addStrategy(),updateStrategyFeeBPS(),updateStrategyAllocBPS(),revokeStrategy(),availableCapital(),setWithdrawalQueue(),balance(),getPricePerFullShare(),depositAll(),deposit(),withdrawAll(),withdraw(),report(),updateWithdrawMaxLoss(),updateTvlCap(),removeTvlCap(),setEmergencyShutdown(),setLockedProfitDegradation(),updateTreasury(),inCaseTokensGetStuck(), 21: contract ReaperVaultV2 is ReaperAccessControl, ERC20, IERC4626Events, AccessControlEnumerable, ReentrancyGuard {
File : 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
/// @Audit setEmergencyExit(),initiateUpgradeCooldown(),clearUpgradeCooldown(), 14: abstract contract ReaperBaseStrategyv4 is
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
/// @Audit setHarvestSteps(),authorizedWithdrawUnderlying(),balanceOfWant(),balanceOfPool() 19: contract ReaperStrategyGranarySupplyOnly is ReaperBaseStrategyv4, VeloSolidMixin {
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations
File : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
86 : mapping (address => mapping (address => Trove)) public Troves; 88 : mapping (address => uint) public totalStakes; 91 : mapping (address => uint) public totalStakesSnapshot; 94: mapping (address => uint) public totalCollateralSnapshot; 104: mapping (address => uint) public L_Collateral; 105: mapping (address => uint) public L_LUSDDebt; 109 : mapping (address => mapping (address => RewardSnapshot)) public rewardSnapshots; 116: mapping (address => address[]) public TroveOwners; 119 : mapping (address => uint) public lastCollateralError_Redistribution; 120: mapping (address => uint) public lastLUSDDebtError_Redistribution;
File : File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
54 : mapping (address => uint256) private _nonces; 57: mapping (address => uint256) private _balances; 58: mapping (address => mapping (address => uint256)) private _allowances; 62: mapping (address => bool) public troveManagers; 63: mapping (address => bool) public stabilityPools; 64: mapping (address => bool) public borrowerOperations;
File : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
41: mapping (address => uint256) internal collAmount; // collateral => amount tracker 42: mapping (address => uint256) internal LUSDDebt; // collateral => corresponding debt tracker 44: mapping (address => uint256) public yieldingPercentage; // collateral => % to use for yield farming (in BPS, <= 10k) 45: mapping (address => uint256) public yieldingAmount; // collateral => actual wei amount being used for yield farming 46: mapping (address => address) public yieldGenerator; // collateral => corresponding ERC4626 vault 47: mapping (address => uint256) public yieldClaimThreshold; // collateral => minimum wei amount of yield to claim and redistribute
File : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
167: mapping (address => uint256) internal collAmounts; // deposited collateral tracker 186: mapping (address => Deposit) public deposits; // depositor address -> Deposit struct 187: mapping (address => Snapshots) public depositSnapshots; // depositor address -> snapshots struct 214: mapping (uint128 => mapping(uint128 => mapping (address => uint))) public epochToScaleToSum; 223: mapping (uint128 => mapping(uint128 => uint)) public epochToScaleToG; 228: mapping (address => uint) public lastCollateralError_Offset;
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
25: mapping( address => uint) public stakes; 28: mapping (address => uint) public F_Collateral; 32: mapping (address => Snapshot) public snapshots;
Instances (12) :
CONTEXT: ALL CONTRACTS
Using the latest version of a smart contract can help you reduce gas costs because the latest version is usually optimized for efficiency and can use fewer gas fees to execute the same function compared to an older version. In addition, the latest version may include new features that can further optimize gas usage, such as the use of newer algorithms or data structures.
Use a Solidity version of at least 0.8.2 to get simple compiler automatic inlining.
Use a Solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads.
Use a Solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings.
Use a Solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.
File : CollateralConfig.sol
3 : pragma solidity 0.6.11;
File: 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
3: pragma solidity 0.6.11;
File : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
3: pragma solidity 0.6.11;
File: 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
3: pragma solidity 0.6.11;
File : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
3: pragma solidity 0.6.11;
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
3: pragma solidity 0.6.11;
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
3: pragma solidity 0.6.11;
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
3: pragma solidity ^0.8.0;
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
3: pragma solidity ^0.8.0;
File : 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
3: pragma solidity ^0.8.0;
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
3: pragma solidity ^0.8.0;
In Solidity, declaring a variable inside a loop can result in higher gas costs compared to declaring it outside the loop. This is because every time the loop runs, a new instance of the variable is created, which can lead to unnecessary memory allocation and increased gas costs
On the other hand, if you declare a variable outside the loop, the variable is only initialized once, and you can reuse the same instance of the variable inside the loop. This approach can save gas and optimize the execution of your code
Instances (10) :
Approximate Gas Saved : 90 gas
GAS SAMPLE TEST IN remix ide(Without gas optimizations) :
Before Mitigation :
function testGas() public view { for(uint256 i = 0; i < 10; i++) { address collateral = msg.sender; address collateral1 = msg.sender; }
The execution Cost : 2176
After Mitigation :
function testGas() public view { address collateral; address collateral1; for(uint256 i = 0; i < 10; i++) { collateral = msg.sender; collateral1 = msg.sender; }
The execution Cost : 2086 .
Hence clearly after mitigation the gas cost is reduced. Approximately its possible to save 9 gas for every iterations
File : CollateralConfig.sol
for(uint256 i = 0; i < _collaterals.length; i++) { address collateral = _collaterals[i]; //@Audit Declared inside the loop. This should be avoided . Should declare outside the loop checkContract(collateral); collaterals.push(collateral); Config storage config = collateralConfig[collateral]; //@Audit Declared inside the loop. This should be avoided . Should declare outside the loop config.allowed = true; uint256 decimals = IERC20(collateral).decimals(); config.decimals = decimals; require(_MCRs[i] >= MIN_ALLOWED_MCR, "MCR below allowed minimum"); config.MCR = _MCRs[i]; require(_CCRs[i] >= MIN_ALLOWED_CCR, "CCR below allowed minimum"); config.CCR = _CCRs[i]; emit CollateralWhitelisted(collateral, decimals, _MCRs[i], _CCRs[i]); }
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
for (vars.i = 0; vars.i < _n && vars.user != firstUser; vars.i++) { // we need to cache it, because current user is likely going to be deleted address nextUser = _sortedTroves.getPrev(_collateral, vars.user);
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
for (uint256 i = 0; i < queueLength; i = i.uncheckedInc()) { address strategy = _withdrawalQueue[i]; StrategyParams storage params = strategies[strategy];
Functions can have local variables. When a modifier is used, all of the variables used in the modifier are stored in memory for the entire duration of the function call, even if they are not used after the modifier. This can result in unnecessary gas costs. With a function, you can declare local variables that are only stored in memory for the duration of the function call, which can save on gas costs
Modifier can make it harder to optimize the code for gas efficiency. Functions, on the other hand, are more straightforward and easier to optimize for gas
File : CollateralConfig.sol
modifier checkCollateral(address _collateral) { require(collateralConfig[_collateral].allowed, "Invalid collateral address"); _; }
This modifier can be replaced with function like below :
function checkCollateral(address _collateral) private view returns(bool) { return collateralConfig[_collateral].allowed ;
}
If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity.
bytes32 is a fixed-size data type, meaning that its size is known at compile time and cannot change during runtime. This allows Solidity to allocate the exact amount of memory needed for a bytes32 variable, which can be more efficient than allocating memory dynamically for a string variable
Because string variables are dynamically-sized and can grow or shrink during runtime, performing operations on them can be more expensive in terms of gas costs than operations on fixed-size data types like bytes32.
Instances (8) :
PROOF OF CONCEPT :
IDE USED REMIX (Without Optimization) :
USING STRING :
pragma solidity ^0.8.17;
contract StorageTest {
string constant public NAME = "BorrowerOperations";
}
Remix Reports :
gas : 152195 gas transaction cost : 132343 gas execution cost : 73523 gas
USING BYTES32 :
pragma solidity ^0.8.17;
contract StorageTest {
bytes32 constant public NAME = bytes32("BorrowerOperations");
}
gas : 113157 gas transaction cost : 98397 gas execution cost : 41893 gas
GAS SAVED :
gas : 39038 transaction cost : 33946 execution cost : 31630
File : BorrowerOperations.sol
21 : string constant public NAME = "BorrowerOperations";
FILE : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
30 : string constant public NAME = "ActivePool";
FILE : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
150 : string constant public NAME = "StabilityPool";
FILE : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
19: string constant public NAME = "CommunityIssuance";
FILE : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
23 : string constant public NAME = "LQTYStaking";
FILE : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
32 : string constant internal _NAME = "LUSD Stablecoin"; 33: string constant internal _SYMBOL = "LUSD"; 34: string constant internal _VERSION = "1";
When a require statement fails, any gas spent after the failing condition is refunded to the caller. This can help prevent unnecessary gas usage and make the contract more efficient. On the other hand, when an assert statement fails, all remaining gas is consumed and cannot be refunded, which can be wasteful and lead to more expensive transactions
The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.
Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.
Instances (12)
File : BorrowerOperations.sol
128: assert(MIN_NET_DEBT > 0);
197: assert(vars.compositeDebt > 0);
331: assert(_collWithdrawal <= vars.coll);
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
1281 : assert(closedStatus != Status.nonExistent && closedStatus != Status.active);
1342 : assert(troveStatus != Status.nonExistent && troveStatus != Status.active); 1348 : assert(index <= idxLast);
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
312: assert(sender != address(0)); 313: assert(recipient != address(0));
321: assert(account != address(0));
329: assert(account != address(0));
337: assert(owner != address(0)); 338: assert(spender != address(0));
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
Instances (11)
File : BorrowerOperations.sol
175 : ContractsCache memory contractsCache = ContractsCache(troveManager, activePool, lusdToken);
283 : ContractsCache memory contractsCache = ContractsCache(troveManager, activePool, lusdToken);
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
313 : address[] memory borrowers = new address[](1);
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
166: address[2] memory step = _newSteps[i];
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
660 : bytes32[] memory cascadingAccessRoles = new bytes32[](5);
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
166: address[2] memory step = _newSteps[i];
File : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
687: Snapshots memory snapshots = depositSnapshots[_depositor];
722: Snapshots memory snapshots = depositSnapshots[_depositor];
806: address[] memory collaterals = collateralConfig.getAllowedCollaterals(); 807: uint[] memory amounts = new uint[](collaterals.length);
External functions do not require a context switch to the EVM, while public functions do.
Its possible to save 10-15 gas using external instead public for every function call
Instances (1) :
Approximate Saved Gas : 10-15 gas
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
1045 : function getNominalICR(address _borrower, address _collateral) public view override returns (uint) {
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
Instances (35) :
Approximate Saved Gas : 1050
File : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
function _getCollChange( uint _collReceived, uint _requestedCollWithdrawal ) internal pure returns(uint collChange, bool isCollIncrease)
524: function _requireValidCollateralAddress(address _collateral) internal view {
533: function _requireSingularCollChange(uint _collTopUp, uint _collWithdrawal) internal pure {
537: function _requireNonZeroAdjustment(uint _collTopUp, uint _collWithdrawal, uint _LUSDChange) internal pure {
546 : function _requireTroveisNotActive(ITroveManager _troveManager, address _borrower, address _collateral) internal view {
551: function _requireNonZeroDebtChange(uint _LUSDChange) internal pure {
640 : function _requireCallerIsStabilityPool() internal view {
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
320: function _mint(address account, uint256 amount) internal {
328: function _burn(address account, uint256 amount) internal {
360 : function _requireCallerIsBorrowerOperations() internal view {
365: function _requireCallerIsBOorTroveMorSP() internal view {
375: function _requireCallerIsStabilityPool() internal view {
380: function _requireCallerIsTroveMorSP() internal view {
393: function _requireMintingNotPaused() internal view {
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
function _liquidatePosition(uint256 _amountNeeded) internal override returns (uint256 liquidatedAmount, uint256 loss)
176: function _deposit(uint256 toReinvest) internal {
206: function _claimRewards() internal {
Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 30 GAS per &&
Instances (4)
Approximate Saved Gas : 120 gas
PROOF OF WORK (remix without optimizations) :
(https://github.com/sathishpic22/AllGASTestings/blob/main/Require%26%26splitTest)
pragma solidity ^0.8.7; contract MappingTest {
int a=10;
// Execution Cost 2398 function requireTest()public view { require(a==10 && a<11,"wrong inputs"); } // Execution Cost 2368 function requireTest1()public view { require(a==10,"wrong inputs"); require(a<11 ,"wrong inputs"); }
}
So when splitting require then save 30 gas
File : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= DECIMAL_PRECISION, "Max fee percentage must be between 0.5% and 100%");
File : File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
require( _recipient != address(0) && _recipient != address(this), "LUSD: Cannot transfer tokens directly to the LUSD token contract or the zero address" ); require( !stabilityPools[_recipient] && !troveManagers[_recipient] && !borrowerOperations[_recipient], "LUSD: Cannot transfer tokens directly to the StabilityPool, TroveManager or BorrowerOps" );
Instances (13) :
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
function _liquidateNormalMode( IActivePool _activePool, IDefaultPool _defaultPool, address _collateral, address _borrower, uint _LUSDInStabPool ) internal returns (LiquidationValues memory singleLiquidation)
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
function _liquidateAllPositions() internal override returns (uint256 amountFreed) { _withdrawUnderlying(balanceOfPool()); return balanceOfWant(); }
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
function asset() external view override returns (address assetTokenAddress) { return address(token); }
function totalAssets() external view override returns (uint256 totalManagedAssets) { return balance(); }
function convertToShares(uint256 assets) public view override returns (uint256 shares) { if (totalSupply() == 0 || _freeFunds() == 0) return assets; return (assets * totalSupply()) / _freeFunds(); }
66 : function convertToAssets(uint256 shares) public view override returns (uint256 assets) {
79 : function maxDeposit(address) external view override returns (uint256 maxAssets) {
function previewDeposit(uint256 assets) external view override returns (uint256 shares) { return convertToShares(assets); }
122 : function maxMint(address) external view override returns (uint256 maxShares) {
165 : function maxWithdraw(address owner) external view override returns (uint256 maxAssets) {
220 : function maxRedeem(address owner) external view override returns (uint256 maxShares) {
240 : function previewRedeem(uint256 shares) external view override returns (uint256 assets) {
The unchecked keyword was introduced in Solidity version 0.8.0.
Instances (8) :
Approximate Saved Gas : 1600 gas
PROOF OF CONCEPT (remix without optimizations):
pragma solidity ^0.8.7; contract MappingTest {
// Execution Cost 696 function Subtraction() public pure { int a=20; int b=10; int c=10; int d=30; if(a>b){ a=a-b; } if(c<d){ c=d-c; }
} //Execution Cost 276 function uncheckedSubtraction() public pure { int a=20; int b=10; int c=10; int d=30; if(a>b){ unchecked{a=a-b;}
} if(c<d){ unchecked{c=d-c;} }
}
}
So clearly for each unchecked possible to save 210 gas
SOLUTION:
require(a < b); x = b - a => require(a <b); unchecked { x = b - a }
require(a > b); x = a - b => require(a > b); unchecked { x = a - b }
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
80: if (wantBalance > _debt) { 81: uint256 toReinvest = wantBalance - _debt;
92: if (wantBal < _amountNeeded) { 93: _withdraw(_amountNeeded - wantBal);
99: if (_amountNeeded > liquidatedAmount) { 100: loss = _amountNeeded - liquidatedAmount;
131: if (totalAssets > allocated) { 132: uint256 profit = totalAssets - allocated;
135: } else if (totalAssets < allocated) { 136: roi = -int256(allocated - totalAssets);
File : 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
if (amountFreed < debt) { roi = -int256(debt - amountFreed); } else if (amountFreed > debt) { roi = int256(amountFreed - debt);
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
234: if (stratCurrentAllocation > stratMaxAllocation) { 235: return -int256(stratCurrentAllocation - stratMaxAllocation);
330 : _amount = balAfter - balBefore;
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size
(https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html)
Each operation involving a uint128 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint128, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
Instances (3) :
Approximate Saved Gas : 84 gas
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
1329 : index = uint128(TroveOwners[_collateral].length.sub(1)); 1344 : uint128 index = Troves[_borrower][_collateral].arrayIndex;
File : File : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
745: uint128 scaleDiff = currentScale.sub(scaleSnapshot);
block.timestamp are added to event information by default so adding them manually wastes gas
Instances (1)
FILE : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
1505 : emit LastFeeOpTimeUpdated(block.timestamp);
Using the addition/Subtraction operator instead of plus-equals/minus-equals saves 113 gas
Instances (14)
Approximate Saved Gas : 1500 gas
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
168 : totalAllocBPS += _allocBPS;
194: totalAllocBPS -= strategies[_strategy].allocBPS;
195: totalAllocBPS += _allocBPS;
214: totalAllocBPS -= strategies[_strategy].allocBPS;
395: strategies[stratAddr].allocated -= actualWithdrawn; 396: totalAllocated -= actualWithdrawn;
444: stratParams.allocBPS -= bpsChange; 445: totalAllocBPS -= bpsChange;
450 : stratParams.losses += loss; 451: stratParams.allocated -= loss; 452: totalAllocated -= loss;
505 : strategy.gains += vars.gain;
514: strategy.allocated -= vars.debtPayment; 515: totalAllocated -= vars.debtPayment;
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
function initialize( address _vault, address[] memory _strategists, address[] memory _multisigRoles, IAToken _gWant ) public initializer {
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations. Caching an array’s struct avoids recalculating the array offsets into memory/calldata
Instances (1)
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
strategies[_strategy].allocBPS use local variable cache instead calling multiple times
210 : if (strategies[_strategy].allocBPS == 0) {
214: totalAllocBPS -= strategies[_strategy].allocBPS;
215: strategies[_strategy].allocBPS = 0;
Saves ~13 gas per instance
Instances (6) :
Approximate Saved Gas : 78
File : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
225: constructor() public {
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
57: constructor() public {
File : 2023-02-ethos/Ethos-Core/contracts/LUSDToken.sol
84: constructor(
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
111 : constructor(
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
16: constructor(
File : 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
61 : constructor() initializer {}
Before transfer, we should check for amount being 0 so the function doesn't run when its not gonna do anything
Instances (7)
File :2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
_OathAmount is not checked for nonzero values before transfer()
125: OathToken.transfer(_account, _OathAmount);
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
LQTYToWithdraw is not checked for nonzero values before safeTransfer()
LUSDGain is not checked for nonzero values before transfer()
amounts[i] is not checked for nonzero values before safeTransfer()
File : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
_amount is not checked for nonzero values before calling functions
File : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
_collChange is not checked for nonzero values before calling safeTransferFrom()
497: IERC20(_collateral).safeTransferFrom(msg.sender, address(this), _collChange);
It’s cheaper to use msg.sender as compared to caching
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
USE msg.sender INSTEAD OF CACHING WITH stratAddr VARIABLE
function availableCapital() public view returns (int256) { address stratAddr = msg.sender; // @Audit msg.sender if (totalAllocBPS == 0 || emergencyShutdown) { return -int256(strategies[stratAddr].allocated); //@AUDIT USE msg.sender } uint256 stratMaxAllocation = (strategies[stratAddr].allocBPS * balance()) / PERCENT_DIVISOR; //@AUDIT USE msg.sender uint256 stratCurrentAllocation = strategies[stratAddr].allocated; //@AUDIT USE msg.sender
USE msg.sender INSTEAD OF CACHING WITH vars.stratAddr VARIABLE
495 : vars.stratAddr = msg.sender; 496: StrategyParams storage strategy = strategies[vars.stratAddr]; 501 : _reportLoss(vars.stratAddr, vars.loss); 504 : vars.fees = _chargeFees(vars.stratAddr, vars.gain); 526 : token.safeTransfer(vars.stratAddr, vars.credit - vars.freeWantInStrat); 528 : token.safeTransferFrom(vars.stratAddr, address(this), vars.freeWantInStrat - vars.credit); 544: vars.stratAddr, 556: return IStrategy(vars.stratAddr).balanceOf();
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
150: require(!emergencyShutdown, "Cannot add strategy during emergency shutdown"); 151: require(_strategy != address(0), "Invalid strategy address"); 152: require(strategies[_strategy].activation == 0, "Strategy already added"); 153: require(address(this) == IStrategy(_strategy).vault(), "Strategy's vault does not match"); 154: require(address(token) == IStrategy(_strategy).want(), "Strategy's want does not match"); 155: require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS"); 156: require(_allocBPS + totalAllocBPS <= PERCENT_DIVISOR, "Invalid allocBPS value");
Its cheaper to check for _strategy != address(0), _feeBPS <= PERCENT_DIVISOR / 5,_allocBPS + totalAllocBPS <= PERCENT_DIVISOR as compared to !emergencyShutdown,strategies[_strategy].activation == 0,address(this) == IStrategy(_strategy).vault(),address(token) == IStrategy(_strategy).want() as this involves reading the storage variable. Therefore if the require(_strategy != address(0), "Invalid strategy address"); fails it would be cheaper to fail before evaluating the !emergencyShutdown
First check local variable condition checks then move to state variable condition checks .
+151: require(_strategy != address(0), "Invalid strategy address"); +155: require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS"); +156: require(_allocBPS + totalAllocBPS <= PERCENT_DIVISOR, "Invalid allocBPS value"); 150: require(!emergencyShutdown, "Cannot add strategy during emergency shutdown"); -151: require(_strategy != address(0), "Invalid strategy address"); 152: require(strategies[_strategy].activation == 0, "Strategy already added"); 153: require(address(this) == IStrategy(_strategy).vault(), "Strategy's vault does not match"); 154: require(address(token) == IStrategy(_strategy).want(), "Strategy's want does not match"); -155: require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS"); -156: require(_allocBPS + totalAllocBPS <= PERCENT_DIVISOR, "Invalid allocBPS value");
CHECK _feeBPS <= PERCENT_DIVISOR / 5 CONDITION FIRST THEN strategies[_strategy].activation != 0
180: require(strategies[_strategy].activation != 0, "Invalid strategy address"); 181: require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS");
This saves deployment gas
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
!emergencyShutdown, _strategy].activation != 0, _feeBPS <= PERCENT_DIVISOR / 5, strategies[_strategy].activation != 0 can be REFACTORED TO A MODIFIER OR FUNCTION
150: require(!emergencyShutdown, "Cannot add strategy during emergency shutdown"); 321: require(!emergencyShutdown, "Cannot deposit during emergency shutdown"); 180: require(strategies[_strategy].activation != 0, "Invalid strategy address"); 193: require(strategies[_strategy].activation != 0, "Invalid strategy address");
155: require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS"); 181: require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS");
Instances (8) :
Using immutable variables instead of constant variables for expressions that evaluate to constant values, such as a call to keccak256(), can potentially save gas in smart contracts
The reason for this is that immutable variables are evaluated at compile time and the resulting values are hardcoded into the bytecode of the contract. This means that the gas cost of evaluating the expression is incurred only once, during contract compilation, and not at runtime
In contrast, constant variables are evaluated at runtime, which can result in additional gas costs. Specifically, when a constant variable is accessed, the EVM must perform a lookup to retrieve the value of the variable from storage, which can add to the gas cost of executing the contract
File: 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
49: bytes32 public constant KEEPER = keccak256("KEEPER"); 50: bytes32 public constant STRATEGIST = keccak256("STRATEGIST"); 51: bytes32 public constant GUARDIAN = keccak256("GUARDIAN"); 52: bytes32 public constant ADMIN = keccak256("ADMIN");
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
73: bytes32 public constant DEPOSITOR = keccak256("DEPOSITOR"); 74: bytes32 public constant STRATEGIST = keccak256("STRATEGIST"); 75: bytes32 public constant GUARDIAN = keccak256("GUARDIAN"); 76: bytes32 public constant ADMIN = keccak256("ADMIN");
Make sure Solidity’s optimizer is enabled. It reduces gas costs. If you want to gas optimize for contract deployment (costs less to deploy a contract) then set the Solidity optimizer at a low number. If you want to optimize for run-time gas costs (when functions are called on a contract) then set the optimizer to a high number.
Set the optimization value higher than 800 in your hardhat.config.ts file
FILE : 2023-02-ethos/Ethos-Core/hardhat.config.js
version: "0.4.23", settings: { optimizer: { enabled: true, runs: 100 } } }, { version: "0.5.17", settings: { optimizer: { enabled: true, runs: 100 } } }, { version: "0.6.11", settings: { optimizer: { enabled: true, runs: 100 } } }, ]
Instances(49):
File : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
borrowerOperationsAddress = _borrowerOperationsAddress; collateralConfig = ICollateralConfig(_collateralConfigAddress); activePool = IActivePool(_activePoolAddress); defaultPool = IDefaultPool(_defaultPoolAddress); stabilityPool = IStabilityPool(_stabilityPoolAddress); gasPoolAddress = _gasPoolAddress; collSurplusPool = ICollSurplusPool(_collSurplusPoolAddress); priceFeed = IPriceFeed(_priceFeedAddress); lusdToken = ILUSDToken(_lusdTokenAddress); sortedTroves = ISortedTroves(_sortedTrovesAddress); lqtyToken = IERC20(_lqtyTokenAddress); lqtyStaking = ILQTYStaking(_lqtyStakingAddress); redemptionHelper = IRedemptionHelper(_redemptionHelperAddress);
File : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
collateralConfig = ICollateralConfig(_collateralConfigAddress); troveManager = ITroveManager(_troveManagerAddress); activePool = IActivePool(_activePoolAddress); defaultPool = IDefaultPool(_defaultPoolAddress); stabilityPoolAddress = _stabilityPoolAddress; gasPoolAddress = _gasPoolAddress; collSurplusPool = ICollSurplusPool(_collSurplusPoolAddress); priceFeed = IPriceFeed(_priceFeedAddress); sortedTroves = ISortedTroves(_sortedTrovesAddress); lusdToken = ILUSDToken(_lusdTokenAddress); lqtyStakingAddress = _lqtyStakingAddress; lqtyStaking = ILQTYStaking(_lqtyStakingAddress);
File : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
collateralConfigAddress = _collateralConfigAddress; borrowerOperationsAddress = _borrowerOperationsAddress; troveManagerAddress = _troveManagerAddress; stabilityPoolAddress = _stabilityPoolAddress; defaultPoolAddress = _defaultPoolAddress; collSurplusPoolAddress = _collSurplusPoolAddress; treasuryAddress = _treasuryAddress; lqtyStakingAddress = _lqtyStakingAddress;
File : 2023-02-ethos/Ethos-Core/contracts/StabilityPool.sol
borrowerOperations = IBorrowerOperations(_borrowerOperationsAddress); collateralConfig = ICollateralConfig(_collateralConfigAddress); troveManager = ITroveManager(_troveManagerAddress); activePool = IActivePool(_activePoolAddress); lusdToken = ILUSDToken(_lusdTokenAddress); sortedTroves = ISortedTroves(_sortedTrovesAddress); priceFeed = IPriceFeed(_priceFeedAddress); communityIssuance = ICommunityIssuance(_communityIssuanceAddress);
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/CommunityIssuance.sol
OathToken = IERC20(_oathTokenAddress); stabilityPoolAddress = _stabilityPoolAddress;
File : 2023-02-ethos/Ethos-Core/contracts/LQTY/LQTYStaking.sol
lqtyToken = IERC20(_lqtyTokenAddress); lusdToken = ILUSDToken(_lusdTokenAddress); troveManagerAddress = _troveManagerAddress; borrowerOperationsAddress = _borrowerOperationsAddress; activePoolAddress = _activePoolAddress; collateralConfig = ICollateralConfig(_collateralConfigAddress);
Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports
Instances (5) :
File : 2023-02-ethos/Ethos-Core/contracts/TroveManager.sol
616 : if (vars.ICR >= vars.collMCR && vars.remainingLUSDInStabPool == 0) { break; }
817: if (vars.ICR >= vars.collMCR && vars.remainingLUSDInStabPool == 0) { continue; }
FILE : 2023-02-ethos/Ethos-Core/contracts/BorrowerOperations.sol
311: if (_isDebtIncrease && !isRecoveryMode) {
337: if (!_isDebtIncrease && _LUSDChange > 0) {
FILE : 2023-02-ethos/Ethos-Core/contracts/ActivePool.sol
264: if (vars.percentOfFinalBal > vars.yieldingPercentage && vars.percentOfFinalBal.sub(vars.yieldingPercentage) > yieldingPercentageDrift) {
Recommendation Code :
- if (vars.percentOfFinalBal > vars.yieldingPercentage && vars.percentOfFinalBal.sub(vars.yieldingPercentage) >yieldingPercentageDrift) { + if (vars.percentOfFinalBal > vars.yieldingPercentage ) + if (vars.percentOfFinalBal.sub(vars.yieldingPercentage) > yieldingPercentageDrift)
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultERC4626.sol
273: if (x % y != 0) q++;
File: 2023-02-ethos/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol
49: bytes32 public constant KEEPER = keccak256("KEEPER"); 50: bytes32 public constant STRATEGIST = keccak256("STRATEGIST"); 51: bytes32 public constant GUARDIAN = keccak256("GUARDIAN"); 52: bytes32 public constant ADMIN = keccak256("ADMIN");
File : 2023-02-ethos/Ethos-Vault/contracts/ReaperVaultV2.sol
73: bytes32 public constant DEPOSITOR = keccak256("DEPOSITOR"); 74: bytes32 public constant STRATEGIST = keccak256("STRATEGIST"); 75: bytes32 public constant GUARDIAN = keccak256("GUARDIAN"); 76: bytes32 public constant ADMIN = keccak256("ADMIN");
#0 - c4-judge
2023-03-09T17:38:00Z
trust1995 marked the issue as grade-a