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: 38/169
Findings: 2
Award: $467.36
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x3b, 0xAgro, 0xBeirao, 0xMirce, 0xNineDec, 0xRobocop, 0xSmartContract, 0xTraub, 0xWeiss, 2997ms, 41i3xn, Awesome, Aymen0909, Bauer, Bnke0x0, Breeje, Cryptor, DadeKuma, Deathstore, Deekshith99, DevABDee, DevTimSch, Dewaxindo, Diana, Ermaniwe, Guild_3, H0, IceBear, Inspectah, JDeryl, Kaiziron, Kaysoft, Kenshin, Mukund, Praise, RaymondFam, Rickard, Rolezn, Ruhum, Sathish9098, SkyWalkerMan, SleepingBugs, UdarTeam, Udsen, Walter, aashar, adeolu, apvlki, arialblack14, ast3ros, btk, chaduke, chandkommanaboyina, chrisdior4, climber2002, codetilda, cryptonue, cryptostellar5, csanuragjain, ddimitrov22, descharre, dharma09, doublesharp, eccentricexit, ethernomad, fs0c, georgits, halden, hansfriese, hashminer0725, immeas, lukris02, luxartvinsec, matrix_0wl, merlin, mookimgo, mrpathfindr, nadin, olegthegoat, pavankv, rbserver, rebase, savi0ur, sayan, scokaf, seeu, shark, simon135, tnevler, tsvetanovv, ulqiorra, ustas, waldenyan20, y1cunhui, yongskiws, yosuke
397.5374 USDC - $397.54
| Issue | Instances | |
|---|---|---|
| [L‑01] | Unchecked return value of low level call()/delegatecall() | 2 |
| [L‑02] | Upgradeable contract not initialized | 2 |
| [L‑03] | Loss of precision | 2 |
| [L‑04] | Signatures vulnerable to malleability attacks | 3 |
| [L‑05] | Owner can renounce while system is paused | 2 |
| [L‑06] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 1 |
| [L‑07] | Open TODOs | 1 |
Total: 13 instances over 7 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 2 |
| [N‑02] | Missing initializer modifier on constructor | 1 |
| [N‑03] | Unused file | 1 |
| [N‑04] | constants should be defined rather than using magic numbers | 36 |
| [N‑05] | Events that mark critical parameter changes should contain both the old and the new value | 2 |
| [N‑06] | Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) | 1 |
| [N‑07] | Lines are too long | 3 |
| [N‑08] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 9 |
| [N‑09] | Non-library/interface files should use fixed compiler versions, not floating ones | 16 |
| [N‑10] | Typos | 8 |
| [N‑11] | File is missing NatSpec | 14 |
| [N‑12] | NatSpec is incomplete | 18 |
| [N‑13] | Not using the named return variables anywhere in the function is confusing | 3 |
| [N‑14] | Consider using delete rather than assigning zero to clear values | 1 |
| [N‑15] | Contracts should have full test coverage | 1 |
| [N‑16] | Large or complicated code bases should implement fuzzing tests | 1 |
| [N‑17] | Function ordering does not follow the Solidity style guide | 33 |
| [N‑18] | Contract does not follow the Solidity style guide's suggested layout ordering | 35 |
| [N‑19] | Interfaces should be indicated with an I prefix in the contract name | 1 |
Total: 186 instances over 19 issues
call()/delegatecall()There are 2 instances of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol 444 address(strategy).delegatecall( 445 abi.encodeWithSignature("harvest()") 446: );
File: src/vault/AdminProxy.sol 24: return target.call(callData);
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user
There are 2 instances of this issue:
File: src/vault/Vault.sol /// @audit missing __ReentrancyGuard_init() /// @audit missing __Pausable_init() 26: contract Vault is
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
There are 2 instances of this issue:
File: src/utils/MultiRewardStaking.sol 359: return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
File: src/vault/Vault.sol 433 ? managementFee.mulDiv( 434 totalAssets() * (block.timestamp - feesUpdatedAt), 435 SECONDS_PER_YEAR, 436 Math.Rounding.Down 437: ) / 1e18
ecrecover() accepts as valid, two versions of signatures, meaning an attacker can use the same signature twice. Consider adding checks for signature malleability, or using OpenZeppelin's ECDSA library to perform the extra checks necessary in order to prevent this attack.
There are 3 instances of this issue:
File: src/utils/MultiRewardStaking.sol 459 address recoveredAddress = ecrecover( 460 keccak256( 461 abi.encodePacked( 462 "\x19\x01", 463 DOMAIN_SEPARATOR(), 464 keccak256( 465 abi.encode( 466 keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"), 467 owner, 468 spender, 469 value, 470 nonces[owner]++, 471 deadline 472 ) 473 ) 474 ) 475 ), 476 v, 477 r, 478 s 479: );
File: src/vault/adapter/abstracts/AdapterBase.sol 646 address recoveredAddress = ecrecover( 647 keccak256( 648 abi.encodePacked( 649 "\x19\x01", 650 DOMAIN_SEPARATOR(), 651 keccak256( 652 abi.encode( 653 keccak256( 654 "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 655 ), 656 owner, 657 spender, 658 value, 659 nonces[owner]++, 660 deadline 661 ) 662 ) 663 ) 664 ), 665 v, 666 r, 667 s 668: );
File: src/vault/Vault.sol 678 address recoveredAddress = ecrecover( 679 keccak256( 680 abi.encodePacked( 681 "\x19\x01", 682 DOMAIN_SEPARATOR(), 683 keccak256( 684 abi.encode( 685 keccak256( 686 "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 687 ), 688 owner, 689 spender, 690 value, 691 nonces[owner]++, 692 deadline 693 ) 694 ) 695 ) 696 ), 697 v, 698 r, 699 s 700: );
The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely
There are 2 instances of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol 574 function pause() external onlyOwner { 575 _protocolWithdraw(totalAssets(), totalSupply()); 576 // Update the underlying balance to prevent inflation attacks 577 underlyingBalance = 0; 578 _pause(); 579: }
File: src/vault/Vault.sol 643 function pause() external onlyOwner { 644 _pause(); 645: }
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There is 1 instance of this issue:
File: src/vault/Vault.sol 93 contractName = keccak256( 94 abi.encodePacked("Popcorn", name(), block.timestamp, "Vault") 95: );
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There is 1 instance of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol 516: // TODO use deterministic fee recipient proxy
__gap[50] storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There are 2 instances of this issue:
File: src/utils/MultiRewardStaking.sol 26: contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
File: src/vault/Vault.sol 26 contract Vault is 27 ERC20Upgradeable, 28 ReentrancyGuardUpgradeable, 29 PausableUpgradeable, 30: OwnedUpgradeable
initializer modifier on constructorOpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
There is 1 instance of this issue:
File: src/vault/Vault.sol 28: ReentrancyGuardUpgradeable,
The file is never imported by any other source file. If the file is needed for tests, it should be moved to a test directory
There is 1 instance of this issue:
File: src/interfaces/IEIP165.sol 1: // SPDX-License-Identifier: AGPL-3.0-only
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 36 instances of this issue:
File: src/interfaces/vault/IStrategy.sol /// @audit 8 9: function verifyAdapterSelectorCompatibility(bytes4[8] memory sigs) external;
File: src/interfaces/vault/ITemplateRegistry.sol /// @audit 8 20: bytes4[8] requiredSigs;
File: src/interfaces/vault/IVaultRegistry.sol /// @audit 8 17: address[8] swapTokenAddresses;
File: src/utils/MultiRewardEscrow.sol /// @audit 1e18 108: uint256 fee = Math.mulDiv(amount, feePerc, 1e18); /// @audit 1e17 211: if (tokenFees[i] >= 1e17) revert DontGetGreedy(tokenFees[i]);
File: src/utils/MultiRewardStaking.sol /// @audit 1e18 197: uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit 8 64: bytes4[8] memory _requiredSigs, /// @audit 8 68: (address, address, address, uint256, bytes4[8], bytes) /// @audit 1e18 85: highWaterMark = 1e18; /// @audit 8 479: function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal { /// @audit 1e18 531: uint256 shareValue = convertToAssets(1e18); /// @audit 1e36 538: 1e36, /// @audit 2e17 551: if (newFee > 2e17) revert InvalidPerformanceFee(newFee); /// @audit 1e18 565: uint256 shareValue = convertToAssets(1e18);
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit 8 41: (address, address, address, uint256, bytes4[8], bytes)
File: src/vault/VaultController.sol /// @audit 8 210: bytes4[8] memory requiredSigs; /// @audit 2e17 753: if (newFee > 2e17) revert InvalidPerformanceFee(newFee);
File: src/vault/Vault.sol /// @audit 1e18 144: assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down) /// @audit 1e18 183: 1e18 - depositFee, /// @audit 1e18 224: 1e18 - withdrawalFee, /// @audit 1e18 265: 1e18, /// @audit 1e18 330: assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down) /// @audit 1e18 345: 1e18 - depositFee, /// @audit 1e18 367: 1e18 - withdrawalFee, /// @audit 1e18 389: 1e18, /// @audit 1e18 437: ) / 1e18 /// @audit 1e18 449: uint256 shareValue = convertToAssets(1e18); /// @audit 1e36 456: 1e36, /// @audit 1e18 484: uint256 shareValue = convertToAssets(1e18); /// @audit 1e18 498: highWaterMark = convertToAssets(1e18); /// @audit 1e18 527: newFees.deposit >= 1e18 || /// @audit 1e18 528: newFees.withdrawal >= 1e18 || /// @audit 1e18 529: newFees.management >= 1e18 || /// @audit 1e18 530: newFees.performance >= 1e18 /// @audit 3 619: uint256 public quitPeriod = 3 days; /// @audit 7 630: if (_quitPeriod < 1 days || _quitPeriod > 7 days)
This should especially be done if the new value is not required to be different from the old value
There are 2 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit setFees() 214: emit FeeSet(tokens[i], tokenFees[i]);
File: src/vault/Vault.sol /// @audit setQuitPeriod() 635: emit QuitPeriodSet(quitPeriod);
1e18) rather than exponentiation (e.g. 10**18)While the compiler knows to optimize away the exponentiation, it's still better coding practice to use idioms that do not require compiler optimization, if they exist
There is 1 instance of this issue:
File: src/vault/adapter/yearn/YearnAdapter.sol 25: uint256 constant DEGRADATION_COEFFICIENT = 10**18;
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
There are 3 instances of this issue:
File: src/utils/MultiRewardEscrow.sol 49: * @dev there is no check to ensure that all escrows are owned by the same account. Make sure to account for this either by only sending ids for a specific account or by filtering the Escrows by account later on.
File: src/utils/MultiRewardStaking.sol 7: import { ERC4626Upgradeable, ERC20Upgradeable, IERC20Upgradeable as IERC20, IERC20MetadataUpgradeable as IERC20Metadata } from "openzeppelin-contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
File: src/vault/adapter/abstracts/AdapterBase.sol 6: import {ERC4626Upgradeable, IERC20Upgradeable as IERC20, IERC20MetadataUpgradeable as IERC20Metadata, ERC20Upgradeable as ERC20} from "openzeppelin-contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 9 instances of this issue:
File: src/interfaces/IMultiRewardStaking.sol 16: uint64 ONE;
File: src/utils/MultiRewardStaking.sol 274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64(); 438: uint256 internal INITIAL_CHAIN_ID; 439: bytes32 internal INITIAL_DOMAIN_SEPARATOR;
File: src/vault/adapter/abstracts/AdapterBase.sol 517: address FEE_RECIPIENT = address(0x4444); 625: uint256 internal INITIAL_CHAIN_ID; 626: bytes32 internal INITIAL_DOMAIN_SEPARATOR;
File: src/vault/Vault.sol 657: uint256 internal INITIAL_CHAIN_ID; 658: bytes32 internal INITIAL_DOMAIN_SEPARATOR;
There are 16 instances of this issue:
File: src/utils/EIP165.sol 4: pragma solidity ^0.8.15;
File: src/utils/MultiRewardEscrow.sol 4: pragma solidity ^0.8.15;
File: src/utils/MultiRewardStaking.sol 4: pragma solidity ^0.8.15;
File: src/vault/adapter/abstracts/OnlyStrategy.sol 4: pragma solidity ^0.8.15;
File: src/vault/adapter/abstracts/WithRewards.sol 4: pragma solidity ^0.8.15;
File: src/vault/adapter/beefy/BeefyAdapter.sol 4: pragma solidity ^0.8.15;
File: src/vault/adapter/yearn/YearnAdapter.sol 4: pragma solidity ^0.8.15;
File: src/vault/AdminProxy.sol 4: pragma solidity ^0.8.15;
File: src/vault/CloneFactory.sol 4: pragma solidity ^0.8.15;
File: src/vault/CloneRegistry.sol 4: pragma solidity ^0.8.15;
File: src/vault/DeploymentController.sol 4: pragma solidity ^0.8.15;
File: src/vault/PermissionRegistry.sol 4: pragma solidity ^0.8.15;
File: src/vault/TemplateRegistry.sol 4: pragma solidity ^0.8.15;
File: src/vault/VaultController.sol 3: pragma solidity ^0.8.15;
File: src/vault/VaultRegistry.sol 4: pragma solidity ^0.8.15;
File: src/vault/Vault.sol 4: pragma solidity ^0.8.15;
There are 8 instances of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit overriden 24: * All specific interactions for the underlying protocol need to be overriden in the actual implementation. /// @audit aftwards 477: * @dev It aftwards sets up anything required by the strategy to call `harvest()` like approvals etc.
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit profts 100: /// @notice The amount of assets that are free to be withdrawn from the yVault after locked profts.
File: src/vault/AdminProxy.sol /// @audit Ownes 11: * @notice Ownes contracts in the vault ecosystem to allow for easy ownership changes.
File: src/vault/VaultController.sol /// @audit ownes 47: * @param _adminProxy `AdminProxy` ownes contracts in the vault ecosystem. /// @audit auxiliery 87: * @dev This function is the one stop solution to create a new vault with all necessary admin functions or auxiliery contracts. /// @audit DEPLYOMENT 816: DEPLYOMENT CONTROLLER LOGIC /// @audit auxilary 829: * @notice Sets a new `DeploymentController` and saves its auxilary contracts. Caller must be owner.
There are 14 instances of this issue:
File: src/interfaces/IEIP165.sol
File: src/interfaces/vault/IAdapter.sol
File: src/interfaces/vault/IAdminProxy.sol
File: src/interfaces/vault/ICloneFactory.sol
File: src/interfaces/vault/ICloneRegistry.sol
File: src/interfaces/vault/IDeploymentController.sol
File: src/interfaces/vault/IERC4626.sol
File: src/interfaces/vault/IPermissionRegistry.sol
File: src/interfaces/vault/IStrategy.sol
File: src/interfaces/vault/IVaultController.sol
File: src/interfaces/vault/IWithRewards.sol
File: src/utils/EIP165.sol
File: src/vault/adapter/beefy/IBeefy.sol
File: src/vault/adapter/yearn/IYearn.sol
There are 18 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit Missing: '@return' 49 * @dev there is no check to ensure that all escrows are owned by the same account. Make sure to account for this either by only sending ids for a specific account or by filtering the Escrows by account later on. 50 */ 51: function getEscrows(bytes32[] calldata escrowIds) external view returns (Escrow[] memory) {
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit Missing: '@return' 108 * @param receiver Receiver of the shares. 109 */ 110 function deposit(uint256 assets, address receiver) 111 public 112 virtual 113 override 114: returns (uint256) /// @audit Missing: '@return' 127 * @param receiver Receiver of the shares. 128 */ 129 function mint(uint256 shares, address receiver) 130 public 131 virtual 132 override 133: returns (uint256) /// @audit Missing: '@return' 171 * @param owner Owner of the shares. 172 */ 173 function withdraw( 174 uint256 assets, 175 address receiver, 176 address owner 177: ) public virtual override returns (uint256) { /// @audit Missing: '@return' 191 * @param owner Owner of the shares. 192 */ 193 function redeem( 194 uint256 shares, 195 address receiver, 196 address owner 197: ) public virtual override returns (uint256) { /// @audit Missing: '@param address' 399 /** 400 * @return Maximum amount of vault shares that may be minted to given address. Delegates to adapter. 401 * @dev Return 0 if paused since no further deposits are allowed. 402 * @dev Override this function if the underlying protocol has a unique deposit logic and/or deposit fees. 403 */ 404 function maxDeposit(address) 405 public 406 view 407 virtual 408 override 409: returns (uint256) /// @audit Missing: '@param address' 414 /** 415 * @return Maximum amount of vault shares that may be minted to given address. Delegates to adapter. 416 * @dev Return 0 if paused since no further deposits are allowed. 417 * @dev Override this function if the underlying protocol has a unique deposit logic and/or deposit fees. 418 */ 419: function maxMint(address) public view virtual override returns (uint256) {
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit Missing: '@param bytes' 27 /** 28 * @notice Initialize a new Yearn Adapter. 29 * @param adapterInitData Encoded data for the base adapter initialization. 30 * @param externalRegistry Yearn registry address. 31 * @dev This function is called by the factory contract when deploying a new vault. 32 * @dev The yearn registry will be used given the `asset` from `adapterInitData` to find the latest yVault. 33 */ 34 function initialize( 35 bytes memory adapterInitData, 36 address externalRegistry, 37 bytes memory 38: ) external initializer {
File: src/vault/CloneFactory.sol /// @audit Missing: '@return' 37 * @param data The data to pass to the clone's initializer. 38 */ 39: function deploy(Template calldata template, bytes calldata data) external onlyOwner returns (address clone) {
File: src/vault/DeploymentController.sol /// @audit Missing: '@return' 97 * @dev Registers the clone in `CloneRegistry`. 98 */ 99 function deploy( 100 bytes32 templateCategory, 101 bytes32 templateId, 102 bytes calldata data 103: ) external onlyOwner returns (address clone) {
File: src/vault/VaultController.sol /// @audit Missing: '@return' 87 * @dev This function is the one stop solution to create a new vault with all necessary admin functions or auxiliery contracts. 88 */ 89 function deployVault( 90 VaultInitParams memory vaultData, 91 DeploymentArgs memory adapterData, 92 DeploymentArgs memory strategyData, 93 address staking, 94 bytes memory rewardsData, 95 VaultMetadata memory metadata, 96 uint256 initialDeposit 97: ) external canCreate returns (address vault) { /// @audit Missing: '@param initialDeposit' /// @audit Missing: '@return' 180 /** 181 * @notice Deploy a new Adapter with our without a strategy. Caller must be owner. 182 * @param asset Asset which will be used by the adapter. 183 * @param adapterData Encoded adapter init data. 184 * @param strategyData Encoded strategy init data. 185 */ 186 function deployAdapter( 187 IERC20 asset, 188 DeploymentArgs memory adapterData, 189 DeploymentArgs memory strategyData, 190 uint256 initialDeposit 191: ) external canCreate returns (address adapter) { /// @audit Missing: '@return' 276 * @dev Deploys `MultiRewardsStaking` based on the latest templateTemplateKey. 277 */ 278: function deployStaking(IERC20 asset) external canCreate returns (address) {
File: src/vault/Vault.sol /// @audit Missing: '@param caller' 398 /// @return Maximum amount of underlying `asset` token that may be deposited for a given address. Delegates to adapter. 399: function maxDeposit(address caller) public view returns (uint256) { /// @audit Missing: '@param caller' 403 /// @return Maximum amount of vault shares that may be minted to given address. Delegates to adapter. 404: function maxMint(address caller) external view returns (uint256) { /// @audit Missing: '@param caller' 408 /// @return Maximum amount of underlying `asset` token that can be withdrawn by `caller` address. Delegates to adapter. 409: function maxWithdraw(address caller) external view returns (uint256) { /// @audit Missing: '@param caller' 413 /// @return Maximum amount of shares that may be redeemed by `caller` address. Delegates to adapter. 414: function maxRedeem(address caller) external view returns (uint256) {
Consider changing the variable to be an unnamed one
There are 3 instances of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit shares 380 function _convertToShares(uint256 assets, Math.Rounding rounding) 381 internal 382 view 383 virtual 384 override 385: returns (uint256 shares)
File: src/vault/AdminProxy.sol /// @audit success /// @audit returndata 19 function execute(address target, bytes calldata callData) 20 external 21 onlyOwner 22: returns (bool success, bytes memory returndata)
delete rather than assigning zero to clear valuesThe delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic
There is 1 instance of this issue:
File: src/utils/MultiRewardStaking.sol 186: accruedRewards[user][_rewardTokens[i]] = 0;
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 33 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit _getClaimableAmount() came earlier 207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {
File: src/utils/MultiRewardStaking.sol /// @audit decimals() came earlier 75: function deposit(uint256 _amount) external returns (uint256) { /// @audit _transfer() came earlier 170: function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) { /// @audit _lockToken() came earlier 243 function addRewardToken( 244 IERC20 rewardToken, 245 uint160 rewardsPerSecond, 246 uint256 amount, 247 bool useEscrow, 248 uint192 escrowPercentage, 249 uint32 escrowDuration, 250 uint32 offset 251: ) external onlyOwner { /// @audit _calcRewardsEnd() came earlier 362: function getAllRewardsTokens() external view returns (IERC20[] memory) { /// @audit _accrueUser() came earlier 445 function permit( 446 address owner, 447 address spender, 448 uint256 value, 449 uint256 deadline, 450 uint8 v, 451 bytes32 r, 452: bytes32 s
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit __AdapterBase_init() came earlier 89 function decimals() 90 public 91 view 92 override(IERC20Metadata, ERC20) 93: returns (uint8) /// @audit _deposit() came earlier 173 function withdraw( 174 uint256 assets, 175 address receiver, 176 address owner 177: ) public virtual override returns (uint256) { /// @audit _withdraw() came earlier 247: function totalAssets() public view override returns (uint256) { /// @audit _underlyingBalance() came earlier 271 function convertToUnderlyingShares(uint256 assets, uint256 shares) 272 public 273 view 274 virtual 275: returns (uint256) /// @audit _previewDeposit() came earlier 304 function previewMint(uint256 shares) 305 public 306 view 307 virtual 308 override 309: returns (uint256) /// @audit _previewMint() came earlier 329 function previewWithdraw(uint256 assets) 330 public 331 view 332 virtual 333 override 334: returns (uint256) /// @audit _previewWithdraw() came earlier 353 function previewRedeem(uint256 shares) 354 public 355 view 356 virtual 357 override 358: returns (uint256) /// @audit _convertToShares() came earlier 404 function maxDeposit(address) 405 public 406 view 407 virtual 408 override 409: returns (uint256) /// @audit _verifyAndSetupStrategy() came earlier 500: function setHarvestCooldown(uint256 newCooldown) external onlyOwner { /// @audit setPerformanceFee() came earlier 574: function pause() external onlyOwner { /// @audit _protocolWithdraw() came earlier 610 function supportsInterface(bytes4 interfaceId) 611 public 612 view 613 virtual 614 override 615: returns (bool)
File: src/vault/adapter/beefy/BeefyAdapter.sol /// @audit _underlyingBalance() came earlier 122 function convertToUnderlyingShares(uint256, uint256 shares) 123 public 124 view 125 override 126: returns (uint256) /// @audit convertToUnderlyingShares() came earlier 136: function rewardTokens() external view override returns (address[] memory) { /// @audit _protocolWithdraw() came earlier 221: function claim() public override onlyStrategy {
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit _calculateLockedProfit() came earlier 129 function convertToUnderlyingShares(uint256, uint256 shares) 130 public 131 view 132 override 133: returns (uint256)
File: src/vault/VaultController.sol /// @audit _handleInitialDeposit() came earlier 186 function deployAdapter( 187 IERC20 asset, 188 DeploymentArgs memory adapterData, 189 DeploymentArgs memory strategyData, 190 uint256 initialDeposit 191: ) external canCreate returns (address adapter) { /// @audit _deployStrategy() came earlier 278: function deployStaking(IERC20 asset) external canCreate returns (address) { /// @audit _deployStaking() came earlier 313: function proposeVaultAdapters(address[] calldata vaults, IERC4626[] calldata newAdapter) external { /// @audit _registerVault() came earlier 408: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner { /// @audit addStakingRewardsTokens() came earlier 483 function changeStakingRewardsSpeeds( 484 address[] calldata vaults, 485 IERC20[] calldata rewardTokens, 486: uint160[] calldata rewardsSpeeds /// @audit _verifyEqualArrayLength() came earlier 723: function nominateNewAdminProxyOwner(address newOwner) external onlyOwner { /// @audit _setDeploymentController() came earlier 864: function setActiveTemplateId(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
File: src/vault/Vault.sol /// @audit deposit() came earlier 160: function mint(uint256 shares) external returns (uint256) { /// @audit withdraw() came earlier 242: function redeem(uint256 shares) external returns (uint256) { /// @audit previewMint() came earlier 358 function previewWithdraw(uint256 assets) 359 external 360 view 361: returns (uint256 shares) /// @audit maxDeposit() came earlier 404: function maxMint(address caller) external view returns (uint256) { /// @audit accruedPerformanceFee() came earlier 473 function takeManagementAndPerformanceFees() 474 external 475 nonReentrant 476: takeFees
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There are 35 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit function getEscrows came earlier 64 mapping(bytes32 => Escrow) public escrows; 65 66 // User => Escrows 67 mapping(address => bytes32[]) public userEscrowIds; 68 // User => RewardsToken => Escrows 69: mapping(address => mapping(IERC20 => bytes32[])) public userEscrowIdsByToken; /// @audit function lock came earlier 136: event RewardsClaimed(IERC20 indexed token, address indexed account, uint256 amount); /// @audit function _getClaimableAmount came earlier 191: address public feeRecipient;
File: src/utils/MultiRewardStaking.sol /// @audit function _transfer came earlier 157: IMultiRewardEscrow public escrow; /// @audit function _lockToken came earlier 208: IERC20[] public rewardTokens; /// @audit function getAllRewardsTokens came earlier 371 modifier accrueRewards(address _caller, address _receiver) { 372 IERC20[] memory _rewardTokens = rewardTokens; 373 for (uint8 i; i < _rewardTokens.length; i++) { 374 IERC20 rewardToken = _rewardTokens[i]; 375 RewardInfo memory rewards = rewardInfos[rewardToken]; 376 377 if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards)); 378 _accrueUser(_receiver, rewardToken); 379 380 // If a deposit/withdraw operation gets called for another user we should accrue for both of them to avoid potential issues like in the Convex-Vulnerability 381 if (_receiver != _caller) _accrueUser(_caller, rewardToken); 382 } 383 _; 384: } /// @audit function _accrueUser came earlier 438: uint256 internal INITIAL_CHAIN_ID;
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit function _withdraw came earlier 241: uint256 internal underlyingBalance; /// @audit function maxMint came earlier 427: IStrategy public strategy; /// @audit function _verifyAndSetupStrategy came earlier 489: uint256 public harvestCooldown; /// @audit function setHarvestCooldown came earlier 513: uint256 public performanceFee; /// @audit function setPerformanceFee came earlier 559 modifier takeFees() { 560 _; 561 562 uint256 fee = accruedPerformanceFee(); 563 if (fee > 0) _mint(FEE_RECIPIENT, convertToShares(fee)); 564 565 uint256 shareValue = convertToAssets(1e18); 566 if (shareValue > highWaterMark) highWaterMark = shareValue; 567: } /// @audit function supportsInterface came earlier 625: uint256 internal INITIAL_CHAIN_ID;
File: src/vault/CloneFactory.sol /// @audit function constructor came earlier 29: event Deployment(address indexed clone);
File: src/vault/CloneRegistry.sol /// @audit function constructor came earlier 28: mapping(address => bool) public cloneExists;
File: src/vault/PermissionRegistry.sol /// @audit function constructor came earlier 26: mapping(address => Permission) public permissions;
File: src/vault/TemplateRegistry.sol /// @audit function constructor came earlier 31 mapping(bytes32 => mapping(bytes32 => Template)) public templates; 32 mapping(bytes32 => bytes32[]) public templateIds; 33 mapping(bytes32 => bool) public templateExists; 34 35: mapping(bytes32 => bool) public templateCategoryExists; /// @audit function addTemplate came earlier 88 event TemplateEndorsementToggled( 89 bytes32 templateCategory, 90 bytes32 templateId, 91 bool oldEndorsement, 92 bool newEndorsement 93: );
File: src/vault/VaultController.sol /// @audit function constructor came earlier 76: event VaultDeployed(address indexed vault, address indexed staking, address indexed adapter); /// @audit function changeVaultFees came earlier 387: IVaultRegistry public vaultRegistry; /// @audit function fundStakingRewards came earlier 535: IMultiRewardEscrow public escrow; /// @audit function _verifyEqualArrayLength came earlier 704 modifier canCreate() { 705 if ( 706 permissionRegistry.endorsed(address(1)) 707 ? !permissionRegistry.endorsed(msg.sender) 708 : permissionRegistry.rejected(msg.sender) 709 ) revert NotAllowed(msg.sender); 710 _; 711: } /// @audit modifier canCreate came earlier 717: IAdminProxy public adminProxy; /// @audit function acceptAdminProxyOwnership came earlier 739: uint256 public performanceFee; /// @audit function setAdapterPerformanceFees came earlier 779: uint256 public harvestCooldown; /// @audit function setAdapterHarvestCooldowns came earlier 819: IDeploymentController public deploymentController; /// @audit function _setDeploymentController came earlier 851: mapping(bytes32 => bytes32) public activeTemplateId;
File: src/vault/VaultRegistry.sol /// @audit function constructor came earlier 28 mapping(address => VaultMetadata) public metadata; 29 30 // asset to vault addresses 31: mapping(address => address[]) public vaultsByAsset;
File: src/vault/Vault.sol /// @audit function decimals came earlier 108 event Deposit( 109 address indexed caller, 110 address indexed owner, 111 uint256 assets, 112 uint256 shares 113: ); /// @audit function accruedPerformanceFee came earlier 466: uint256 public highWaterMark = 1e18; /// @audit function takeManagementAndPerformanceFees came earlier 480 modifier takeFees() { 481 uint256 managementFee = accruedManagementFee(); 482 uint256 totalFee = managementFee + accruedPerformanceFee(); 483 uint256 currentAssets = totalAssets(); 484 uint256 shareValue = convertToAssets(1e18); 485 486 if (shareValue > highWaterMark) highWaterMark = shareValue; 487 488 if (managementFee > 0) feesUpdatedAt = block.timestamp; 489 490 if (totalFee > 0 && currentAssets > 0) 491 _mint(feeRecipient, convertToShares(totalFee)); 492 493 _; 494: } /// @audit modifier syncFeeCheckpoint came earlier 505: VaultFees public fees; /// @audit function setFeeRecipient came earlier 565: IERC4626 public adapter; /// @audit function changeAdapter came earlier 619: uint256 public quitPeriod = 3 days; /// @audit function unpause came earlier 657: uint256 internal INITIAL_CHAIN_ID;
I prefix in the contract nameThere is 1 instance of this issue:
File: src/vault/adapter/yearn/IYearn.sol 8: interface VaultAPI is IERC20 {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [L‑01] | Unsafe use of transfer()/transferFrom() with IERC20 | 1 |
| [L‑02] | Return values of transfer()/transferFrom() not checked | 1 |
| [L‑03] | safeApprove() is deprecated | 1 |
| [L‑04] | Missing checks for address(0x0) when assigning values to address state variables | 1 |
Total: 4 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | Return values of approve() not checked | 5 |
| [N‑02] | public functions not called by the contract should be declared external instead | 4 |
| [N‑03] | Event is missing indexed fields | 27 |
Total: 36 instances over 3 issues
transfer()/transferFrom() with IERC20Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions on L1 do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert (see this link for a test case). Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead
There is 1 instance of this issue:
File: src/vault/VaultController.sol /// @audit (valid but excluded finding) 457: IERC20(rewardsToken).transferFrom(msg.sender, address(adminProxy), amount);
transfer()/transferFrom() not checkedNot all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment
There is 1 instance of this issue:
File: src/vault/VaultController.sol /// @audit (valid but excluded finding) 457: IERC20(rewardsToken).transferFrom(msg.sender, address(adminProxy), amount);
safeApprove() is deprecatedDeprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to no longer has this function, you'll encounter unnecessary delays in porting and testing replacement contracts.
There is 1 instance of this issue:
File: src/utils/MultiRewardStaking.sol /// @audit (valid but excluded finding) 271: rewardToken.safeApprove(address(escrow), type(uint256).max);
address(0x0) when assigning values to address state variablesThere is 1 instance of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit (valid but excluded finding) 31: feeRecipient = _feeRecipient;
approve() not checkedNot all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything
There are 5 instances of this issue:
File: src/vault/adapter/beefy/BeefyAdapter.sol /// @audit (valid but excluded finding) 80: IERC20(asset()).approve(_beefyVault, type(uint256).max); /// @audit (valid but excluded finding) 83: IERC20(_beefyVault).approve(_beefyBooster, type(uint256).max);
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit (valid but excluded finding) 54: IERC20(_asset).approve(address(yVault), type(uint256).max);
File: src/vault/VaultController.sol /// @audit (valid but excluded finding) 171: asset.approve(address(target), initialDeposit); /// @audit (valid but excluded finding) 456: IERC20(rewardsToken).approve(staking, type(uint256).max);
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 4 instances of this issue:
File: src/vault/Vault.sol /// @audit (valid but excluded finding) 323 function previewDeposit(uint256 assets) 324 public 325 view 326: returns (uint256 shares) /// @audit (valid but excluded finding) 340: function previewMint(uint256 shares) public view returns (uint256 assets) { /// @audit (valid but excluded finding) 380 function previewRedeem(uint256 shares) 381 public 382 view 383: returns (uint256 assets) /// @audit (valid but excluded finding) 399: function maxDeposit(address caller) public view returns (uint256) {
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 27 instances of this issue:
File: src/interfaces/vault/IERC4626.sol /// @audit (valid but excluded finding) 8: event Deposit(address indexed sender, address indexed owner, uint256 assets, uint256 shares);
File: src/utils/MultiRewardEscrow.sol /// @audit (valid but excluded finding) 73: event Locked(IERC20 indexed token, address indexed account, uint256 amount, uint32 duration, uint32 offset); /// @audit (valid but excluded finding) 136: event RewardsClaimed(IERC20 indexed token, address indexed account, uint256 amount); /// @audit (valid but excluded finding) 196: event FeeSet(IERC20 indexed token, uint256 amount);
File: src/utils/MultiRewardStaking.sol /// @audit (valid but excluded finding) 159: event RewardsClaimed(address indexed user, IERC20 rewardToken, uint256 amount, bool escrowed); /// @audit (valid but excluded finding) 220: event RewardInfoUpdate(IERC20 rewardToken, uint160 rewardsPerSecond, uint32 rewardsEndTimestamp);
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit (valid but excluded finding) 491: event HarvestCooldownChanged(uint256 oldCooldown, uint256 newCooldown); /// @audit (valid but excluded finding) 519: event PerformanceFeeChanged(uint256 oldFee, uint256 newFee);
File: src/vault/CloneRegistry.sol /// @audit (valid but excluded finding) 33: event CloneAdded(address clone);
File: src/vault/PermissionRegistry.sol /// @audit (valid but excluded finding) 28: event PermissionSet(address target, bool newEndorsement, bool newRejection);
File: src/vault/TemplateRegistry.sol /// @audit (valid but excluded finding) 38: event TemplateCategoryAdded(bytes32 templateCategory); /// @audit (valid but excluded finding) 39: event TemplateAdded(bytes32 templateCategory, bytes32 templateId, address implementation); /// @audit (valid but excluded finding) 40: event TemplateUpdated(bytes32 templateCategory, bytes32 templateId); /// @audit (valid but excluded finding) 88 event TemplateEndorsementToggled( 89 bytes32 templateCategory, 90 bytes32 templateId, 91 bool oldEndorsement, 92 bool newEndorsement 93: );
File: src/vault/VaultController.sol /// @audit (valid but excluded finding) 741: event PerformanceFeeChanged(uint256 oldFee, uint256 newFee); /// @audit (valid but excluded finding) 781: event HarvestCooldownChanged(uint256 oldCooldown, uint256 newCooldown); /// @audit (valid but excluded finding) 824: event DeploymentControllerChanged(address oldController, address newController); /// @audit (valid but excluded finding) 853: event ActiveTemplateIdChanged(bytes32 oldKey, bytes32 newKey);
File: src/vault/VaultRegistry.sol /// @audit (valid but excluded finding) 36: event VaultAdded(address vaultAddress, string metadataCID);
File: src/vault/Vault.sol /// @audit (valid but excluded finding) 42: event VaultInitialized(bytes32 contractName, address indexed asset); /// @audit (valid but excluded finding) 108 event Deposit( 109 address indexed caller, 110 address indexed owner, 111 uint256 assets, 112 uint256 shares 113: ); /// @audit (valid but excluded finding) 512: event NewFeesProposed(VaultFees newFees, uint256 timestamp); /// @audit (valid but excluded finding) 513: event ChangedFees(VaultFees oldFees, VaultFees newFees); /// @audit (valid but excluded finding) 514: event FeeRecipientUpdated(address oldFeeRecipient, address newFeeRecipient); /// @audit (valid but excluded finding) 569: event NewAdapterProposed(IERC4626 newAdapter, uint256 timestamp); /// @audit (valid but excluded finding) 570: event ChangedAdapter(IERC4626 oldAdapter, IERC4626 newAdapter); /// @audit (valid but excluded finding) 621: event QuitPeriodSet(uint256 quitPeriod);
#0 - c4-judge
2023-02-28T17:40:37Z
dmvt marked the issue as grade-a
#1 - c4-judge
2023-03-01T22:34:36Z
dmvt marked the issue as selected for report
#2 - dmvt
2023-03-07T12:46:56Z
The report is complete and accurate with the following exceptions: L-06 - Out of Scope (automated finding) N-01 - Should be Low Risk N-02 - Should be Low Risk
🌟 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
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Remove or replace unused state variables | 1 | - |
| [G‑02] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 3 | - |
| [G‑03] | State variables only set in the constructor should be declared immutable | 8 | 16776 |
| [G‑04] | State variables can be packed into fewer storage slots | 1 | - |
| [G‑05] | State variables should be cached in stack variables rather than re-reading them from storage | 21 | 2037 |
| [G‑06] | Multiple accesses of a mapping/array should use a local variable cache | 5 | 210 |
| [G‑07] | The result of function calls should be cached rather than re-calling the function | 1 | - |
| [G‑08] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 2 | 226 |
| [G‑09] | internal functions only called once can be inlined to save gas | 13 | 260 |
| [G‑10] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 2 | 170 |
| [G‑11] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 21 | 1260 |
| [G‑12] | keccak256() should only need to be called on a specific string literal once | 9 | 378 |
| [G‑13] | Optimize names to save gas | 26 | 572 |
| [G‑14] | Remove unused local variable | 1 | - |
| [G‑15] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 2 | - |
| [G‑16] | Using private rather than public for constants, saves gas | 4 | - |
| [G‑17] | Superfluous event fields | 2 | - |
| [G‑18] | Functions guaranteed to revert when called by normal users can be marked payable | 9 | 189 |
| [G‑19] | Constructors can be marked payable | 9 | 189 |
| [G‑20] | Don't use _msgSender() if not supporting EIP-2771 | 4 | 64 |
Total: 144 instances over 20 issues with 22331 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
Saves a storage slot. If the variable is assigned a non-zero value, saves Gsset (20000 gas). If it's assigned a zero value, saves Gsreset (2900 gas). If the variable remains unassigned, there is no gas savings unless the variable is public, in which case the compiler-generated non-payable getter deployment cost is saved. If the state variable is overriding an interface's public function, mark the variable as constant or immutable so that it does not use a storage slot
There is 1 instance of this issue:
File: src/vault/Vault.sol 467: uint256 public assetsCheckpoint;
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves 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.
There are 3 instances of this issue:
File: src/utils/MultiRewardEscrow.sol 64 mapping(bytes32 => Escrow) public escrows; 65 66 // User => Escrows 67 mapping(address => bytes32[]) public userEscrowIds; 68 // User => RewardsToken => Escrows 69: mapping(address => mapping(IERC20 => bytes32[])) public userEscrowIdsByToken;
File: src/utils/MultiRewardStaking.sol 216 mapping(address => mapping(IERC20 => uint256)) public userIndex; 217 // user => rewardToken -> accruedRewards 218: mapping(address => mapping(IERC20 => uint256)) public accruedRewards;
File: src/vault/TemplateRegistry.sol 31 mapping(bytes32 => mapping(bytes32 => Template)) public templates; 32 mapping(bytes32 => bytes32[]) public templateIds; 33 mapping(bytes32 => bool) public templateExists; 34 35: mapping(bytes32 => bool) public templateCategoryExists;
immutableAvoids 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).
While strings are not value types, and therefore cannot be immutable/constant if not hard-coded outside of the constructor, the same behavior can be achieved by making the current contract abstract with virtual functions for the string accessors, and having a child contract override the functions with the hard-coded implementation-specific values.
There are 8 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit feeRecipient (constructor) 31: feeRecipient = _feeRecipient;
File: src/vault/DeploymentController.sol /// @audit cloneFactory (constructor) 41: cloneFactory = _cloneFactory; /// @audit cloneRegistry (constructor) 42: cloneRegistry = _cloneRegistry; /// @audit templateRegistry (constructor) 43: templateRegistry = _templateRegistry;
File: src/vault/VaultController.sol /// @audit vaultRegistry (constructor) 62: vaultRegistry = _vaultRegistry; /// @audit escrow (constructor) 64: escrow = _escrow; /// @audit adminProxy (constructor) 61: adminProxy = _adminProxy; /// @audit permissionRegistry (constructor) 63: permissionRegistry = _permissionRegistry;
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
There is 1 instance of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit Variable ordering with 11 slots instead of the current 12: /// uint256(32):underlyingBalance, bytes(32):strategyConfig, uint256(32):lastHarvest, uint256(32):harvestCooldown, uint256(32):performanceFee, uint256(32):highWaterMark, uint256(32):INITIAL_CHAIN_ID, bytes32(32):INITIAL_DOMAIN_SEPARATOR, mapping(32):nonces, address(20):strategy, uint8(1):_decimals, address(20):FEE_RECIPIENT 38: uint8 internal _decimals;
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.
There are 21 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit nonce on line 102 104: bytes32 id = keccak256(abi.encodePacked(token, account, amount, nonce));
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit strategy on line 440 444: address(strategy).delegatecall( /// @audit strategy on line 480 481: strategy.verifyAdapterCompatibility(strategyConfig); /// @audit strategy on line 481 /// @audit strategyConfig on line 481 482: strategy.setUp(strategyConfig);
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit yVault on line 45 54: IERC20(_asset).approve(address(yVault), type(uint256).max); /// @audit yVault on line 90 95: yVault.totalSupply(), /// @audit yVault on line 110 110: return IERC20(asset()).balanceOf(address(yVault)) + yVault.totalDebt(); /// @audit yVault on line 115 116: yVault.lockedProfitDegradation(); /// @audit yVault on line 116 119: uint256 lockedProfit = yVault.lockedProfit();
File: src/vault/VaultController.sol /// @audit adminProxy on line 124 126: (bool success, bytes memory returnData) = adminProxy.execute( /// @audit adminProxy on line 230 238: adminProxy.execute(adapter, abi.encodeWithSelector(IAdapter.setPerformanceFee.selector, performanceFee)); /// @audit adminProxy on line 457 459: (success, returnData) = adminProxy.execute( /// @audit permissionRegistry on line 682 683: ? !permissionRegistry.endorsed(token) /// @audit permissionRegistry on line 683 684: : permissionRegistry.rejected(token)
File: src/vault/Vault.sol /// @audit highWaterMark on line 448 453: performanceFee > 0 && shareValue > highWaterMark /// @audit highWaterMark on line 453 455: (shareValue - highWaterMark) * totalSupply(), /// @audit adapter on line 286 286: return adapter.convertToAssets(adapter.balanceOf(address(this))); /// @audit adapter on line 598 599: adapter.balanceOf(address(this)), /// @audit adapter on line 608 610: asset.approve(address(adapter), type(uint256).max); /// @audit adapter on line 610 612: adapter.deposit(asset.balanceOf(address(this)), address(this));
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
There are 5 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit escrows[escrowId] on line 141 141: return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0; /// @audit escrows[escrowId] on line 162 163: escrows[escrowId].lastUpdateTime = block.timestamp.safeCastTo32();
File: src/utils/MultiRewardStaking.sol /// @audit rewardInfos[rewardToken] on line 313 314: rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp; /// @audit rewardInfos[rewardToken] on line 343 346: rewardInfos[rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32(); /// @audit rewardInfos[_rewardToken] on line 408 409: rewardInfos[_rewardToken].lastUpdatedTimestamp = block.timestamp.safeCastTo32();
The instances below point to the second+ call of the function within a single function
There is 1 instance of this issue:
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit yVault.totalSupply() on line 90 95: yVault.totalSupply(),
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 2 instances of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol 158: underlyingBalance += _underlyingBalance() - underlyingBalance_; 225: underlyingBalance -= underlyingBalance_ - _underlyingBalance();
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 13 instances of this issue:
File: src/utils/MultiRewardStaking.sol 191 function _lockToken( 192 address user, 193 IERC20 rewardToken, 194 uint256 rewardAmount, 195: EscrowInfo memory escrowInfo
File: src/vault/adapter/yearn/YearnAdapter.sol 89: function _shareValue(uint256 yShares) internal view returns (uint256) { 101: function _freeFunds() internal view returns (uint256) { 109: function _yTotalAssets() internal view returns (uint256) { 114: function _calculateLockedProfit() internal view returns (uint256) {
File: src/vault/VaultController.sol 120 function _deployVault(VaultInitParams memory vaultData, IDeploymentController _deploymentController) 121 internal 122: returns (address vault) 141 function _registerCreatedVault( 142 address vault, 143 address staking, 144: VaultMetadata memory metadata 154: function _handleVaultStakingRewards(address vault, bytes memory rewardsData) internal { 225 function __deployAdapter( 226 DeploymentArgs memory adapterData, 227 bytes memory baseAdapterData, 228 IDeploymentController _deploymentController 229: ) internal returns (address adapter) { 242 function _encodeAdapterData(DeploymentArgs memory adapterData, bytes memory baseAdapterData) 243 internal 244: returns (bytes memory) 256 function _deployStrategy(DeploymentArgs memory strategyData, IDeploymentController _deploymentController) 257 internal 258: returns (address strategy) 390: function _registerVault(address vault, VaultMetadata memory metadata) internal { 692: function _verifyAdapterConfiguration(address adapter, bytes32 adapterId) internal view {
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There are 2 instances of this issue:
File: src/utils/MultiRewardStaking.sol /// @audit if-condition on line 356 357: amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp); /// @audit if-condition on line 394 395: elapsed = rewards.rewardsEndTimestamp - rewards.lastUpdatedTimestamp;
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe 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
There are 21 instances of this issue:
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++) {
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++) {
keccak256() should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once
There are 9 instances of this issue:
File: src/utils/MultiRewardStaking.sol 466: keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"), 495: keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), 497: keccak256("1"),
File: src/vault/adapter/abstracts/AdapterBase.sol 653 keccak256( 654 "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 655: ), 688 keccak256( 689 "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" 690: ), 692: keccak256("1"),
File: src/vault/Vault.sol 685 keccak256( 686 "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 687: ), 720 keccak256( 721 "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" 722: ), 724: keccak256("1"),
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. 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
There are 26 instances of this issue:
File: src/interfaces/IMultiRewardEscrow.sol /// @audit lock(), setFees(), fees() 31: interface IMultiRewardEscrow {
File: src/interfaces/IMultiRewardStaking.sol /// @audit addRewardToken(), changeRewardSpeed(), fundReward(), initialize(), rewardInfos(), escrowInfos() 37: interface IMultiRewardStaking is IERC4626, IOwned, IPermit, IPausable {
File: src/interfaces/vault/IAdapter.sol /// @audit strategy(), strategyConfig(), strategyDeposit(), strategyWithdraw(), setPerformanceFee(), performanceFee(), highWaterMark(), harvest(), harvestCooldown(), setHarvestCooldown(), initialize() 11: interface IAdapter is IERC4626, IOwned, IPermit, IPausable {
File: src/interfaces/vault/ICloneRegistry.sol /// @audit cloneExists(), addClone() 8: interface ICloneRegistry is IOwned {
File: src/interfaces/vault/IDeploymentController.sol /// @audit templateCategoryExists(), templateExists(), addTemplate(), addTemplateCategory(), toggleTemplateEndorsement(), getTemplate(), nominateNewDependencyOwner(), acceptDependencyOwnership(), cloneFactory(), cloneRegistry(), templateRegistry(), PermissionRegistry(), addClone() 11: interface IDeploymentController is ICloneFactory, ICloneRegistry {
File: src/interfaces/vault/IPermissionRegistry.sol /// @audit setPermissions(), endorsed(), rejected() 13: interface IPermissionRegistry is IOwned {
File: src/interfaces/vault/IStrategy.sol /// @audit harvest(), verifyAdapterSelectorCompatibility(), verifyAdapterCompatibility(), setUp() 6: interface IStrategy {
File: src/interfaces/vault/ITemplateRegistry.sol /// @audit templates(), templateCategoryExists(), templateExists(), getTemplateCategories(), getTemplate(), getTemplateIds(), addTemplate(), addTemplateCategory(), toggleTemplateEndorsement() 23: interface ITemplateRegistry is IOwned {
File: src/interfaces/vault/IVaultController.sol /// @audit deployVault(), deployAdapter(), deployStaking(), proposeVaultAdapters(), changeVaultAdapters(), proposeVaultFees(), changeVaultFees(), registerVaults(), addClones(), toggleEndorsements(), toggleRejections(), addStakingRewardsTokens(), changeStakingRewardsSpeeds(), fundStakingRewards(), setEscrowTokenFees(), addTemplateCategories(), toggleTemplateEndorsements(), pauseAdapters(), pauseVaults(), unpauseAdapters(), unpauseVaults(), nominateNewAdminProxyOwner(), acceptAdminProxyOwnership(), setPerformanceFee(), setAdapterPerformanceFees(), performanceFee(), setHarvestCooldown(), setAdapterHarvestCooldowns(), harvestCooldown(), setDeploymentController(), setActiveTemplateId(), activeTemplateId() 19: interface IVaultController {
File: src/interfaces/vault/IVaultRegistry.sol /// @audit getVault(), getSubmitter(), registerVault() 24: interface IVaultRegistry is IOwned {
File: src/interfaces/vault/IVault.sol /// @audit accruedManagementFee(), accruedPerformanceFee(), highWaterMark(), assetsCheckpoint(), feesUpdatedAt(), feeRecipient(), deposit(), takeManagementAndPerformanceFees(), adapter(), proposedAdapter(), proposedAdapterTime(), proposeAdapter(), changeAdapter(), fees(), proposedFees(), proposedFeeTime(), proposeFees(), changeFees(), setFeeRecipient(), quitPeriod(), setQuitPeriod(), initialize() 29: interface IVault is IERC4626 {
File: src/interfaces/vault/IWithRewards.sol /// @audit claim(), rewardTokens() 6: interface IWithRewards {
File: src/utils/MultiRewardEscrow.sol /// @audit getEscrowIdsByUser(), getEscrowIdsByUserAndToken(), getEscrows(), lock(), isClaimable(), getClaimableAmount(), claimRewards(), setFees() 21: contract MultiRewardEscrow is Owned {
File: src/utils/MultiRewardStaking.sol /// @audit initialize(), deposit(), claimRewards(), addRewardToken(), changeRewardSpeed(), fundReward(), getAllRewardsTokens() 26: contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit convertToUnderlyingShares(), harvest(), strategyDeposit(), strategyWithdraw(), setHarvestCooldown(), accruedPerformanceFee(), setPerformanceFee() 27: abstract contract AdapterBase is
File: src/vault/adapter/abstracts/WithRewards.sol /// @audit rewardTokens(), claim() 12: contract WithRewards is EIP165, OnlyStrategy {
File: src/vault/adapter/beefy/IBeefy.sol /// @audit want(), deposit(), withdrawAll(), balance(), earn(), getPricePerFullShare(), strategy() 6: interface IBeefyVault { /// @audit earned(), stakedToken(), rewardToken(), periodFinish(), rewardPerToken(), stake(), exit(), getReward() 29: interface IBeefyBooster {
File: src/vault/adapter/yearn/IYearn.sol /// @audit deposit(), pricePerShare(), depositLimit(), token(), lastReport(), lockedProfit(), lockedProfitDegradation(), totalDebt() 8: interface VaultAPI is IERC20 {
File: src/vault/CloneRegistry.sol /// @audit addClone(), getClonesByCategoryAndId(), getAllClones() 16: contract CloneRegistry is Owned {
File: src/vault/DeploymentController.sol /// @audit addTemplateCategory(), addTemplate(), toggleTemplateEndorsement(), deploy(), nominateNewDependencyOwner(), acceptDependencyOwnership() 18: contract DeploymentController is Owned {
File: src/vault/PermissionRegistry.sol /// @audit setPermissions(), endorsed(), rejected() 14: contract PermissionRegistry is Owned {
File: src/vault/TemplateRegistry.sol /// @audit addTemplateCategory(), addTemplate(), toggleTemplateEndorsement(), getTemplateCategories(), getTemplateIds(), getTemplate() 18: contract TemplateRegistry is Owned {
File: src/vault/VaultController.sol /// @audit deployVault(), deployAdapter(), deployStaking(), proposeVaultAdapters(), changeVaultAdapters(), proposeVaultFees(), changeVaultFees(), setPermissions(), addStakingRewardsTokens(), changeStakingRewardsSpeeds(), fundStakingRewards(), setEscrowTokenFees(), addTemplateCategories(), toggleTemplateEndorsements(), pauseAdapters(), pauseVaults(), unpauseAdapters(), unpauseVaults(), nominateNewAdminProxyOwner(), acceptAdminProxyOwnership(), setPerformanceFee(), setAdapterPerformanceFees(), setHarvestCooldown(), setAdapterHarvestCooldowns(), setDeploymentController(), setActiveTemplateId() 29: contract VaultController is Owned {
File: src/vault/VaultRegistry.sol /// @audit registerVault(), getVault(), getVaultsByAsset(), getTotalVaults(), getRegisteredAddresses(), getSubmitter() 15: contract VaultRegistry is Owned {
File: src/vault/Vault.sol /// @audit initialize(), deposit(), accruedManagementFee(), accruedPerformanceFee(), takeManagementAndPerformanceFees(), proposeFees(), changeFees(), setFeeRecipient(), proposeAdapter(), changeAdapter(), setQuitPeriod() 26: contract Vault is
There is 1 instance of this issue:
File: src/vault/Vault.sol /// @audit highWaterMark_ 448: uint256 highWaterMark_ = highWaterMark;
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen 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 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. Use a larger size then downcast where needed
There are 2 instances of this issue:
File: src/utils/MultiRewardStaking.sol /// @audit uint32 rewardsEndTimestamp 342: rewardsEndTimestamp = _calcRewardsEnd(rewards.rewardsEndTimestamp, rewards.rewardsPerSecond, amount); /// @audit uint224 deltaIndex 406: deltaIndex = accrued.mulDiv(uint256(10**decimals()), supplyTokens, Math.Rounding.Down).safeCastTo224();
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
There are 4 instances of this issue:
File: src/vault/VaultController.sol 36: bytes32 public immutable VAULT = "Vault"; 37: bytes32 public immutable ADAPTER = "Adapter"; 38: bytes32 public immutable STRATEGY = "Strategy"; 39: bytes32 public immutable STAKING = "Staking";
block.timestamp and block.number are added to event information by default so adding them manually wastes gas
There are 2 instances of this issue:
File: src/vault/Vault.sol 512: event NewFeesProposed(VaultFees newFees, uint256 timestamp); 569: event NewAdapterProposed(IERC4626 newAdapter, uint256 timestamp);
payableIf 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. 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
There are 9 instances of this issue:
File: src/utils/MultiRewardStaking.sol 243 function addRewardToken( 244 IERC20 rewardToken, 245 uint160 rewardsPerSecond, 246 uint256 amount, 247 bool useEscrow, 248 uint192 escrowPercentage, 249 uint32 escrowDuration, 250 uint32 offset 251: ) external onlyOwner {
File: src/vault/adapter/abstracts/AdapterBase.sol 55 function __AdapterBase_init(bytes memory popERC4626InitData) 56 internal 57: onlyInitializing 456 function strategyDeposit(uint256 amount, uint256 shares) 457 public 458: onlyStrategy 467 function strategyWithdraw(uint256 amount, uint256 shares) 468 public 469: onlyStrategy
File: src/vault/AdminProxy.sol 19 function execute(address target, bytes calldata callData) 20 external 21 onlyOwner 22: returns (bool success, bytes memory returndata)
File: src/vault/CloneRegistry.sol 41 function addClone( 42 bytes32 templateCategory, 43 bytes32 templateId, 44 address clone 45: ) external onlyOwner {
File: src/vault/DeploymentController.sol 99 function deploy( 100 bytes32 templateCategory, 101 bytes32 templateId, 102 bytes calldata data 103: ) external onlyOwner returns (address clone) {
File: src/vault/TemplateRegistry.sol 67 function addTemplate( 68 bytes32 templateCategory, 69 bytes32 templateId, 70 Template memory template 71: ) external onlyOwner {
File: src/vault/VaultController.sol 579 function toggleTemplateEndorsements(bytes32[] calldata templateCategories, bytes32[] calldata templateIds) 580 external 581: onlyOwner
payablePayable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 9 instances of this issue:
File: src/utils/MultiRewardEscrow.sol 30: constructor(address _owner, address _feeRecipient) Owned(_owner) {
File: src/vault/AdminProxy.sol 16: constructor(address _owner) Owned(_owner) {}
File: src/vault/CloneFactory.sol 23: constructor(address _owner) Owned(_owner) {}
File: src/vault/CloneRegistry.sol 22: constructor(address _owner) Owned(_owner) {}
File: src/vault/DeploymentController.sol 35 constructor( 36 address _owner, 37 ICloneFactory _cloneFactory, 38 ICloneRegistry _cloneRegistry, 39 ITemplateRegistry _templateRegistry 40: ) Owned(_owner) {
File: src/vault/PermissionRegistry.sol 20: constructor(address _owner) Owned(_owner) {}
File: src/vault/TemplateRegistry.sol 24: constructor(address _owner) Owned(_owner) {}
File: src/vault/VaultController.sol 53 constructor( 54 address _owner, 55 IAdminProxy _adminProxy, 56 IDeploymentController _deploymentController, 57 IVaultRegistry _vaultRegistry, 58 IPermissionRegistry _permissionRegistry, 59 IMultiRewardEscrow _escrow 60: ) Owned(_owner) {
File: src/vault/VaultRegistry.sol 21: constructor(address _owner) Owned(_owner) {}
_msgSender() if not supporting EIP-2771Use msg.sender if the code does not implement EIP-2771 trusted forwarder support
There are 4 instances of this issue:
File: src/vault/adapter/abstracts/AdapterBase.sol 119: _deposit(_msgSender(), receiver, assets, shares); 138: _deposit(_msgSender(), receiver, assets, shares); 182: _withdraw(_msgSender(), receiver, owner, assets, shares); 201: _withdraw(_msgSender(), receiver, owner, assets, shares);
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Using calldata instead of memory for read-only arguments in external functions saves gas | 16 | 1920 |
| [G‑02] | State variables should be cached in stack variables rather than re-reading them from storage | 6 | - |
| [G‑03] | <array>.length should not be looked up in every loop of a for-loop | 5 | 15 |
| [G‑04] | Using bools for storage incurs overhead | 3 | 51300 |
| [G‑05] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 22 | 110 |
| [G‑06] | Using private rather than public for constants, saves gas | 1 | - |
| [G‑07] | Functions guaranteed to revert when called by normal users can be marked payable | 32 | 672 |
Total: 85 instances over 7 issues with 54017 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 16 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit escrowIds - (valid but excluded finding) 154: function claimRewards(bytes32[] memory escrowIds) external { /// @audit tokens - (valid but excluded finding) /// @audit tokenFees - (valid but excluded finding) 207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {
File: src/utils/MultiRewardStaking.sol /// @audit _rewardTokens - (valid but excluded finding) 170: function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
File: src/vault/adapter/beefy/BeefyAdapter.sol /// @audit adapterInitData - (valid but excluded finding) /// @audit beefyInitData - (valid but excluded finding) 46 function initialize( 47 bytes memory adapterInitData, 48 address registry, 49 bytes memory beefyInitData 50: ) external initializer {
File: src/vault/adapter/yearn/YearnAdapter.sol /// @audit adapterInitData - (valid but excluded finding) /// @audit (valid but excluded finding) 34 function initialize( 35 bytes memory adapterInitData, 36 address externalRegistry, 37 bytes memory 38: ) external initializer {
File: src/vault/TemplateRegistry.sol /// @audit template - (valid but excluded finding) 67 function addTemplate( 68 bytes32 templateCategory, 69 bytes32 templateId, 70 Template memory template 71: ) external onlyOwner {
File: src/vault/VaultController.sol /// @audit vaultData - (valid but excluded finding) /// @audit adapterData - (valid but excluded finding) /// @audit strategyData - (valid but excluded finding) /// @audit rewardsData - (valid but excluded finding) /// @audit metadata - (valid but excluded finding) 89 function deployVault( 90 VaultInitParams memory vaultData, 91 DeploymentArgs memory adapterData, 92 DeploymentArgs memory strategyData, 93 address staking, 94 bytes memory rewardsData, 95 VaultMetadata memory metadata, 96 uint256 initialDeposit 97: ) external canCreate returns (address vault) { /// @audit adapterData - (valid but excluded finding) /// @audit strategyData - (valid but excluded finding) 186 function deployAdapter( 187 IERC20 asset, 188 DeploymentArgs memory adapterData, 189 DeploymentArgs memory strategyData, 190 uint256 initialDeposit 191: ) external canCreate returns (address adapter) {
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.
There are 6 instances of this issue:
File: src/vault/VaultController.sol /// @audit adminProxy on line 288 - (valid but excluded finding) 294: abi.encodeWithSelector(IMultiRewardStaking.initialize.selector, asset, escrow, adminProxy) /// @audit adminProxy on line 450 - (valid but excluded finding) 457: IERC20(rewardsToken).transferFrom(msg.sender, address(adminProxy), amount);
File: src/vault/Vault.sol /// @audit proposedFees on line 544 - (valid but excluded finding) 545: fees = proposedFees; /// @audit adapter on line 599 - (valid but excluded finding) 604: asset.approve(address(adapter), 0); /// @audit adapter on line 604 - (valid but excluded finding) 606: emit ChangedAdapter(adapter, proposedAdapter); /// @audit proposedAdapter on line 606 - (valid but excluded finding) 608: adapter = proposedAdapter;
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)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
There are 5 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit (valid but excluded finding) 53: for (uint256 i = 0; i < escrowIds.length; i++) { /// @audit (valid but excluded finding) 155: for (uint256 i = 0; i < escrowIds.length; i++) { /// @audit (valid but excluded finding) 210: for (uint256 i = 0; i < tokens.length; i++) {
File: src/utils/MultiRewardStaking.sol /// @audit (valid but excluded finding) 171: for (uint8 i; i < _rewardTokens.length; i++) { /// @audit (valid but excluded finding) 373: for (uint8 i; i < _rewardTokens.length; i++) {
bools for storage incurs overhead// 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.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
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
There are 3 instances of this issue:
File: src/vault/CloneRegistry.sol /// @audit (valid but excluded finding) 28: mapping(address => bool) public cloneExists;
File: src/vault/TemplateRegistry.sol /// @audit (valid but excluded finding) 33: mapping(bytes32 => bool) public templateExists; /// @audit (valid but excluded finding) 35: mapping(bytes32 => bool) public templateCategoryExists;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 22 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit (valid but excluded finding) 53: for (uint256 i = 0; i < escrowIds.length; i++) { /// @audit (valid but excluded finding) 102: nonce++; /// @audit (valid but excluded finding) 155: for (uint256 i = 0; i < escrowIds.length; i++) { /// @audit (valid but excluded finding) 210: for (uint256 i = 0; i < tokens.length; i++) {
File: src/utils/MultiRewardStaking.sol /// @audit (valid but excluded finding) 171: for (uint8 i; i < _rewardTokens.length; i++) { /// @audit (valid but excluded finding) 373: for (uint8 i; i < _rewardTokens.length; i++) {
File: src/vault/PermissionRegistry.sol /// @audit (valid but excluded finding) 42: for (uint256 i = 0; i < len; i++) {
File: src/vault/VaultController.sol /// @audit (valid but excluded finding) 319: for (uint8 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 337: for (uint8 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 357: for (uint8 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 374: for (uint8 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 437: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 494: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 523: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 564: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 587: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 607: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 620: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 633: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 646: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 766: for (uint256 i = 0; i < len; i++) { /// @audit (valid but excluded finding) 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
There is 1 instance of this issue:
File: src/vault/adapter/beefy/BeefyAdapter.sol /// @audit (valid but excluded finding) 31: uint256 public constant BPS_DENOMINATOR = 10_000;
payableIf 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. 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
There are 32 instances of this issue:
File: src/utils/MultiRewardEscrow.sol /// @audit (valid but excluded finding) 207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {
File: src/utils/MultiRewardStaking.sol /// @audit (valid but excluded finding) 296: function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
File: src/vault/adapter/abstracts/AdapterBase.sol /// @audit (valid but excluded finding) 500: function setHarvestCooldown(uint256 newCooldown) external onlyOwner { /// @audit (valid but excluded finding) 549: function setPerformanceFee(uint256 newFee) public onlyOwner { /// @audit (valid but excluded finding) 574: function pause() external onlyOwner { /// @audit (valid but excluded finding) 582: function unpause() external onlyOwner {
File: src/vault/adapter/abstracts/WithRewards.sol /// @audit (valid but excluded finding) 15: function claim() public virtual onlyStrategy {}
File: src/vault/adapter/beefy/BeefyAdapter.sol /// @audit (valid but excluded finding) 221: function claim() public override onlyStrategy {
File: src/vault/CloneFactory.sol /// @audit (valid but excluded finding) 39: function deploy(Template calldata template, bytes calldata data) external onlyOwner returns (address clone) {
File: src/vault/DeploymentController.sol /// @audit (valid but excluded finding) 55: function addTemplateCategory(bytes32 templateCategory) external onlyOwner { /// @audit (valid but excluded finding) 80: function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner { /// @audit (valid but excluded finding) 121: function nominateNewDependencyOwner(address _owner) external onlyOwner {
File: src/vault/PermissionRegistry.sol /// @audit (valid but excluded finding) 38: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner {
File: src/vault/TemplateRegistry.sol /// @audit (valid but excluded finding) 52: function addTemplateCategory(bytes32 templateCategory) external onlyOwner { /// @audit (valid but excluded finding) 102: function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
File: src/vault/VaultController.sol /// @audit (valid but excluded finding) 408: function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner { /// @audit (valid but excluded finding) 543: function setEscrowTokenFees(IERC20[] calldata tokens, uint256[] calldata fees) external onlyOwner { /// @audit (valid but excluded finding) 561: function addTemplateCategories(bytes32[] calldata templateCategories) external onlyOwner { /// @audit (valid but excluded finding) 723: function nominateNewAdminProxyOwner(address newOwner) external onlyOwner { /// @audit (valid but excluded finding) 751: function setPerformanceFee(uint256 newFee) external onlyOwner { /// @audit (valid but excluded finding) 764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner { /// @audit (valid but excluded finding) 791: function setHarvestCooldown(uint256 newCooldown) external onlyOwner { /// @audit (valid but excluded finding) 804: function setAdapterHarvestCooldowns(address[] calldata adapters) external onlyOwner { /// @audit (valid but excluded finding) 832: function setDeploymentController(IDeploymentController _deploymentController) external onlyOwner { /// @audit (valid but excluded finding) 864: function setActiveTemplateId(bytes32 templateCategory, bytes32 templateId) external onlyOwner {
File: src/vault/VaultRegistry.sol /// @audit (valid but excluded finding) 44: function registerVault(VaultMetadata calldata _metadata) external onlyOwner {
File: src/vault/Vault.sol /// @audit (valid but excluded finding) 525: function proposeFees(VaultFees calldata newFees) external onlyOwner { /// @audit (valid but excluded finding) 553: function setFeeRecipient(address _feeRecipient) external onlyOwner { /// @audit (valid but excluded finding) 578: function proposeAdapter(IERC4626 newAdapter) external onlyOwner { /// @audit (valid but excluded finding) 629: function setQuitPeriod(uint256 _quitPeriod) external onlyOwner { /// @audit (valid but excluded finding) 643: function pause() external onlyOwner { /// @audit (valid but excluded finding) 648: function unpause() external onlyOwner {
#0 - c4-judge
2023-02-28T14:52:53Z
dmvt marked the issue as grade-b