Platform: Code4rena
Start Date: 21/12/2023
Pot Size: $90,500 USDC
Total HM: 10
Participants: 39
Period: 18 days
Judge: LSDan
Total Solo HM: 5
Id: 315
League: ETH
Rank: 10/39
Findings: 1
Award: $573.29
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0xAnah
Also found by: 0x11singh99, 0xA5DF, IllIllI, JCK, Raihan, Sathish9098, alphacipher, c3phas, hunter_w3b, naman1778
573.2935 USDC - $573.29
Highlighted below are optimizations exclusively targeting state-mutating functions and view/pure functions invoked by state-mutating functions. In the discussion that follows, only runtime gas is emphasized, given its inevitable dominance over deployment gas costs throughout the protocol's lifetime.
Please be aware that some code snippets may be shortened to conserve space, and certain code snippets may include @audit tags in comments to facilitate issue explanations."
mapUnits[unitId].unitHash member to memoryIn the getDependencies() function a complete storage Unit struct is copied into a memory Uint struct on Line 163 but only the dependencies member of the storage Unit struct is needed in the function so rather than coping the complete struct from storage we could access and cache the specfic member of the storage Unit struct directly. The diff below shows how the code should be refactored:
file: registries/contracts/UnitRegistry.sol 160: function getDependencies(uint256 unitId) external view virtual 161: returns (uint256 numDependencies, uint32[] memory dependencies) 162: { 163: Unit memory unit = mapUnits[unitId]; 164: return (unit.dependencies.length, unit.dependencies); 165: }
diff --git a/registries/contracts/UnitRegistry.sol b/registries/contracts/UnitRegistry.sol index 80b3235..9e7d8ba 100644 --- a/registries/contracts/UnitRegistry.sol +++ b/registries/contracts/UnitRegistry.sol @@ -160,8 +160,8 @@ abstract contract UnitRegistry is GenericRegistry { function getDependencies(uint256 unitId) external view virtual returns (uint256 numDependencies, uint32[] memory dependencies) { - Unit memory unit = mapUnits[unitId]; - return (unit.dependencies.length, unit.dependencies); + dependencies = mapUnits[unitId].dependencies; + numDependencies = dependencies.length; } /// @dev Gets updated unit hashes.
Getters for public state variables are automatically generated by the solidity compiler so there is no need to code them manually as this increases deployment cost.
mapUnits mapping variable private or internal since a getter function was defined for it.The solidity compiler would automatically create a getter function for the mapUnits mapping since it is declared as a public variable but a getter function getUnit() was also declared in the contract for the same variable thereby making it two getter functions for the same variable in the contract. We could rectify this issue by making the mapUnits variable private or internal (if the variable is to be inherited). The diff below shows how the code could be refactored:
file: registries/contracts/UnitRegistry.sol 33: mapping(uint256 => Unit) public mapUnits; //@audit make private or internal . . . 152: function getUnit(uint256 unitId) external view virtual returns (Unit memory unit) { 153: unit = mapUnits[unitId]; 154: }
diff --git a/registries/contracts/UnitRegistry.sol b/registries/contracts/UnitRegistry.sol index 80b3235..afb54b9 100644 --- a/registries/contracts/UnitRegistry.sol +++ b/registries/contracts/UnitRegistry.sol @@ -30,7 +30,7 @@ abstract contract UnitRegistry is GenericRegistry { // Map of unit Id => set of subcomponents (possible to derive from any registry) mapping(uint256 => uint32[]) public mapSubComponents; // Map of unit Id => unit - mapping(uint256 => Unit) public mapUnits; + mapping(uint256 => Unit) internal mapUnits; constructor(UnitType _unitType) { unitType = _unitType;
mapBlacklistedDonators mapping variable private or internal since a getter function was defined for it.The solidity compiler would automatically create a getter function for the mapBlacklistedDonators mapping since it is declared as a public variable but a getter function isDonatorBlacklisted() was also declared in the contract for the same variable thereby making it two getter functions for the same variable in the contract. We could rectify this issue by making the mapBlacklistedDonators variable private or internal (if the variable is to be inherited). The diff below shows how the code could be refactored:
file: tokenomics/contracts/DonatorBlacklist.sol 27: mapping(address => bool) public mapBlacklistedDonators; //@audit make private or internal . . . 82: function isDonatorBlacklisted(address account) external view returns (bool status) { 83: status = mapBlacklistedDonators[account]; 84: }
diff --git a/tokenomics/contracts/DonatorBlacklist.sol b/tokenomics/contracts/DonatorBlacklist.sol index 00d84f4..f173321 100644 --- a/tokenomics/contracts/DonatorBlacklist.sol +++ b/tokenomics/contracts/DonatorBlacklist.sol @@ -24,7 +24,7 @@ contract DonatorBlacklist { // Owner address address public owner; // Mapping account address => blacklisting status - mapping(address => bool) public mapBlacklistedDonators; + mapping(address => bool) internal mapBlacklistedDonators; /// @dev DonatorBlacklist constructor. constructor() {
There are some checks to avoid an underflow, but in some scenarios where it is impossible for underflow to occur we can use unchecked blocks to have some gas savings.
uint256 numYears = (block.timestamp - timeLaunch) / oneYear can be uncheckedIn the inflationRemainder() function it is not possible that the computation block.timestamp - timeLaunch statement would underflow as the value of block.timestamp would always be greater than that of timeLaunch so we can perform the uint256 numYears = (block.timestamp - timeLaunch) / oneYear statement in an unchecked {} block as this would save some gas from the unnecessary internal underflow checks. The code could be recfatored as shown in the diff below.
file: governance/contracts/OLAS.sol 98: function inflationRemainder() public view returns (uint256 remainder) { 99: uint256 _totalSupply = totalSupply; 100: // Current year 101: uint256 numYears = (block.timestamp - timeLaunch) / oneYear; //@audit can be unchecked 102: // Calculate maximum mint amount to date 103: uint256 supplyCap = tenYearSupplyCap; 104: // After 10 years, adjust supplyCap according to the yearly inflation % set in maxMintCapFraction 105: if (numYears > 9) { 106: // Number of years after ten years have passed (including ongoing ones) 107: numYears -= 9; 108: for (uint256 i = 0; i < numYears; ++i) { 109: supplyCap += (supplyCap * maxMintCapFraction) / 100; 110: } 111: } 112: // Check for the requested mint overflow 113: remainder = supplyCap - _totalSupply; 114: }
diff --git a/governance/contracts/OLAS.sol b/governance/contracts/OLAS.sol index a1700ed..33fc569 100644 --- a/governance/contracts/OLAS.sol +++ b/governance/contracts/OLAS.sol @@ -98,7 +98,10 @@ contract OLAS is ERC20 { function inflationRemainder() public view returns (uint256 remainder) { uint256 _totalSupply = totalSupply; // Current year - uint256 numYears = (block.timestamp - timeLaunch) / oneYear; + uint256 numYears; + unchecked { + numYears = (block.timestamp - timeLaunch) / oneYear; + } // Calculate maximum mint amount to date uint256 supplyCap = tenYearSupplyCap; // After 10 years, adjust supplyCap according to the yearly inflation % set in maxMintCapFraction
constructor of Depository contract such that all the checks on the arguments are performed before assigning the values to state variables.The constructor of Depository contract should be refactored such that all the checks on the arguments are performed before assigning the values to state variables because for example in a scenario such that the check on line 111 - Line 113 fails the function would revert but it would have assigned values to the owner state variables costing 1 SSTORE 20000 gas units. But if all the checks are performed first before assigning any value to any of state variables and a check fails the deployment would revert without performing consuming huge amount of gas thereby making the constructor better gas efficient. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Depository.sol 106: constructor(address _olas, address _tokenomics, address _treasury, address _bondCalculator) 107: { 108: owner = msg.sender; 109: 110: // Check for at least one zero contract address 111: if (_olas == address(0) || _tokenomics == address(0) || _treasury == address(0) || _bondCalculator == address(0)) { 112: revert ZeroAddress(); 113: } 114: olas = _olas; 115: tokenomics = _tokenomics; 116: treasury = _treasury; 117: bondCalculator = _bondCalculator; 118: }
diff --git a/tokenomics/contracts/Depository.sol b/tokenomics/contracts/Depository.sol index f9c7d64..9bde7d4 100644 --- a/tokenomics/contracts/Depository.sol +++ b/tokenomics/contracts/Depository.sol @@ -105,12 +105,11 @@ contract Depository is IErrorsTokenomics { /// @param _tokenomics Tokenomics address. constructor(address _olas, address _tokenomics, address _treasury, address _bondCalculator) { - owner = msg.sender; - // Check for at least one zero contract address if (_olas == address(0) || _tokenomics == address(0) || _treasury == address(0) || _bondCalculator == address(0)) { revert ZeroAddress(); } + owner = msg.sender; olas = _olas; tokenomics = _tokenomics; treasury = _treasury;
Estimated gas saved: 20000 gas units
constructor of Dispenser contract such that all the checks on the arguments are performed before assigning the values to state variables.The constructor of Dispenser contract should be refactored such that all the checks on the arguments are performed before assigning the values to state variables because for example in a scenario such that the check on line 36 - Line 38 fails the function would revert but it would have assigned values to the owner and _locked state variables costing 2 SSTORE 40000 gas units. But if all the checks are performed first before assigning any value to any of state variables and a check fails the deployment would revert without performing consuming huge amount of gas thereby making the constructor better gas efficient. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Dispenser.sol 30: constructor(address _tokenomics, address _treasury) 31: { 32: owner = msg.sender; 33: _locked = 1; 34: 35: // Check for at least one zero contract address 36: if (_tokenomics == address(0) || _treasury == address(0)) { 37: revert ZeroAddress(); 38: } 39: 40: tokenomics = _tokenomics; 41: treasury = _treasury; 42: }
Estimated gas saved: 40000 gas units
Performing STATICCALLs that do not depend on variables incremented in loops should always try to be avoided within the loop. In the following instances, we are able to cache the external calls outside of the loop to save a STATICCALL (100 gas) per loop iteration.
IVotingEscrow(ve).getVotes(donator) outside of loop.The external call IVotingEscrow(ve).getVotes(donator) should be made outside the loop and the result cached since the value it returns is not dependent on the loop iterations.
file: tokenomics/contracts/Tokenomics.sol 687: function _trackServiceDonations(address donator, uint256[] memory serviceIds, uint256[] memory amounts, uint256 curEpoch) internal { 688: // Component / agent registry addresses 689: address[] memory registries = new address[](2); 690: (registries[0], registries[1]) = (componentRegistry, agentRegistry); . . . 702: for (uint256 i = 0; i < numServices; ++i) { 703: // Check if the service owner or donator stakes enough OLAS for its components / agents to get a top-up 704: // If both component and agent owner top-up fractions are zero, there is no need to call external contract 705: // functions to check each service owner veOLAS balance 706: bool topUpEligible; 707: if (incentiveFlags[2] || incentiveFlags[3]) { 708: address serviceOwner = IToken(serviceRegistry).ownerOf(serviceIds[i]); 709: topUpEligible = (IVotingEscrow(ve).getVotes(serviceOwner) >= veOLASThreshold || 710: IVotingEscrow(ve).getVotes(donator) >= veOLASThreshold) ? true : false; //@audit cache external call `IVotingEscrow(ve).getVotes(donator)` outside of loop 711: } . . . 774: }
diff --git a/tokenomics/contracts/Tokenomics.sol b/tokenomics/contracts/Tokenomics.sol index 2ef2708..83ecf18 100644 --- a/tokenomics/contracts/Tokenomics.sol +++ b/tokenomics/contracts/Tokenomics.sol @@ -696,6 +696,8 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { incentiveFlags[2] = (mapEpochTokenomics[curEpoch].unitPoints[0].topUpUnitFraction > 0); incentiveFlags[3] = (mapEpochTokenomics[curEpoch].unitPoints[1].topUpUnitFraction > 0); + uint256 donatorVotes = IVotingEscrow(ve).getVotes(donator); + // Get the number of services uint256 numServices = serviceIds.length; // Loop over service Ids to calculate their partial contributions @@ -707,7 +709,7 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { if (incentiveFlags[2] || incentiveFlags[3]) { address serviceOwner = IToken(serviceRegistry).ownerOf(serviceIds[i]); topUpEligible = (IVotingEscrow(ve).getVotes(serviceOwner) >= veOLASThreshold || - IVotingEscrow(ve).getVotes(donator) >= veOLASThreshold) ? true : false; + donatorVotes >= veOLASThreshold) ? true : false; } // Loop over component and agent Ids
Estimated Gas saved: Above 97 gas units per loop iteration
In Solidity, performing unnecessary operations can consume more gas than needed, leading to cost inefficiencies. For instance, if a transfer function doesn't have a zero amount check and someone calls it with a zero amount, unnecessary gas will be consumed in executing the function, even though the state of the contract remains the same. By implementing a zero amount check, such unnecessary function calls can be avoided, thereby saving gas and making the contract more efficient. Amounts should be checked for 0 before calling a transfer Checking non-zero transfer values can avoid an expensive external call and save gas. I suggest adding a non-zero-value.
veOLAS.withdraw() function to avoid making zero transfersa check for if amount == 0 should be introduced to veOLAS.withdraw() function so as to prevent making unnecessary transfer that does not change state of the contract in scenarios where amount = 0. The diff below shows how `veOLAS.withdraw() function could be refactored.
file: governance/contracts/veOLAS.sol 510: function withdraw() external { 511: LockedBalance memory lockedBalance = mapLockedBalances[msg.sender]; 512: if (lockedBalance.endTime > block.timestamp) { 513: revert LockNotExpired(msg.sender, lockedBalance.endTime, block.timestamp); 514: } 515: uint256 amount = uint256(lockedBalance.amount); 516: 517: mapLockedBalances[msg.sender] = LockedBalance(0, 0); 518: uint256 supplyBefore = supply; 519: uint256 supplyAfter; 520: // The amount cannot be less than the total supply 521: unchecked { 522: supplyAfter = supplyBefore - amount; 523: supply = supplyAfter; 524: } 525: // oldLocked can have either expired <= timestamp or zero end 526: // lockedBalance has only 0 end 527: // Both can have >= 0 amount 528: _checkpoint(msg.sender, lockedBalance, LockedBalance(0, 0), uint128(supplyAfter)); 528: 529: emit Withdraw(msg.sender, amount, block.timestamp); 530: emit Supply(supplyBefore, supplyAfter); 531: 532: // OLAS is a solmate-based ERC20 token with optimized transfer() that either returns true or reverts 533: IERC20(token).transfer(msg.sender, amount); //@audit should check for amount == 0 before transfer 534: }
diff --git a/governance/contracts/veOLAS.sol b/governance/contracts/veOLAS.sol index 6f14419..4ab19b5 100644 --- a/governance/contracts/veOLAS.sol +++ b/governance/contracts/veOLAS.sol @@ -531,7 +531,10 @@ contract veOLAS is IErrors, IVotes, IERC20, IERC165 { emit Supply(supplyBefore, supplyAfter); // OLAS is a solmate-based ERC20 token with optimized transfer() that either returns true or reverts - IERC20(token).transfer(msg.sender, amount); + if (amount > 0) { + IERC20(token).transfer(msg.sender, amount); + } + } /// @dev Finds a closest point that has a specified block number.
Estimated gas saved: 100 gas units
struct Coordinates { uint256 x; uint256 y; } contract AssingningNullStruct{ mapping(address => Coordinates) public userPositions; constructor() { userPositions[msg.sender].x = 4; userPositions[msg.sender].y = 4; } function deleteUserPosition() public { userPositions[msg.sender] = Coordinates(0,0); } }
test for test/AssingningNullStruct.t.sol:AssingningNullStruct [PASS] test_DeleteUserPosition() (gas: 10293)
struct Coordinates { uint256 x; uint256 y; } contract UsingDelete{ mapping(address => Coordinates) public userPositions; constructor() { userPositions[msg.sender].x = 4; userPositions[msg.sender].y = 4; } function deleteUserPosition() public { delete userPositions[msg.sender]; } }
test for test/UsingDelete.t.sol:UsingDelete [PASS] test_DeleteUserPosition() (gas: 10218)
veOLAS.withdraw() to use delete to clear a struct mapping rather than assigning a null struct.file: governance/contracts/veOLAS.sol 510: function withdraw() external { 511: LockedBalance memory lockedBalance = mapLockedBalances[msg.sender]; 512: if (lockedBalance.endTime > block.timestamp) { 513: revert LockNotExpired(msg.sender, lockedBalance.endTime, block.timestamp); 514: } 515: uint256 amount = uint256(lockedBalance.amount); 516: 517: mapLockedBalances[msg.sender] = LockedBalance(0, 0); //@audit use delete rather than null structs 518: uint256 supplyBefore = supply; . . . 535: }
diff --git a/governance/contracts/veOLAS.sol b/governance/contracts/veOLAS.sol index 6f14419..90f9d51 100644 --- a/governance/contracts/veOLAS.sol +++ b/governance/contracts/veOLAS.sol @@ -514,7 +514,7 @@ contract veOLAS is IErrors, IVotes, IERC20, IERC165 { } uint256 amount = uint256(lockedBalance.amount); - mapLockedBalances[msg.sender] = LockedBalance(0, 0); + delete mapLockedBalances[msg.sender]; uint256 supplyBefore = supply; uint256 supplyAfter; // The amount cannot be less than the total supply
Estimated gas saved: 75 gas units saved.
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
file: governance/contracts/veOLAS.sol 593: function _balanceOfLocked(address account, uint64 ts) internal view returns (uint256 vBalance) { 594: uint256 pointNumber = mapUserPoints[account].length; 595: if (pointNumber > 0) { 596: PointVoting memory uPoint = mapUserPoints[account][pointNumber - 1]; //@audit use storage in place of memory 597: uPoint.bias -= uPoint.slope * int128(int64(ts) - int64(uPoint.ts)); 598: if (uPoint.bias > 0) { 599: vBalance = uint256(int256(uPoint.bias)); 600: } 601: } 602: }
diff --git a/governance/contracts/veOLAS.sol b/governance/contracts/veOLAS.sol index 6f14419..b811d7f 100644 --- a/governance/contracts/veOLAS.sol +++ b/governance/contracts/veOLAS.sol @@ -593,7 +593,7 @@ contract veOLAS is IErrors, IVotes, IERC20, IERC165 { function _balanceOfLocked(address account, uint64 ts) internal view returns (uint256 vBalance) { uint256 pointNumber = mapUserPoints[account].length; if (pointNumber > 0) { - PointVoting memory uPoint = mapUserPoints[account][pointNumber - 1]; + PointVoting storage uPoint = mapUserPoints[account][pointNumber - 1]; uPoint.bias -= uPoint.slope * int128(int64(ts) - int64(uPoint.ts)); if (uPoint.bias > 0) { vBalance = uint256(int256(uPoint.bias));
State variables only set in the constructor should be declared immutable as it avoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
olas state variable could be made immutableMaking the olas state variable immutable would help avoid Gsset (20000 gas units) in the constructor during deployment. In implementing this change a cheaper stack read 3 gas would be used in place of 1 SLOAD (cold access 2100 gas units) in the functions depositTokenForOLAS()and withdrawToAccount() which reads the olas variable. The diff below shows how the code should be refactored:
file: tokenomics/contracts/Treasury.sol 65: address public olas;
diff --git a/tokenomics/contracts/Treasury.sol b/tokenomics/contracts/Treasury.sol index 516c631..dfc0aae 100644 --- a/tokenomics/contracts/Treasury.sol +++ b/tokenomics/contracts/Treasury.sol @@ -62,7 +62,7 @@ contract Treasury is IErrorsTokenomics { uint96 public ETHFromServices; // OLAS token address - address public olas; + address public immutable olas; // ETH owned by treasury
Estimated Gas saved: 24200 gas units
pool state variable could be made immutableMaking the pool state variable immutable would help avoid Gsset (20000 gas units) in the constructor during deployment. In implementing this change a cheaper stack read 3 gas would be used in place of 1 SLOAD (cold access 2100 gas units) in the functions depositToke_getPositionDatanForOLAS()and withdraw() which reads the pool variable. The diff below shows how the code should be refactored:
file: lockbox-solana/solidity/liquidity_lockbox.sol 26: address public pool;
diff --git a/lockbox-solana/solidity/liquidity_lockbox.sol b/lockbox-solana/solidity/liquidity_lockbox.sol index 6760041..90e50e4 100644 --- a/lockbox-solana/solidity/liquidity_lockbox.sol +++ b/lockbox-solana/solidity/liquidity_lockbox.sol @@ -23,7 +23,7 @@ contract liquidity_lockbox { // Orca whirlpool program address address public constant orca = address"whirLbMiicVdio4qvUfM5KAg6Ct8VwpYzGff3uctyCc"; // Whirlpool (LP) pool address - address public pool; + address public immutable pool; // Current program owned PDA account address address public pdaProgram; // Bridged token mint address
Estimated Gas saved: 24200 gas units
pdaProgram state variable could be made immutableMaking the pdaProgram state variable immutable would help avoid Gsset (20000 gas units) in the constructor during deployment. In implementing this change a cheaper stack read 3 gas would be used in place of 1 SLOAD (cold access 2100 gas units) in the functions deposit()and withdraw() which reads the pdaProgram variable. The diff below shows how the code should be refactored:
file: lockbox-solana/solidity/liquidity_lockbox.sol 28: address public pdaProgram;
diff --git a/lockbox-solana/solidity/liquidity_lockbox.sol b/lockbox-solana/solidity/liquidity_lockbox.sol index 6760041..a62337b 100644 --- a/lockbox-solana/solidity/liquidity_lockbox.sol +++ b/lockbox-solana/solidity/liquidity_lockbox.sol @@ -25,7 +25,7 @@ contract liquidity_lockbox { // Whirlpool (LP) pool address address public pool; // Current program owned PDA account address - address public pdaProgram; + address public immutable pdaProgram; // Bridged token mint address address public bridgedTokenMint; // PDA bridged token account address
Estimated Gas saved: 24200 gas units
bridgedTokenMint state variable could be made immutableMaking the bridgedTokenMint state variable immutable would help avoid Gsset (20000 gas units) in the constructor during deployment. In implementing this change a cheaper stack read 3 gas would be used in place of 1 SLOAD (cold access 2100 gas units) in the functions deposit()and withdraw() which reads the bridgedTokenMint variable. The diff below shows how the code should be refactored:
30: address public bridgedTokenMint;
diff --git a/lockbox-solana/solidity/liquidity_lockbox.sol b/lockbox-solana/solidity/liquidity_lockbox.sol index 6760041..6e4b2dc 100644 --- a/lockbox-solana/solidity/liquidity_lockbox.sol +++ b/lockbox-solana/solidity/liquidity_lockbox.sol @@ -27,7 +27,7 @@ contract liquidity_lockbox { // Current program owned PDA account address address public pdaProgram; // Bridged token mint address - address public bridgedTokenMint; + address public immutable bridgedTokenMint; // PDA bridged token account address address public pdaBridgedTokenAccount; // PDA header for position account
Estimated Gas saved: 24200 gas units
pdaBridgedTokenAccount state variable could be made immutableMaking the pdaBridgedTokenAccount state variable immutable would help avoid Gsset (20000 gas units) in the constructor during deployment. In implementing this change a cheaper stack read 3 gas would be used in place of 1 SLOAD (cold access 2100 gas units) in the withdraw() function which reads the pdaBridgedTokenAccount variable. The diff below shows how the code should be refactored:
file: lockbox-solana/solidity/liquidity_lockbox.sol 32: address public pdaBridgedTokenAccount;
diff --git a/lockbox-solana/solidity/liquidity_lockbox.sol b/lockbox-solana/solidity/liquidity_lockbox.sol index 6760041..88f6abd 100644 --- a/lockbox-solana/solidity/liquidity_lockbox.sol +++ b/lockbox-solana/solidity/liquidity_lockbox.sol @@ -29,7 +29,7 @@ contract liquidity_lockbox { // Bridged token mint address address public bridgedTokenMint; // PDA bridged token account address - address public pdaBridgedTokenAccount; + address public immutable pdaBridgedTokenAccount; // PDA header for position account uint64 public pdaHeader = 0xd0f7407ae48fbcaa;
Estimated Gas saved: 22100 gas units
pdaBump state variable could be made immutableMaking the pdaBump state variable immutable would help avoid Gsset (20000 gas units) in the constructor during deployment. In implementing this change a cheaper stack read 3 gas would be used in place of 1 SLOAD (cold access 2100 gas units) in the functions deposit() and withdraw() which reads the pdaBump variable. The diff below shows how the code should be refactored:
file: lockbox-solana/solidity/liquidity_lockbox.sol 28: bytes1 public pdaBump;
diff --git a/lockbox-solana/solidity/liquidity_lockbox.sol b/lockbox-solana/solidity/liquidity_lockbox.sol index 6760041..7c6c4c9 100644 --- a/lockbox-solana/solidity/liquidity_lockbox.sol +++ b/lockbox-solana/solidity/liquidity_lockbox.sol @@ -35,7 +35,7 @@ contract liquidity_lockbox { // Program PDA seed bytes public constant pdaProgramSeed = "pdaProgram"; // Program PDA bump - bytes1 public pdaBump; + bytes1 public immutable pdaBump; int32 public constant minTickLowerIndex = -443632; int32 public constant maxTickLowerIndex = 443632;
Estimated Gas saved: 24200 gas units
When you declare a state variable as a constant, you're telling the Solidity compiler that this variable's value is known at compile time and will never change during the contract's lifetime. Because of this: The value of the constant state variable is computed and set at the time of contract deployment, not during runtime.
Since the value is known beforehand and cannot change, there is no need to store this value on the Ethereum blockchain, as it can be computed directly whenever needed. When you access the constant state variable in your contract functions, it consume very little gas because the value is readily available (saved in the contracts bytecode) and does not require any computation.
So, by using constants for state variables whose value is known beforehand and is static (never changed), you save gas because you avoid unnecessary storage and computation costs associated with regular state variables.
pdaHeader state variable should be made a constatnt since its value is known before hand and never modified.Making the pdaHeader state variable constant would help avoid SLOAD cold access (2100) and SLOAD warm access (100) for subsequents reads and replace them with cheaper stack read 3 gas units. The diff below shows how the code could be refactored:
file: lockbox-solana/solidity/liquidity_lockbox.sol 34: uint64 public pdaHeader = 0xd0f7407ae48fbcaa;
diff --git a/lockbox-solana/solidity/liquidity_lockbox.sol b/lockbox-solana/solidity/liquidity_lockbox.sol index 6760041..701af47 100644 --- a/lockbox-solana/solidity/liquidity_lockbox.sol +++ b/lockbox-solana/solidity/liquidity_lockbox.sol @@ -31,7 +31,7 @@ contract liquidity_lockbox { // PDA bridged token account address address public pdaBridgedTokenAccount; // PDA header for position account - uint64 public pdaHeader = 0xd0f7407ae48fbcaa; + uint64 public constant pdaHeader = 0xd0f7407ae48fbcaa; // Program PDA seed bytes public constant pdaProgramSeed = "pdaProgram"; // Program PDA bump
library NoNamedReturnArithmetic { function sum(uint256 num1, uint256 num2) internal pure returns(uint256){ return num1 + num2; } } contract NoNamedReturn { using NoNamedReturnArithmetic for uint256; uint256 public stateVar; function add2State(uint256 num) public { stateVar = stateVar.sum(num); } }
test for test/NoNamedReturn.t.sol:NamedReturnTest [PASS] test_Increment() (gas: 27639)
library NamedReturnArithmetic { function sum(uint256 num1, uint256 num2) internal pure returns(uint256 theSum){ theSum = num1 + num2; } } contract NamedReturn { using NamedReturnArithmetic for uint256; uint256 public stateVar; function add2State(uint256 num) public { stateVar = stateVar.sum(num); } }
test for test/NamedReturn.t.sol:NamedReturnTest [PASS] test_Increment() (gas: 27613)
SplToken.total_supply() functionfile: lockbox-solana/solidity/library/spl_token.sol 109: function total_supply(AccountInfo account) internal view returns (uint64) { 110: return account.data.readUint64LE(36); 112: 113: }
diff --git a/lockbox-solana/solidity/library/spl_token.sol b/lockbox-solana/solidity/library/spl_token.sol index 4790760..6f89568 100644 --- a/lockbox-solana/solidity/library/spl_token.sol +++ b/lockbox-solana/solidity/library/spl_token.sol @@ -106,8 +106,8 @@ library SplToken { /// @dev Get the total supply for the mint, i.e. the total amount in circulation /// @param account The AccountInfo struct for the mint account - function total_supply(AccountInfo account) internal view returns (uint64) { - return account.data.readUint64LE(36); + function total_supply(AccountInfo account) internal view returns (uint64 totalSupply) { + totalSupply = account.data.readUint64LE(36); }
SplToken.total_supply() functionfile: lockbox-solana/solidity/library/spl_token.sol 115: function get_balance(AccountInfo account) internal view returns (uint64) { 116: return account.data.readUint64LE(64); 117: }
diff --git a/lockbox-solana/solidity/library/spl_token.sol b/lockbox-solana/solidity/library/spl_token.sol index 4790760..5ed4147 100644 --- a/lockbox-solana/solidity/library/spl_token.sol +++ b/lockbox-solana/solidity/library/spl_token.sol @@ -112,7 +112,7 @@ library SplToken { /// @dev Get the balance for an account. /// @param account the struct AccountInfo whose account balance we want to retrieve - function get_balance(AccountInfo account) internal view returns (uint64) { - return account.data.readUint64LE(64); + function get_balance(AccountInfo account) internal view returns (uint64 balance) { + balance = account.data.readUint64LE(64); }
OLAS.decreaseAllowance() to avoid writing to state if amount is zeroIn the OLAS.decreaseAllowance() function as shown below checks should be implemented to avoid writing to state if the amount argument is zero this is because if amount is 0 the statement spenderAllowance -= amount would not change the value of spenderAllowance since its being decremented by zero. This then means that in scenarios where amount is 0 the statement allowance[msg.sender][spender] = spenderAllowance is re-assigning the same value to state i.e the is no state change. The diff below shows how the function should be refactored:
file: governance/contracts/OLAS.sol 128: function decreaseAllowance(address spender, uint256 amount) external returns (bool) { 129: uint256 spenderAllowance = allowance[msg.sender][spender]; 130: 131: if (spenderAllowance != type(uint256).max) { 132: spenderAllowance -= amount; 133: allowance[msg.sender][spender] = spenderAllowance; 134: emit Approval(msg.sender, spender, spenderAllowance); 135: } 136: 137: return true; 138: }
C:\Users\USER\Coding Projects\AUDITS DIFFS\2023-12-autonolas (main -> origin) λ git diff diff --git a/governance/contracts/OLAS.sol b/governance/contracts/OLAS.sol index a1700ed..5c030ea 100644 --- a/governance/contracts/OLAS.sol +++ b/governance/contracts/OLAS.sol @@ -18,6 +18,8 @@ contract OLAS is ERC20 { event MinterUpdated(address indexed minter); event OwnerUpdated(address indexed owner); + error ZeroValue(); + // One year interval uint256 public constant oneYear = 1 days * 365; // Total supply cap for the first ten years (one billion OLAS tokens) @@ -126,6 +128,9 @@ contract OLAS is ERC20 { /// @param amount Amount to decrease approval by. /// @return True if the operation succeeded. function decreaseAllowance(address spender, uint256 amount) external returns (bool) { + if(amount == 0) { + revert ZeroValue(); + } uint256 spenderAllowance = allowance[msg.sender][spender]; if (spenderAllowance != type(uint256).max) {
Estimated gas saved: 2900 gas units
OLAS.increaseAllowance() to avoid writing to state if amount is zeroIn the OLAS.increaseAllowance() function as shown below checks should be implemented to avoid writing to state if the amount argument is zero this is because if amount is 0 the statement spenderAllowance += amount would not change the value of spenderAllowance since its being incremented by zero. This then means that in scenarios where amount is 0 the statement allowance[msg.sender][spender] = spenderAllowance is re-assigning the same value to state i.e the is no state change. The diff below shows how the function should be refactored:
file: governance/contracts/OLAS.sol 145: function increaseAllowance(address spender, uint256 amount) external returns (bool) { 146: uint256 spenderAllowance = allowance[msg.sender][spender]; 147: 148: spenderAllowance += amount; 149: allowance[msg.sender][spender] = spenderAllowance; 150: emit Approval(msg.sender, spender, spenderAllowance); 151: 152: return true; 153 }
diff --git a/governance/contracts/OLAS.sol b/governance/contracts/OLAS.sol index a1700ed..6a31e48 100644 --- a/governance/contracts/OLAS.sol +++ b/governance/contracts/OLAS.sol @@ -18,6 +18,7 @@ contract OLAS is ERC20 { event MinterUpdated(address indexed minter); event OwnerUpdated(address indexed owner); + error ZeroValue(); // One year interval uint256 public constant oneYear = 1 days * 365; // Total supply cap for the first ten years (one billion OLAS tokens) @@ -143,6 +144,9 @@ contract OLAS is ERC20 { /// @param amount Amount to increase approval by. /// @return True if the operation succeeded. function increaseAllowance(address spender, uint256 amount) external returns (bool) { + if (amount == 0) { + revert ZeroValue(); + } uint256 spenderAllowance = allowance[msg.sender][spender]; spenderAllowance += amount;
Estimated gas saved: 2900 gas units
OLAS.increaseAllowance() to avoid writing to state if amount is zeroIn the veOLAS._depositFor() function as shown below checks should be implemented to avoid writing to state if the amount argument is zero this is because if amount is 0 the statement supplyAfter = supplyBefore + amount would not change the value of supplyAfter since its being incremented by zero. This then means that in scenarios where amount is 0 the statement supply = supplyAfter is re-assigning the same value to state i.e the is no state change. The diff below shows how the function should be refactored:
file: governance/contracts/veOLAS.sol 330: function _depositFor( 331: address account, 332: uint256 amount, 333: uint256 unlockTime, 334: LockedBalance memory lockedBalance, 335: DepositType depositType 336: ) internal { 337: uint256 supplyBefore = supply; 338: uint256 supplyAfter; 339: // Cannot overflow because the total supply << 2^128-1 340: unchecked { 341: supplyAfter = supplyBefore + amount; 342: supply = supplyAfter; 343: } 344: // Get the old locked data 345: LockedBalance memory oldLocked; 346: (oldLocked.amount, oldLocked.endTime) = (lockedBalance.amount, lockedBalance.endTime); 347: // Adding to the existing lock, or if a lock is expired - creating a new one 348: // This cannot be larger than the total supply 349: unchecked { 350: lockedBalance.amount += uint128(amount); 351: } 352: if (unlockTime > 0) { 353: lockedBalance.endTime = uint64(unlockTime); 354: } 355: mapLockedBalances[account] = lockedBalance; 356: 357: // Possibilities: 358: // Both oldLocked.endTime could be current or expired (>/< block.timestamp) 359: // amount == 0 (extend lock) or amount > 0 (add to lock or extend lock) 360: // lockedBalance.endTime > block.timestamp (always) 361: _checkpoint(account, oldLocked, lockedBalance, uint128(supplyAfter)); 362: if (amount > 0) { 363: // OLAS is a solmate-based ERC20 token with optimized transferFrom() that either returns true or reverts 364: IERC20(token).transferFrom(msg.sender, address(this), amount); 365: } 366: 367: emit Deposit(account, amount, lockedBalance.endTime, depositType, block.timestamp); 368: emit Supply(supplyBefore, supplyAfter); 369: }
diff --git a/governance/contracts/veOLAS.sol b/governance/contracts/veOLAS.sol index 6f14419..126b6f2 100644 --- a/governance/contracts/veOLAS.sol +++ b/governance/contracts/veOLAS.sol @@ -95,6 +95,8 @@ contract veOLAS is IErrors, IVotes, IERC20, IERC165 { event Withdraw(address indexed account, uint256 amount, uint256 ts); event Supply(uint256 previousSupply, uint256 currentSupply); + error ZeroValue(); + // 1 week time uint64 internal constant WEEK = 1 weeks; // Maximum lock time (4 years) @@ -334,6 +336,9 @@ contract veOLAS is IErrors, IVotes, IERC20, IERC165 { LockedBalance memory lockedBalance, DepositType depositType ) internal { + if(amount == 0) { + revert ZeroValue() + } uint256 supplyBefore = supply; uint256 supplyAfter; // Cannot overflow because the total supply << 2^128-1 @@ -359,10 +364,9 @@ contract veOLAS is IErrors, IVotes, IERC20, IERC165 { // amount == 0 (extend lock) or amount > 0 (add to lock or extend lock) // lockedBalance.endTime > block.timestamp (always) _checkpoint(account, oldLocked, lockedBalance, uint128(supplyAfter)); - if (amount > 0) { - // OLAS is a solmate-based ERC20 token with optimized transferFrom() that either returns true or reverts - IERC20(token).transferFrom(msg.sender, address(this), amount); - } + // OLAS is a solmate-based ERC20 token with optimized transferFrom() that either returns true or reverts + + IERC20(token).transferFrom(msg.sender, address(this), amount); emit Deposit(account, amount, lockedBalance.endTime, depositType, block.timestamp); emit Supply(supplyBefore, supplyAfter);
Estimated gas saved: 2900 gas units
Tokenomics.reserveAmountForBondProgram() to avoid writing to state if amount is zeroIn the Tokenomics.reserveAmountForBondProgram() function as shown below checks should be implemented to avoid writing to state if the amount argument is zero this is because if amount is 0 the statement eBond -= amount would not change the value of eBond since its being decremented by zero. This then means that in scenarios where amount is 0 the statement effectiveBond = uint96(eBond) is re-assigning the same value to state i.e there is no state change. The diff below shows how the function should be refactored:
file: tokenomics/contracts/Tokenomics.sol 609: function reserveAmountForBondProgram(uint256 amount) external returns (bool success) { 610: // Check for the depository access 611: if (depository != msg.sender) { 612: revert ManagerOnly(msg.sender, depository); 613: } 614: 615: // Effective bond must be bigger than the requested amount 616: uint256 eBond = effectiveBond; 617: if (eBond >= amount) { 618: // The effective bond value is adjusted with the amount that is reserved for bonding 619: // The unrealized part of the bonding amount will be returned when the bonding program is closed 620: eBond -= amount; 621: effectiveBond = uint96(eBond); 622: success = true; 623: emit EffectiveBondUpdated(eBond); 624: } 625: }
diff --git a/tokenomics/contracts/Tokenomics.sol b/tokenomics/contracts/Tokenomics.sol index 2ef2708..808880b 100644 --- a/tokenomics/contracts/Tokenomics.sol +++ b/tokenomics/contracts/Tokenomics.sol @@ -136,6 +136,8 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { event EpochSettled(uint256 indexed epochCounter, uint256 treasuryRewards, uint256 accountRewards, uint256 accountTopUps); event TokenomicsImplementationUpdated(address indexed implementation); + error ZeroValue(); + // Owner address address public owner; // Max bond per epoch: calculated as a fraction from the OLAS inflation parameter @@ -607,6 +609,9 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { /// @return success True if effective bond threshold is not reached. /// #if_succeeds {:msg "effectiveBond"} old(effectiveBond) > amount ==> effectiveBond == old(effectiveBond) - amount; function reserveAmountForBondProgram(uint256 amount) external returns (bool success) { + if(amount == 0) { + revert ZeroValue(); + } // Check for the depository access if (depository != msg.sender) { revert ManagerOnly(msg.sender, depository);
Estimated gas saved: 2900 gas units
Tokenomics.refundFromBondProgram() to avoid writing to state if amount is zeroIn the Tokenomics.refundFromBondProgram() function as shown below checks should be implemented to avoid writing to state if the amount argument is zero this is because if amount is 0 the statement uint256 eBond = effectiveBond + amount would not change the value of effectiveBond since its being decremented by zero. This then means that in scenarios where amount is 0 the statement effectiveBond = uint96(eBond) is re-assigning the same value to state i.e there is no state change. The diff below shows how the function should be refactored:
file: tokenomics/contracts/Tokenomics.sol 630: function refundFromBondProgram(uint256 amount) external { 631: // Check for the depository access 632: if (depository != msg.sender) { 633: revert ManagerOnly(msg.sender, depository); 634: } 635: 636: uint256 eBond = effectiveBond + amount; 637: // This scenario is not realistically possible. It is only possible when closing the bonding program 638: // with the effectiveBond value close to uint96 max 639: if (eBond > type(uint96).max) { 640: revert Overflow(eBond, type(uint96).max); 641: } 642: effectiveBond = uint96(eBond); 643: emit EffectiveBondUpdated(eBond); 644: }
diff --git a/tokenomics/contracts/Tokenomics.sol b/tokenomics/contracts/Tokenomics.sol index 2ef2708..ad396eb 100644 --- a/tokenomics/contracts/Tokenomics.sol +++ b/tokenomics/contracts/Tokenomics.sol @@ -136,6 +136,8 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { event EpochSettled(uint256 indexed epochCounter, uint256 treasuryRewards, uint256 accountRewards, uint256 accountTopUps); event TokenomicsImplementationUpdated(address indexed implementation); + error ZeroValue(); + // Owner address address public owner; // Max bond per epoch: calculated as a fraction from the OLAS inflation parameter @@ -628,6 +630,9 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { /// @param amount Amount to be refunded from the closed bond program. /// #if_succeeds {:msg "effectiveBond"} old(effectiveBond + amount) <= type(uint96).max ==> effectiveBond == old(effectiveBond) + amount; function refundFromBondProgram(uint256 amount) external { + if(amount == 0) { + revert ZeroValue(); + } // Check for the depository access if (depository != msg.sender) { revert ManagerOnly(msg.sender, depository);
Estimated gas saved: 2900 gas units
In scenarios where a function emits multiple event but these events are only emitted by that function we can combine these events to a single event in doing this we would save at least 1 Glog0 375 gas units
Depository.changeManagers() to reduce gas costThe Depository.changeManagers() function emits two events TokenomicsUpdated and TreasuryUpdated but these events are only emitted in this function. We can make the Depository.changeManagers() function more gas efficient if we combine these TokenomicsUpdated and TreasuryUpdated events to a single event. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Depository.sol 143: function changeManagers(address _tokenomics, address _treasury) external { 144: // Check for the contract ownership 145: if (msg.sender != owner) { 146: revert OwnerOnly(msg.sender, owner); 147: } 148: 149: // Change Tokenomics contract address 150: if (_tokenomics != address(0)) { 151: tokenomics = _tokenomics; 152: emit TokenomicsUpdated(_tokenomics); 153: } 154: // Change Treasury contract address 155: if (_treasury != address(0)) { 156: treasury = _treasury; 157: emit TreasuryUpdated(_treasury); 158: } 159: }
diff --git a/tokenomics/contracts/Depository.sol b/tokenomics/contracts/Depository.sol index f9c7d64..e9ac51c 100644 --- a/tokenomics/contracts/Depository.sol +++ b/tokenomics/contracts/Depository.sol @@ -61,8 +61,7 @@ struct Product { /// @author Aleksandr Kuperman - <aleksandr.kuperman@valory.xyz> contract Depository is IErrorsTokenomics { event OwnerUpdated(address indexed owner); - event TokenomicsUpdated(address indexed tokenomics); - event TreasuryUpdated(address indexed treasury); + event TokenomicsAndTreasuryUpdated(address indexed tokenomics, address indexed treasury); event BondCalculatorUpdated(address indexed bondCalculator); event CreateBond(address indexed token, uint256 indexed productId, address indexed owner, uint256 bondId, uint256 amountOLAS, uint256 tokenAmount, uint256 maturity); @@ -146,16 +145,13 @@ contract Depository is IErrorsTokenomics { revert OwnerOnly(msg.sender, owner); } - // Change Tokenomics contract address - if (_tokenomics != address(0)) { + // Change Tokenomics contract address and Treasury contract address + if (_tokenomics != address(0) && _treasury != address(0)) { tokenomics = _tokenomics; - emit TokenomicsUpdated(_tokenomics); - } - // Change Treasury contract address - if (_treasury != address(0)) { treasury = _treasury; - emit TreasuryUpdated(_treasury); + emit TokenomicsAndTreasuryUpdated(_tokenomics, _treasury); }
Estimated gas saved: 375 gas units
Dispenser.changeManagers() to reduce gas costThe Dispenser.changeManagers() function emits two events TokenomicsUpdated and TreasuryUpdated but these events are only emitted in this function. We can make the Dispenser.changeManagers() function more gas efficient if we combine these TokenomicsUpdated and TreasuryUpdated events to a single event. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Dispenser.sol 64: function changeManagers(address _tokenomics, address _treasury) external { 65: // Check for the contract ownership 66: if (msg.sender != owner) { 67: revert OwnerOnly(msg.sender, owner); 68: } 69: 70: // Change Tokenomics contract address 71: if (_tokenomics != address(0)) { 72: tokenomics = _tokenomics; 73: emit TokenomicsUpdated(_tokenomics); 74: } 75: // Change Treasury contract address 76: if (_treasury != address(0)) { 77: treasury = _treasury; 78: emit TreasuryUpdated(_treasury); 79: } 80: }
diff --git a/tokenomics/contracts/Dispenser.sol b/tokenomics/contracts/Dispenser.sol index 9b8f4f5..79e9173 100644 --- a/tokenomics/contracts/Dispenser.sol +++ b/tokenomics/contracts/Dispenser.sol @@ -10,8 +10,7 @@ import "./interfaces/ITreasury.sol"; /// @author Aleksandr Kuperman - <aleksandr.kuperman@valory.xyz> contract Dispenser is IErrorsTokenomics { event OwnerUpdated(address indexed owner); - event TokenomicsUpdated(address indexed tokenomics); - event TreasuryUpdated(address indexed treasury); + event TokenomicsAndTreasuryUpdated(address indexed tokenomics, address indexed treasury); event IncentivesClaimed(address indexed owner, uint256 reward, uint256 topUp); // Owner address @@ -67,16 +66,13 @@ contract Dispenser is IErrorsTokenomics { revert OwnerOnly(msg.sender, owner); } - // Change Tokenomics contract address - if (_tokenomics != address(0)) { + // Change Tokenomics contract address and Treasury contract address + if (_tokenomics != address(0) && _treasury != address(0)) { tokenomics = _tokenomics; - emit TokenomicsUpdated(_tokenomics); - } - // Change Treasury contract address - if (_treasury != address(0)) { treasury = _treasury; - emit TreasuryUpdated(_treasury); + emit TokenomicsAndTreasuryUpdated(_tokenomics, _treasury); } +
Estimated gas saved: 375 gas units
Tokenomics.changeManagers() to reduce gas costThe Tokenomics.changeManagers() function emits three events DepositoryUpdated, DispenserUpdated and TreasuryUpdated but these events are only emitted in this function. We can make the Tokenomics.changeManagers() function more gas efficient if we combine these DepositoryUpdated, DispenserUpdated and TreasuryUpdated events to a single event. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Tokenomics.sol 423: function changeManagers(address _treasury, address _depository, address _dispenser) external { 424: // Check for the contract ownership 425: if (msg.sender != owner) { 426: revert OwnerOnly(msg.sender, owner); 427: } 428: 429: // Change Treasury contract address 430: if (_treasury != address(0)) { 431: treasury = _treasury; 432: emit TreasuryUpdated(_treasury); 433: } 434: // Change Depository contract address 435: if (_depository != address(0)) { 436: depository = _depository; 437: emit DepositoryUpdated(_depository); 438: } 439: // Change Dispenser contract address 440: if (_dispenser != address(0)) { 441: dispenser = _dispenser; 442: emit DispenserUpdated(_dispenser); 443: } 444: }
diff --git a/tokenomics/contracts/Tokenomics.sol b/tokenomics/contracts/Tokenomics.sol index 2ef2708..5b30142 100644 --- a/tokenomics/contracts/Tokenomics.sol +++ b/tokenomics/contracts/Tokenomics.sol @@ -117,9 +117,7 @@ struct IncentiveBalances { /// @author Aleksandr Kuperman - <aleksandr.kuperman@valory.xyz> contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { event OwnerUpdated(address indexed owner); - event TreasuryUpdated(address indexed treasury); - event DepositoryUpdated(address indexed depository); - event DispenserUpdated(address indexed dispenser); + event TreasuryDepositoryAndDispenserUpdated(address indexed treasury, address indexed depository, address indexed dispenser); event EpochLengthUpdated(uint256 epochLen); event EffectiveBondUpdated(uint256 effectiveBond); event IDFUpdated(uint256 idf); @@ -426,21 +424,14 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { revert OwnerOnly(msg.sender, owner); } - // Change Treasury contract address - if (_treasury != address(0)) { + // Change Treasury contract address, Depository contract address and Dispenser contract address + if (_treasury != address(0) && _depository != address(0) && _dispenser != address(0)) { treasury = _treasury; - emit TreasuryUpdated(_treasury); - } - // Change Depository contract address - if (_depository != address(0)) { depository = _depository; - emit DepositoryUpdated(_depository); - } - // Change Dispenser contract address - if (_dispenser != address(0)) { dispenser = _dispenser; - emit DispenserUpdated(_dispenser); + emit TreasuryDepositoryAndDispenserUpdated(_treasury, _depository, _dispenser); } + }
Estimated Gas saved: 750 gas units
Tokenomics.changeRegistries() to reduce gas costThe Tokenomics.changeRegistries() function emits three events ComponentRegistryUpdated, AgentRegistryUpdated and ServiceRegistryUpdated but these events are only emitted in this function. We can make the Tokenomics.changeManagers() function more gas efficient if we combine these ComponentRegistryUpdated, AgentRegistryUpdated and ServiceRegistryUpdated events to a single event. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Tokenomics.sol 450: function changeRegistries(address _componentRegistry, address _agentRegistry, address _serviceRegistry) external { 451: // Check for the contract ownership 452: if (msg.sender != owner) { 453: revert OwnerOnly(msg.sender, owner); 454: } 455: 456: // Check for registries addresses 457: if (_componentRegistry != address(0)) { 458: componentRegistry = _componentRegistry; 459: emit ComponentRegistryUpdated(_componentRegistry); 460: } 461: if (_agentRegistry != address(0)) { 462: agentRegistry = _agentRegistry; 463: emit AgentRegistryUpdated(_agentRegistry); 464: } 465: if (_serviceRegistry != address(0)) { 466: serviceRegistry = _serviceRegistry; 467: emit ServiceRegistryUpdated(_serviceRegistry); 468: } 469: }
diff --git a/tokenomics/contracts/Tokenomics.sol b/tokenomics/contracts/Tokenomics.sol index 2ef2708..28e3bfb 100644 --- a/tokenomics/contracts/Tokenomics.sol +++ b/tokenomics/contracts/Tokenomics.sol @@ -129,9 +129,7 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { event IncentiveFractionsUpdateRequested(uint256 indexed epochNumber, uint256 rewardComponentFraction, uint256 rewardAgentFraction, uint256 maxBondFraction, uint256 topUpComponentFraction, uint256 topUpAgentFraction); event IncentiveFractionsUpdated(uint256 indexed epochNumber); - event ComponentRegistryUpdated(address indexed componentRegistry); - event AgentRegistryUpdated(address indexed agentRegistry); - event ServiceRegistryUpdated(address indexed serviceRegistry); + event ComponentRegistryAgentRegistryAndServiceRegistryUpdated(address indexed componentRegistry, address indexed agentRegistry, address indexed serviceRegistry); event DonatorBlacklistUpdated(address indexed blacklist); event EpochSettled(uint256 indexed epochCounter, uint256 treasuryRewards, uint256 accountRewards, uint256 accountTopUps); event TokenomicsImplementationUpdated(address indexed implementation); @@ -454,17 +452,11 @@ contract Tokenomics is TokenomicsConstants, IErrorsTokenomics { } // Check for registries addresses - if (_componentRegistry != address(0)) { + if (_componentRegistry != address(0) && _agentRegistry != address(0) && _serviceRegistry != address(0)) { componentRegistry = _componentRegistry; - emit ComponentRegistryUpdated(_componentRegistry); - } - if (_agentRegistry != address(0)) { agentRegistry = _agentRegistry; - emit AgentRegistryUpdated(_agentRegistry); - } - if (_serviceRegistry != address(0)) { serviceRegistry = _serviceRegistry; - emit ServiceRegistryUpdated(_serviceRegistry); + emit ComponentRegistryAgentRegistryAndServiceRegistryUpdated(_componentRegistry, _agentRegistry, _serviceRegistry) } }
Estimated gas saved: 750 gas units
Treasury.changeManagers() to reduce gas costThe Treasury.changeManagers() function emits three events TokenomicsUpdated, DepositoryUpdated and DispenserUpdated but these events are only emitted in this function. We can make the Treasury.changeManagers() function more gas efficient if we combine these TokenomicsUpdated, DepositoryUpdated and DispenserUpdated events to a single event. The diff below shows how the code could be refactored:
file: tokenomics/contracts/Treasury.sol 156: function changeManagers(address _tokenomics, address _depository, address _dispenser) external { 157: // Check for the contract ownership 158: if (msg.sender != owner) { 159: revert OwnerOnly(msg.sender, owner); 160: } 161: 162: // Change Tokenomics contract address 163: if (_tokenomics != address(0)) { 164: tokenomics = _tokenomics; 165: emit TokenomicsUpdated(_tokenomics); 166: } 167: // Change Depository contract address 168: if (_depository != address(0)) { 169: depository = _depository; 170: emit DepositoryUpdated(_depository); 171: } 172: // Change Dispenser contract address 173: if (_dispenser != address(0)) { 174: dispenser = _dispenser; 175: emit DispenserUpdated(_dispenser); 176: } 177: }
diff --git a/tokenomics/contracts/Treasury.sol b/tokenomics/contracts/Treasury.sol index 516c631..cc28ac9 100644 --- a/tokenomics/contracts/Treasury.sol +++ b/tokenomics/contracts/Treasury.sol @@ -38,9 +38,7 @@ import "./interfaces/ITokenomics.sol"; /// invariant {:msg "broken conservation law"} address(this).balance == ETHFromServices + ETHOwned; contract Treasury is IErrorsTokenomics { event OwnerUpdated(address indexed owner); - event TokenomicsUpdated(address indexed tokenomics); - event DepositoryUpdated(address indexed depository); - event DispenserUpdated(address indexed dispenser); + event TokenomicsDepositoryAndDispenserUpdated(address indexed tokenomics, address indexed depository, address indexed dispenser); event DepositTokenFromAccount(address indexed account, address indexed token, uint256 tokenAmount, uint256 olasAmount); event DonateToServicesETH(address indexed sender, uint256[] serviceIds, uint256[] amounts, uint256 donation); event Withdraw(address indexed token, address indexed to, uint256 tokenAmount); @@ -159,20 +157,12 @@ contract Treasury is IErrorsTokenomics { revert OwnerOnly(msg.sender, owner); } - // Change Tokenomics contract address - if (_tokenomics != address(0)) { + // Change Tokenomics contract address, Depository contract address and Dispenser contract address + if (_tokenomics != address(0) && depository != address(0) && dispenser != address(0)) { tokenomics = _tokenomics; - emit TokenomicsUpdated(_tokenomics); - } - // Change Depository contract address - if (_depository != address(0)) { depository = _depository; - emit DepositoryUpdated(_depository); - } - // Change Dispenser contract address - if (_dispenser != address(0)) { dispenser = _dispenser; - emit DispenserUpdated(_dispenser); + TokenomicsDepositoryAndDispenserUpdated(_tokenomics, _depository,_dispenser); }
Estimated gas saved: 750 gas units
Instead of using abi.decode, we can use assembly to decode our desired calldata values directly. This will allow us to avoid decoding calldata values that we will not use.
file: governance/contracts/multisigs/GuardCM.sol 337: function _verifySchedule(bytes memory data, bytes4 selector) internal { 338: // Copy the data without the selector 339: bytes memory payload = new bytes(data.length - SELECTOR_DATA_LENGTH); 340: for (uint256 i = 0; i < payload.length; ++i) { 341: payload[i] = data[i + 4]; 342: } 343: 344: // Prepare the decoding data sets 345: address[] memory targets; 346: bytes[] memory callDatas; 347: if (selector == SCHEDULE) { 348: targets = new address[](1); 349: callDatas = new bytes[](1); 350: // Decode the data in the schedule function 351: (targets[0], , callDatas[0], , , ) = 352: abi.decode(payload, (address, uint256, bytes, bytes32, bytes32, uint256)); //@audit use assembly 353: } else { 354: // Decode the data in the scheduleBatch function 355: (targets, , callDatas, , , ) = 356: abi.decode(payload, (address[], uint256[], bytes[], bytes32, bytes32, uint256)); //@audit use assembly . . . 379: }
Require() or revert() statements that check input arguments or cost lesser gas should be at the top of the function. Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting alot of gas in a function that may ultimately revert in the unhappy case.
When 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.
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
If a function modifier or require such as onlyOwner/onlyX is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost. the recommended Mitigation Steps is that Functions guaranteed to revert when called by normal users can be marked payable.
As you embark on incorporating the recommended optimizations, we want to emphasize the utmost importance of proceeding with vigilance and dedicating thorough efforts to comprehensive testing. It is of paramount significance to ensure that the proposed alterations do not inadvertently introduce fresh vulnerabilities, while also successfully achieving the anticipated enhancements in performance.
We strongly advise conducting a meticulous and exhaustive evaluation of the modifications made to the codebase. This rigorous scrutiny and exhaustive assessment will play a pivotal role in affirming both the security and efficacy of the refactored code. Your careful attention to detail, coupled with the implementation of a robust testing framework, will provide the necessary assurance that the refined code aligns with your security objectives and effectively fulfills the intended performance optimizations.
#0 - c4-pre-sort
2024-01-10T13:38:46Z
alex-ppg marked the issue as high quality report
#1 - c4-sponsor
2024-01-16T12:49:59Z
hannahtuttle (sponsor) acknowledged
#2 - c4-sponsor
2024-01-16T17:47:41Z
kupermind (sponsor) acknowledged
#3 - c4-judge
2024-01-20T14:50:03Z
dmvt marked the issue as grade-a
#4 - c4-judge
2024-01-20T18:21:09Z
dmvt marked the issue as selected for report