Popcorn contest - 0xackermann's results

A multi-chain regenerative yield-optimizing protocol.

General Information

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

Popcorn

Findings Distribution

Researcher Performance

Rank: 85/169

Findings: 1

Award: $69.82

Gas:
grade-b

๐ŸŒŸ Selected for report: 0

๐Ÿš€ Solo Findings: 0

Awards

69.8247 USDC - $69.82

Labels

bug
G (Gas Optimization)
grade-b
G-10

External Links

Gas Report

Exlusive findings is below the automated findings.


Automated Findings

Gas Optimizations

IssueInstances
GAS-1Use assembly to check for address(0)24
GAS-2Using bools for storage incurs overhead3
GAS-3Cache array length outside of loop5
GAS-4State variables should be cached in stack variables rather than re-reading them from storage3
GAS-5Use calldata instead of memory for function arguments that do not get mutated16
GAS-6Don't initialize variables with default value19
GAS-7Functions guaranteed to revert when called by normal users can be marked payable28
GAS-8++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)24
GAS-9Using private rather than public for constants, saves gas1
GAS-10Use != 0 instead of > 0 for unsigned integer comparison23

<a name="GAS-1"></a>[GAS-1] Use assembly to check for address(0)

Saves 6 gas per instance

Instances (24):

File: src/utils/MultiRewardEscrow.sol

96:     if (account == address(0)) revert ZeroAddress();

Link to code

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);

Link to code

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)

Link to code

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))

Link to code

File: src/vault/VaultRegistry.sol

45:     if (metadata[_metadata.vault].vault != address(0)) revert VaultAlreadyRegistered();

Link to code

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();

Link to code

<a name="GAS-2"></a>[GAS-2] Using bools for storage incurs overhead

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;

Link to code

File: src/vault/TemplateRegistry.sol

33:   mapping(bytes32 => bool) public templateExists;

35:   mapping(bytes32 => bool) public templateCategoryExists;

Link to code

<a name="GAS-3"></a>[GAS-3] Cache array length outside of loop

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++) {

Link to code

File: src/utils/MultiRewardStaking.sol

171:     for (uint8 i; i < _rewardTokens.length; i++) {

373:     for (uint8 i; i < _rewardTokens.length; i++) {

Link to code

<a name="GAS-4"></a>[GAS-4] State variables should be cached in stack variables rather than re-reading them from storage

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;

Link to code

<a name="GAS-5"></a>[GAS-5] Use calldata instead of memory for function arguments that do not get mutated

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 {

Link to code

File: src/utils/MultiRewardStaking.sol

170:   function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {

Link to code

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 {

Link to code

File: src/vault/adapter/beefy/BeefyAdapter.sol

47:         bytes memory adapterInitData,

49:         bytes memory beefyInitData

Link to code

File: src/vault/adapter/yearn/YearnAdapter.sol

35:         bytes memory adapterInitData,

37:         bytes memory

Link to code

<a name="GAS-6"></a>[GAS-6] Don't initialize variables with default value

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++) {

Link to code

File: src/vault/PermissionRegistry.sol

42:     for (uint256 i = 0; i < len; i++) {

Link to code

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++) {

Link to code

<a name="GAS-7"></a>[GAS-7] Functions guaranteed to revert when called by normal users can be marked 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 {

Link to code

File: src/utils/MultiRewardStaking.sol

296:   function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {

Link to code

File: src/vault/CloneFactory.sol

39:   function deploy(Template calldata template, bytes calldata data) external onlyOwner returns (address clone) {

Link to code

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 {

Link to code

File: src/vault/PermissionRegistry.sol

38:   function setPermissions(address[] calldata targets, Permission[] calldata newPermissions) external onlyOwner {

Link to code

File: src/vault/TemplateRegistry.sol

52:   function addTemplateCategory(bytes32 templateCategory) external onlyOwner {

102:   function toggleTemplateEndorsement(bytes32 templateCategory, bytes32 templateId) external onlyOwner {

Link to code

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 {

Link to code

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 {

Link to code

File: src/vault/VaultRegistry.sol

44:   function registerVault(VaultMetadata calldata _metadata) external onlyOwner {

Link to code

File: src/vault/adapter/abstracts/WithRewards.sol

15:   function claim() public virtual onlyStrategy {}

Link to code

File: src/vault/adapter/beefy/BeefyAdapter.sol

221:     function claim() public override onlyStrategy {

Link to code

<a name="GAS-8"></a>[GAS-8] ++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++) {

Link to code

File: src/utils/MultiRewardStaking.sol

171:     for (uint8 i; i < _rewardTokens.length; i++) {

373:     for (uint8 i; i < _rewardTokens.length; i++) {

470:                 nonces[owner]++,

Link to code

File: src/vault/PermissionRegistry.sol

42:     for (uint256 i = 0; i < len; i++) {

Link to code

File: src/vault/Vault.sol

691:                                 nonces[owner]++,

Link to code

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++) {

Link to code

<a name="GAS-9"></a>[GAS-9] Using private rather than public for constants, saves gas

If 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;

Link to code

<a name="GAS-10"></a>[GAS-10] Use != 0 instead of > 0 for unsigned integer comparison

Instances (23):

File: src/utils/MultiRewardEscrow.sol

107:     if (feePerc > 0) {

141:     return escrows[escrowId].lastUpdateTime != 0 && escrows[escrowId].balance > 0;

Link to code

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));

Link to code

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)

Link to code

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();

Link to code

File: src/vault/adapter/beefy/BeefyAdapter.sol

156:         if (beefyFee > 0)

177:         if (beefyFee > 0)

Link to code


Exlusive Findings


Gas Optimizations Report


[G-01] Using 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:


[G-02] Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead


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:


[G-03] ++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-loop and while-loops


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:

https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md#g011---unnecessary-checked-arithmetic-in-for-loop

Lines of Code:


[G-04] Optimize names to save gas [22 gas per instance]


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:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Lines of Code:


[G-05] Setting the constructor to payable [~13 gas per instance]


Lines of Code:


[G-06] Superfluous event fields


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:


[G-07] Comparison operators


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:


[G-08] Use a more recent version of solidity


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:


[G-09] <array>.length should not be looked up in every loop of a for-loop


Description:

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:


[G-10] x += y costs more gas than x = x + y for state variables


Lines of Code:

#0 - c4-judge

2023-02-28T14:52:02Z

dmvt marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax ยฉ 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter