Platform: Code4rena
Start Date: 31/01/2023
Pot Size: $90,500 USDC
Total HM: 47
Participants: 169
Period: 7 days
Judge: LSDan
Total Solo HM: 9
Id: 211
League: ETH
Rank: 85/169
Findings: 1
Award: $69.82
๐ Selected for report: 0
๐ Solo Findings: 0
๐ Selected for report: c3phas
Also found by: 0xSmartContract, 0xackermann, 0xdaydream, Aymen0909, CodingNameKiki, Dewaxindo, Diana, IllIllI, Madalad, NoamYakov, Pheonix, Polaris_tow, ReyAdmirado, Rolezn, arialblack14, atharvasama, cryptostellar5, descharre, eyexploit, lukris02, saneryee
69.8247 USDC - $69.82
Exlusive findings is below the automated findings.
Issue | Instances | |
---|---|---|
GAS-1 | Use assembly to check for address(0) | 24 |
GAS-2 | Using bools for storage incurs overhead | 3 |
GAS-3 | Cache array length outside of loop | 5 |
GAS-4 | State variables should be cached in stack variables rather than re-reading them from storage | 3 |
GAS-5 | Use calldata instead of memory for function arguments that do not get mutated | 16 |
GAS-6 | Don't initialize variables with default value | 19 |
GAS-7 | Functions guaranteed to revert when called by normal users can be marked payable | 28 |
GAS-8 | ++i costs less gas than i++ , especially when it's used in for -loops (--i /i-- too) | 24 |
GAS-9 | Using private rather than public for constants, saves gas | 1 |
GAS-10 | Use != 0 instead of > 0 for unsigned integer comparison | 23 |
address(0)
Saves 6 gas per instance
Instances (24):
File: src/utils/MultiRewardEscrow.sol 96: if (account == address(0)) revert ZeroAddress();
File: src/utils/MultiRewardStaking.sol 142: if (from == address(0) || to == address(0)) revert ZeroAddressTransfer(from, to); 142: if (from == address(0) || to == address(0)) revert ZeroAddressTransfer(from, to); 481: if (recoveredAddress == address(0) || recoveredAddress != owner) revert InvalidSigner(recoveredAddress);
File: src/vault/Vault.sol 74: if (address(asset_) == address(0)) revert InvalidAsset(); 90: if (feeRecipient_ == address(0)) revert InvalidFeeRecipient(); 141: if (receiver == address(0)) revert InvalidReceiver(); 177: if (receiver == address(0)) revert InvalidReceiver(); 216: if (receiver == address(0)) revert InvalidReceiver(); 258: if (receiver == address(0)) revert InvalidReceiver(); 554: if (_feeRecipient == address(0)) revert InvalidFeeRecipient(); 702: if (recoveredAddress == address(0) || recoveredAddress != owner)
File: src/vault/VaultController.sol 108: if (staking == address(0)) staking = _deployStaking(IERC20(address(vault)), _deploymentController); 687: token == address(0) 693: if (adapter != address(0)) { 837: if (address(_deploymentController) == address(0) || address(deploymentController) == address(_deploymentController))
File: src/vault/VaultRegistry.sol 45: if (metadata[_metadata.vault].vault != address(0)) revert VaultAlreadyRegistered();
File: src/vault/adapter/beefy/BeefyAdapter.sol 62: _beefyBooster != address(0) && 77: _beefyBooster == address(0) ? _beefyVault : _beefyBooster 82: if (_beefyBooster != address(0)) 138: if (address(beefyBooster) == address(0)) return _rewardTokens; 198: if (address(beefyBooster) != address(0)) 209: if (address(beefyBooster) != address(0)) 222: if (address(beefyBooster) == address(0)) revert NoBeefyBooster();
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from โfalseโ to โtrueโ, after having been โtrueโ in the past. See source.
Instances (3):
File: src/vault/CloneRegistry.sol 28: mapping(address => bool) public cloneExists;
File: src/vault/TemplateRegistry.sol 33: mapping(bytes32 => bool) public templateExists; 35: mapping(bytes32 => bool) public templateCategoryExists;
If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).
Instances (5):
File: src/utils/MultiRewardEscrow.sol 53: for (uint256 i = 0; i < escrowIds.length; i++) { 155: for (uint256 i = 0; i < escrowIds.length; i++) { 210: for (uint256 i = 0; i < tokens.length; i++) {
File: src/utils/MultiRewardStaking.sol 171: for (uint8 i; i < _rewardTokens.length; i++) { 373: for (uint8 i; i < _rewardTokens.length; i++) {
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.
Saves 100 gas per instance
Instances (3):
File: src/vault/Vault.sol 545: fees = proposedFees; 606: emit ChangedAdapter(adapter, proposedAdapter); 608: adapter = proposedAdapter;
Mark data types as calldata
instead of memory
where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata
. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory
storage.
Instances (16):
File: src/utils/MultiRewardEscrow.sol 154: function claimRewards(bytes32[] memory escrowIds) external { 207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner { 207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {
File: src/utils/MultiRewardStaking.sol 170: function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
File: src/vault/VaultController.sol 91: DeploymentArgs memory adapterData, 92: DeploymentArgs memory strategyData, 94: bytes memory rewardsData, 95: VaultMetadata memory metadata, 188: DeploymentArgs memory adapterData, 189: DeploymentArgs memory strategyData, 433: function addStakingRewardsTokens(address[] memory vaults, bytes[] memory rewardTokenData) public { 433: function addStakingRewardsTokens(address[] memory vaults, bytes[] memory rewardTokenData) public {
File: src/vault/adapter/beefy/BeefyAdapter.sol 47: bytes memory adapterInitData, 49: bytes memory beefyInitData
File: src/vault/adapter/yearn/YearnAdapter.sol 35: bytes memory adapterInitData, 37: bytes memory
Instances (19):
File: src/utils/MultiRewardEscrow.sol 53: for (uint256 i = 0; i < escrowIds.length; i++) { 155: for (uint256 i = 0; i < escrowIds.length; i++) { 210: for (uint256 i = 0; i < tokens.length; i++) {
File: src/vault/PermissionRegistry.sol 42: for (uint256 i = 0; i < len; i++) {
File: src/vault/VaultController.sol 319: for (uint8 i = 0; i < len; i++) { 337: for (uint8 i = 0; i < len; i++) { 357: for (uint8 i = 0; i < len; i++) { 374: for (uint8 i = 0; i < len; i++) { 437: for (uint256 i = 0; i < len; i++) { 494: for (uint256 i = 0; i < len; i++) { 523: for (uint256 i = 0; i < len; i++) { 564: for (uint256 i = 0; i < len; i++) { 587: for (uint256 i = 0; i < len; i++) { 607: for (uint256 i = 0; i < len; i++) { 620: for (uint256 i = 0; i < len; i++) { 633: for (uint256 i = 0; i < len; i++) { 646: for (uint256 i = 0; i < len; i++) { 766: for (uint256 i = 0; i < len; i++) { 806: for (uint256 i = 0; i < len; i++) {
payable
If a function modifier such as onlyOwner
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.
Instances (28):
File: src/utils/MultiRewardEscrow.sol 207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {
File: src/utils/MultiRewardStaking.sol 296: function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
File: src/vault/CloneFactory.sol 39: function deploy(Template calldata template, bytes calldata data) external onlyOwner returns (address clone) {
File: src/vault/DeploymentController.sol 55: function addTemplateCategory(bytes32 templateCategory) external onlyOwner { 80: function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner { 121: function nominateNewDependencyOwner(address _owner) external onlyOwner {
File: src/vault/PermissionRegistry.sol 38: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner {
File: src/vault/TemplateRegistry.sol 52: function addTemplateCategory(bytes32 templateCategory) external onlyOwner { 102: function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
File: src/vault/Vault.sol 525: function proposeFees(VaultFees calldata newFees) external onlyOwner { 553: function setFeeRecipient(address _feeRecipient) external onlyOwner { 578: function proposeAdapter(IERC4626 newAdapter) external onlyOwner { 629: function setQuitPeriod(uint256 _quitPeriod) external onlyOwner { 643: function pause() external onlyOwner { 648: function unpause() external onlyOwner {
File: src/vault/VaultController.sol 408: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner { 543: function setEscrowTokenFees(IERC20[] calldata tokens, uint256[] calldata fees) external onlyOwner { 561: function addTemplateCategories(bytes32[] calldata templateCategories) external onlyOwner { 723: function nominateNewAdminProxyOwner(address newOwner) external onlyOwner { 751: function setPerformanceFee(uint256 newFee) external onlyOwner { 764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner { 791: function setHarvestCooldown(uint256 newCooldown) external onlyOwner { 804: function setAdapterHarvestCooldowns(address[] calldata adapters) external onlyOwner { 832: function setDeploymentController(IDeploymentController _deploymentController) external onlyOwner { 864: function setActiveTemplateId(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
File: src/vault/VaultRegistry.sol 44: function registerVault(VaultMetadata calldata _metadata) external onlyOwner {
File: src/vault/adapter/abstracts/WithRewards.sol 15: function claim() public virtual onlyStrategy {}
File: src/vault/adapter/beefy/BeefyAdapter.sol 221: function claim() public override onlyStrategy {
++i
costs less gas than i++
, especially when it's used in for
-loops (--i
/i--
too)Saves 5 gas per loop
Instances (24):
File: src/utils/MultiRewardEscrow.sol 53: for (uint256 i = 0; i < escrowIds.length; i++) { 102: nonce++; 155: for (uint256 i = 0; i < escrowIds.length; i++) { 210: for (uint256 i = 0; i < tokens.length; i++) {
File: src/utils/MultiRewardStaking.sol 171: for (uint8 i; i < _rewardTokens.length; i++) { 373: for (uint8 i; i < _rewardTokens.length; i++) { 470: nonces[owner]++,
File: src/vault/PermissionRegistry.sol 42: for (uint256 i = 0; i < len; i++) {
File: src/vault/Vault.sol 691: nonces[owner]++,
File: src/vault/VaultController.sol 319: for (uint8 i = 0; i < len; i++) { 337: for (uint8 i = 0; i < len; i++) { 357: for (uint8 i = 0; i < len; i++) { 374: for (uint8 i = 0; i < len; i++) { 437: for (uint256 i = 0; i < len; i++) { 494: for (uint256 i = 0; i < len; i++) { 523: for (uint256 i = 0; i < len; i++) { 564: for (uint256 i = 0; i < len; i++) { 587: for (uint256 i = 0; i < len; i++) { 607: for (uint256 i = 0; i < len; i++) { 620: for (uint256 i = 0; i < len; i++) { 633: for (uint256 i = 0; i < len; i++) { 646: for (uint256 i = 0; i < len; i++) { 766: for (uint256 i = 0; i < len; i++) { 806: for (uint256 i = 0; i < len; i++) {
private
rather than public
for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
Instances (1):
File: src/vault/adapter/beefy/BeefyAdapter.sol 31: uint256 public constant BPS_DENOMINATOR = 10_000;
Instances (23):
File: src/utils/MultiRewardEscrow.sol 107: if (feePerc > 0) { 141: return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0;
File: src/utils/MultiRewardStaking.sol 178: if (escrowInfo.escrowPercentage > 0) { 255: if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken); 257: if (amount > 0) { 341: if (rewards.rewardsPerSecond > 0) { 377: if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
File: src/vault/Vault.sol 149: if (feeShares > 0) _mint(feeRecipient, feeShares); 189: if (feeShares > 0) _mint(feeRecipient, feeShares); 235: if (feeShares > 0) _mint(feeRecipient, feeShares); 273: if (feeShares > 0) _mint(feeRecipient, feeShares); 432: managementFee > 0 453: performanceFee > 0 && shareValue > highWaterMark 488: if (managementFee > 0) feesUpdatedAt = block.timestamp; 490: if (totalFee > 0 && currentAssets > 0) 490: if (totalFee > 0 && currentAssets > 0)
File: src/vault/VaultController.sol 103: if (adapterData.id > 0) 112: if (rewardsData.length > 0) _handleVaultStakingRewards(vault, rewardsData); 169: if (initialDeposit > 0) { 211: if (strategyData.id > 0) { 694: if (adapterId > 0) revert AdapterConfigFaulty();
File: src/vault/adapter/beefy/BeefyAdapter.sol 156: if (beefyFee > 0) 177: if (beefyFee > 0)
uint256
instead of bool
in mappings is more gas efficient [146 gas per instance]Description:
OpenZeppelin uint256 and bool comparison:
Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled. The values being non-zero value makes deployment a bit more expensive.
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from โfalseโ to โtrueโ, after having been โtrueโ in the past.
Recommendation:
Use uint256(1) and uint256(2) instead of bool.
Lines of Code:
Description:
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. Each operation involving a uint8 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 uint8, as well as the associated stack operations of doing so.
Recommendation:
Use a larger size then downcast where needed
Lines of Code:
Description:
In Solidity 0.8+, thereโs a default overflow check on unsigned integers. Itโs possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.
Recommendation:
Using Solidity's unchecked block saves the overflow checks.
Proof Of Concept:
Lines of Code:
Description:
Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.
Recommendation:
Find a lower method ID
name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas. For example, the function IDs in the L1GraphTokenGateway.sol contract will be the most used; A lower method ID may be given.
Proof Of Concept:
Lines of Code:
constructor
to payable
[~13 gas per instance]Lines of Code:
Description:
block.number
and block.timestamp
are added to the event information by default, so adding them manually will waste additional gas.
Lines of Code:
Description:
In the EVM, there is no opcode for >=
or <=
. When using greater than or equal, two operations are performed: >
and =
. Using strict comparison operators hence saves gas.
Recommendation:
Replace <=
with <
, and >=
with >
. Do not forget to increment/decrement the compared variable.
Lines of Code:
Description:
Solidity 0.8.10 has a useful change that reduced gas costs of external calls which expect a return value.
In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.
In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.
Lines of Code:
<array>.length
should not be looked up in every loop of a for-loopDescription:
The overheads outlined below are PER LOOP, excluding the first loop
storage arrays incur a Gwarmaccess (100 gas)
memory arrays use MLOAD
(3 gas)
calldata arrays use CALLDATALOAD
(3 gas)
Caching the length changes each of these to a DUP<N>
(3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
Lines of Code:
x += y
costs more gas than x = x + y
for state variablesLines of Code:
#0 - c4-judge
2023-02-28T14:52:02Z
dmvt marked the issue as grade-b