Popcorn contest - Rolezn's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L107-L118 https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L147

Vulnerability details

(1) Contracts do not work with fee-on-transfer tokens

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

Should a fee-on-transfer token be added as an asset and deposited, it could be abused to mint more shares. In the current implementation the function calls on contracts of MultiRewardStaking.deposit() which also calls for _mint, do not work well with fee-on-transfer tokens as the amount variable is the pre-fee amount, including the fee, whereas the totalAssets do not include the fee anymore.

<ins>Proof Of Concept</ins>

function MultiRewardStaking._deposit() overrides ERC4246Upgradeable's _deposit function to the following: https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L107-L118

When calling deposit, a deposit(1000) should result in the same shares as two deposits of deposit(500) but it does not because amount is the pre-fee amount. Assume a fee-on-transfer of 20%. Assume current totalAssets = 1000, totalSupply = 1000 for simplicity.

• deposit(1000) = 1000 / totalAssets * totalSupply = 1000 shares
• deposit(500) = 500 / totalAssets * totalSupply = 500 shares. Now the totalSupply increased by 500 but the totalAssets only increased by (100% - 20%) * 500 = 400. Therefore, the second deposit(500) = 500 / (totalAssets + 400) * (newTotalSupply) = 500 / (1400) * 1500 = 535.714285714 shares.

In total, the two deposits lead to 35 more shares than a single deposit of the sum of the deposits. That breaks whole math for given token.

107: function _deposit(
    address caller,
    address receiver,
    uint256 assets,
    uint256 shares
  ) internal override accrueRewards(caller, receiver) {
    IERC20(asset()).safeTransferFrom(caller, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(caller, receiver, assets, shares);
  }

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L107-L118

147: function _deposit(
        address caller,
        address receiver,
        uint256 assets,
        uint256 shares
    ) internal nonReentrant virtual override {
        IERC20(asset()).safeTransferFrom(caller, address(this), assets);
        
        uint256 underlyingBalance_ = _underlyingBalance();
        _protocolDeposit(assets, shares);
        
        underlyingBalance += _underlyingBalance() - underlyingBalance_;

        _mint(receiver, shares);

        harvest();

        emit Deposit(caller, receiver, assets, shares);
    }

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L147

This will effect YearnAdapter contract as it inherits from AdapterBase contract.

<ins>Recommended Mitigation Steps</ins>

• Consider comparing before and after balance to get the actual transferred amount.
• Alternatively, disallow tokens with fee-on-transfer mechanics to be added as reward tokens.

Summary<a name="Summary">

Low Risk Issues

Total: 36 contexts over 11 issues

Non-critical Issues

Total: 283 contexts over 20 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> Use of ecrecover is susceptible to signature malleability

The built-in EVM precompile ecrecover is susceptible to signature malleability, which could lead to replay attacks. References: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121, and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57. While this is not immediately exploitable, this may become a vulnerability if used elsewhere.

<ins>Proof Of Concept</ins>

459: function permit(
    address owner,
    address spender,
    uint256 value,
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
  ) public virtual {
    if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

    
    
    unchecked {
      address recoveredAddress = ecrecover(
        keccak256(
          abi.encodePacked(
            "/x19/x01",
            DOMAIN_SEPARATOR(),
            keccak256(
              abi.encode(
                keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
                owner,
                spender,
                value,
                nonces[owner]++,
                deadline
              )
            )
          )
        ),
        v,
        r,
        s
      );

      if (recoveredAddress == address(0) || recoveredAddress != owner) revert InvalidSigner(recoveredAddress);

      _approve(recoveredAddress, spender, value);
    }
  }

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L459

678: function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

        
        
        unchecked {
            address recoveredAddress = ecrecover(
                keccak256(
                    abi.encodePacked(
                        "/x19/x01",
                        DOMAIN_SEPARATOR(),
                        keccak256(
                            abi.encode(
                                keccak256(
                                    "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
                                ),
                                owner,
                                spender,
                                value,
                                nonces[owner]++,
                                deadline
                            )
                        )
                    )
                ),
                v,
                r,
                s
            );

            if (recoveredAddress == address(0) || recoveredAddress != owner)
                revert InvalidSigner(recoveredAddress);

            _approve(recoveredAddress, spender, value);
        }
    }

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L678

646: function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

        
        
        unchecked {
            address recoveredAddress = ecrecover(
                keccak256(
                    abi.encodePacked(
                        "/x19/x01",
                        DOMAIN_SEPARATOR(),
                        keccak256(
                            abi.encode(
                                keccak256(
                                    "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
                                ),
                                owner,
                                spender,
                                value,
                                nonces[owner]++,
                                deadline
                            )
                        )
                    )
                ),
                v,
                r,
                s
            );

            if (recoveredAddress == address(0) || recoveredAddress != owner)
                revert InvalidSigner(recoveredAddress);

            _approve(recoveredAddress, spender, value);
        }
    }

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L646

Recommended Mitigation Steps

Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L138-L149

<a href="#Summary">[LOW‑2]</a><a name="LOW&#x2011;2"> decimals() not part of ERC20 standard

decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

<ins>Proof Of Concept</ins>

51: _decimals = IERC20Metadata(address(_stakingToken)).decimals()

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L51

274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L274

82: _decimals = IERC20Metadata(address(asset_)).decimals()

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L82

77: _decimals = IERC20Metadata(asset).decimals()

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L77

<a href="#Summary">[LOW‑3]</a><a name="LOW&#x2011;3"> The Contract Should approve(0) First

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

<ins>Proof Of Concept</ins>

80: asset.approve(address(adapter_), type(uint256).max);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L80

604: asset.approve(address(adapter), 0);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L604

610: asset.approve(address(adapter), type(uint256).max);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L610

171: asset.approve(address(target), initialDeposit);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L171

456: IERC20(rewardsToken).approve(staking, type(uint256).max);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L456

80: IERC20(asset()).approve(_beefyVault, type(uint256).max);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L80

83: IERC20(_beefyVault).approve(_beefyBooster, type(uint256).max);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L83

54: IERC20(_asset).approve(address(yVault), type(uint256).max);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L54

<ins>Recommended Mitigation Steps</ins>

Approve with a zero amount first before setting the actual amount.

<a href="#Summary">[LOW‑4]</a><a name="LOW&#x2011;4"> Missing parameter validation in constructor

Some parameters of constructors are not checked for invalid values.

<ins>Proof Of Concept</ins>

36: address _owner

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L36

54: address _owner

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L54

<ins>Recommended Mitigation Steps</ins>

Validate the parameters.

<a href="#Summary">[LOW‑5]</a><a name="LOW&#x2011;5"> Missing Contract-existence Checks Before Low-level Calls

Low-level calls return success if there is no code present at the specified address.

<ins>Proof Of Concept</ins>

24: return target.call(callData);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/AdminProxy.sol#L24

43: if (template.requiresInitData) (success, ) = clone.call(data);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L43

<ins>Recommended Mitigation Steps</ins>

In addition to the zero-address checks, add a check to verify that <address>.code.length > 0

<a href="#Summary">[LOW‑6]</a><a name="LOW&#x2011;6"> Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

<ins>Proof of Concept</ins>

8: import { Math } from "openzeppelin-contracts/utils/math/Math.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L8

6: import { Clones } from "openzeppelin-contracts/proxy/Clones.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L6

10: import {IERC20Metadata} from "openzeppelin-contracts/token/ERC20/extensions/IERC20Metadata.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L10

<ins>Recommended Mitigation Steps</ins>

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts. See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for list of available upgradeable contracts

<a href="#Summary">[LOW‑7]</a><a name="LOW&#x2011;7"> Unbounded loop

New items are pushed into the following arrays:

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L263

MultiRewardStaking.accrueRewards() will iterate all the rewardTokens.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L373

Currently, the array can grow indefinitely. E.g. there's no maximum limit and there's no functionality to remove array values.

If the array grows too large, calling MultiRewardStaking.accrueRewards() might run out of gas and revert. Calling these functions will result in a DOS condition.

<ins>Proof Of Concept</ins>

263: rewardTokens.push(rewardToken);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L263

In addition the following can push values in the array but there is no function to remove them:

126: userEscrowIds[account].push(id);
127: userEscrowIdsByToken[account][token].push(id);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L126-L127

47: clones[templateCategory][templateId].push(clone);
48: allClones.push(clone);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneRegistry.sol#L47-L48

56: templateCategories.push(templateCategory);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L56

78: templateIds[templateCategory].push(templateId);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L78

49: allVaults.push(_metadata.vault);
50: vaultsByAsset[IERC4626(_metadata.vault).asset()].push(_metadata.vault);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L49-L50

<ins>Recommended Mitigation Steps</ins>

Add a functionality to delete array values or add a maximum size limit for arrays.

<a href="#Summary">[LOW‑8]</a><a name="LOW&#x2011;8"> No storage gap for upgradeable contract might lead to storage slot collision

For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts.

Refer to the bottom part of this article: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable

<ins>Proof Of Concept</ins>

However, the contract doesn't contain a storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps 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.

ERC4626Upgradeable

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L26

ERC20Upgradeable

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#0

<ins>Recommended Mitigation Steps</ins>

Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.

    uint256[50] private __gap;

<a href="#Summary">[LOW‑9]</a><a name="LOW&#x2011;9"> Upgrade OpenZeppelin Contract Dependency

An outdated OZ version is used (which has known vulnerabilities, see: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).

<ins>Proof Of Concept</ins>

Require dependencies to be at least version of 4.8.1 to include critical vulnerability patches.

    "@openzeppelin/contracts": "4.8.0"

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/../package.json#L25

<ins>Recommended Mitigation Steps</ins>

Update OpenZeppelin Contracts Usage in package.json and require at least version of 4.8.1 via >=4.8.1

<a href="#Summary">[LOW‑10]</a><a name="LOW&#x2011;10"> Missing delegatecall success check

The following function is missing a check on whether the return results of the delegatecall returns a success or failure

<ins>Proof Of Concept</ins>

444: function harvest() public takeFees {
        if (
            address(strategy) != address(0) &&
            ((lastHarvest + harvestCooldown) < block.timestamp)
        ) {
            // solhint-disable
            address(strategy).delegatecall(
                abi.encodeWithSignature("harvest()")
            );
        }

        emit Harvested();
    }

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450

<ins>Recommended Mitigation Steps</ins>

Add a success check using: (bool success, ) = and adding an if condition for success or failure.

<a href="#Summary">[LOW‑11]</a><a name="LOW&#x2011;11"> SECONDS_PER_YEAR value is incorrect

It is stated in https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L423-L428 that Management fee is annualized per minute, based on 525,600 minutes per year. However, SECONDS_PER_YEAR is set to 365.25 days which is 525960 minutes.

<ins>Proof Of Concept</ins>

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L35

<ins>Recommended Mitigation Steps</ins>

Set SECONDS_PER_YEAR to 525960 minutes or 365 days.

Non Critical Issues

<a href="#Summary">[NC‑1]</a><a name="NC&#x2011;1"> Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to the following functions:

<ins>Proof Of Concept</ins>

207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L207

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L296

553: function setFeeRecipient(address _feeRecipient) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L553

372: function changeVaultFees(address[] calldata vaults) external {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L372

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L408

483: function changeStakingRewardsSpeeds(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L483

543: function setEscrowTokenFees(IERC20[] calldata tokens, uint256[] calldata fees) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L543

751: function setPerformanceFee(uint256 newFee) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L751

764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L764

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> Avoid Floating Pragmas: The Version Should Be Locked

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

<ins>Proof Of Concept</ins>

Found usage of floating pragmas ^0.8.15 of Solidity in [IEIP165.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IEIP165.sol#L2

Found usage of floating pragmas ^0.8.15 of Solidity in [IMultiRewardEscrow.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardEscrow.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [IMultiRewardStaking.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [IAdapter.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IAdapter.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IAdminProxy.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IAdminProxy.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [ICloneFactory.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ICloneFactory.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [ICloneRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ICloneRegistry.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IDeploymentController.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IDeploymentController.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IERC4626.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IERC4626.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [IPermissionRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IPermissionRegistry.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IStrategy.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IStrategy.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [ITemplateRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ITemplateRegistry.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [IVault.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [IVaultController.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVaultController.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IVaultRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVaultRegistry.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [IWithRewards.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IWithRewards.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [EIP165.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/EIP165.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [MultiRewardEscrow.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [MultiRewardStaking.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [AdminProxy.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/AdminProxy.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [CloneFactory.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [CloneRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneRegistry.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [DeploymentController.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [PermissionRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [TemplateRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [Vault.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [VaultController.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L3

Found usage of floating pragmas ^0.8.15 of Solidity in [VaultRegistry.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [AdapterBase.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [OnlyStrategy.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/OnlyStrategy.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [WithRewards.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [BeefyAdapter.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IBeefy.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/IBeefy.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [IYearn.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/IYearn.sol#L4

Found usage of floating pragmas ^0.8.15 of Solidity in [YearnAdapter.sol]

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L4

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> Variable Names That Consist Of All Capital Letters Should Be Reserved For Const/immutable Variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead.

<ins>Proof Of Concept</ins>

274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L274

<a href="#Summary">[NC‑4]</a><a name="NC&#x2011;4"> Critical Changes Should Use Two-step Procedure

The critical procedures should be two step process.

See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure

<ins>Proof Of Concept</ins>

207: function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L207

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L296

553: function setFeeRecipient(address _feeRecipient) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L553

372: function changeVaultFees(address[] calldata vaults) external {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L372

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L408

483: function changeStakingRewardsSpeeds(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L483

543: function setEscrowTokenFees(IERC20[] calldata tokens, uint256[] calldata fees) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L543

751: function setPerformanceFee(uint256 newFee) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L751

764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L764

<ins>Recommended Mitigation Steps</ins>

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> block.timestamp is already used when emitting events, no need to input timestamp

<ins>Proof Of Concept</ins>

536: emit NewFeesProposed(newFees, block.timestamp);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L536

585: emit NewAdapterProposed(newAdapter, block.timestamp);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L585

<a href="#Summary">[NC‑6]</a><a name="NC&#x2011;6"> Event emit should emit a parameter

Some emitted events do not have any emitted parameters. It is recommended to add some parameter such as state changes or value changes when events are emitted

<ins>Proof Of Concept</ins>

449: emit Harvested()

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L449

<a href="#Summary">[NC‑7]</a><a name="NC&#x2011;7"> Function writing that does not comply with the Solidity Style Guide

Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private
• within a grouping, place the view and pure functions last

<ins>Proof Of Concept</ins>

All in-scope contracts

<a href="#Summary">[NC‑8]</a><a name="NC&#x2011;8"> Use delete to Clear Variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic. Consider replacing assignments of zero with delete statements.

<ins>Proof Of Concept</ins>

186: accruedRewards[user][_rewardTokens[i]] = 0;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L186

577: underlyingBalance = 0;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L577

<a href="#Summary">[NC‑9]</a><a name="NC&#x2011;9"> Imports can be grouped together

Consider importing OZ first, then all interfaces, then all utils.

<ins>Proof Of Concept</ins>

6: import { Owned } from "../utils/Owned.sol";
7: import { IOwned } from "../interfaces/IOwned.sol";
8: import { ICloneFactory } from "../interfaces/vault/ICloneFactory.sol";
9: import { ICloneRegistry } from "../interfaces/vault/ICloneRegistry.sol";
10: import { ITemplateRegistry, Template } from "../interfaces/vault/ITemplateRegistry.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L6-L10

6: import { Owned } from "../utils/Owned.sol";
7: import { Permission } from "../interfaces/vault/IPermissionRegistry.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L6-L7

6: import { Owned } from "../utils/Owned.sol";
7: import { Template } from "../interfaces/vault/ITemplateRegistry.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L6-L7

6: import {SafeERC20Upgradeable as SafeERC20} from "openzeppelin-contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
7: import {ReentrancyGuardUpgradeable} from "openzeppelin-contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
8: import {PausableUpgradeable} from "openzeppelin-contracts-upgradeable/security/PausableUpgradeable.sol";
9: import {IERC4626, IERC20} from "../interfaces/vault/IERC4626.sol";
10: import {IERC20Metadata} from "openzeppelin-contracts/token/ERC20/extensions/IERC20Metadata.sol";
11: import {VaultFees} from "../interfaces/vault/IVault.sol";
12: import {MathUpgradeable as Math} from "openzeppelin-contracts-upgradeable/utils/math/MathUpgradeable.sol";
13: import {OwnedUpgradeable} from "../utils/OwnedUpgradeable.sol";
14: import {ERC20Upgradeable} from "openzeppelin-contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L6-L14

5: import { SafeERC20Upgradeable as SafeERC20 } from "openzeppelin-contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
6: import { Owned } from "../utils/Owned.sol";
7: import { IVault, VaultInitParams, VaultFees } from "../interfaces/vault/IVault.sol";
8: import { IMultiRewardStaking } from "../interfaces/IMultiRewardStaking.sol";
9: import { IMultiRewardEscrow } from "../interfaces/IMultiRewardEscrow.sol";
10: import { IDeploymentController, ICloneRegistry } from "../interfaces/vault/IDeploymentController.sol";
11: import { ITemplateRegistry, Template } from "../interfaces/vault/ITemplateRegistry.sol";
12: import { IPermissionRegistry, Permission } from "../interfaces/vault/IPermissionRegistry.sol";
13: import { IVaultRegistry, VaultMetadata } from "../interfaces/vault/IVaultRegistry.sol";
14: import { IAdminProxy } from "../interfaces/vault/IAdminProxy.sol";
15: import { IERC4626, IERC20 } from "../interfaces/vault/IERC4626.sol";
16: import { IStrategy } from "../interfaces/vault/IStrategy.sol";
17: import { IAdapter } from "../interfaces/vault/IAdapter.sol";
18: import { IPausable } from "../interfaces/IPausable.sol";
19: import { DeploymentArgs } from "../interfaces/vault/IVaultController.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L5-L19

6: import { IERC4626 } from "../interfaces/vault/IERC4626.sol";
7: import { Owned } from "../utils/Owned.sol";
8: import { VaultMetadata } from "../interfaces/vault/IVaultRegistry.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L6-L8

6: import { EIP165 } from "../../../utils/EIP165.sol";
7: import { OnlyStrategy } from "./OnlyStrategy.sol";
8: import { IWithRewards } from "../../../interfaces/vault/IWithRewards.sol";
9: import { IAdapter } from "../../../interfaces/vault/IAdapter.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L6-L9

6: import {AdapterBase, IERC20, IERC20Metadata, SafeERC20, ERC20, Math, IStrategy, IAdapter} from "../abstracts/AdapterBase.sol";
7: import {WithRewards, IWithRewards} from "../abstracts/WithRewards.sol";
8: import {IBeefyVault, IBeefyBooster, IBeefyBalanceCheck, IBeefyStrat} from "./IBeefy.sol";
9: import {IPermissionRegistry} from "../../../interfaces/vault/IPermissionRegistry.sol";

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L6-L9

<a href="#Summary">[NC‑10]</a><a name="NC&#x2011;10"> NatSpec return parameters should be included in contracts

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

<ins>Proof Of Concept</ins>

All in-scope contracts

i.e. missing NATSPEC

  function _getClaimableAmount(Escrow memory escrow) internal view returns (uint256) {
    if (
      escrow.lastUpdateTime == 0 ||
      escrow.end == 0 ||
      escrow.balance == 0 ||
      block.timestamp.safeCastTo32() < escrow.start
    ) {
      return 0;
    }
    return
      Math.min(
        (escrow.balance * (block.timestamp - uint256(escrow.lastUpdateTime))) /
          (uint256(escrow.end) - uint256(escrow.lastUpdateTime)),
        escrow.balance
      );
  }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L170-L185

<ins>Recommended Mitigation Steps</ins>

Include return parameters in NatSpec comments

Recommendation Code Style: (from Uniswap3)

    /// @notice Adds liquidity for the given recipient/tickLower/tickUpper position
    /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback
    /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends
    /// on tickLower, tickUpper, the amount of liquidity, and the current price.
    /// @param recipient The address for which the liquidity will be created
    /// @param tickLower The lower tick of the position in which to add liquidity
    /// @param tickUpper The upper tick of the position in which to add liquidity
    /// @param amount The amount of liquidity to mint
    /// @param data Any data that should be passed through to the callback
    /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback
    /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback
    function mint(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount,
        bytes calldata data
    ) external returns (uint256 amount0, uint256 amount1);

<a href="#Summary">[NC‑11]</a><a name="NC&#x2011;11"> Insufficient coverage

The test coverage rate of the project is 94.52%. Testing all functions is best practice in terms of security criteria. As stated in the docs: What is the overall line coverage percentage provided by your tests?: 94.52%

Due to its capacity, test coverage is expected to be 100%.

<a href="#Summary">[NC‑12]</a><a name="NC&#x2011;12"> Lines are too long

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 Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

<ins>Proof Of Concept</ins>

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L380

<a href="#Summary">[NC‑13]</a><a name="NC&#x2011;13"> Missing event for critical parameter change

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

<ins>Proof Of Concept</ins>

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L296

335: function changeVaultAdapters(address[] calldata vaults) external {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L335

372: function changeVaultFees(address[] calldata vaults) external {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L372

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L408

483: function changeStakingRewardsSpeeds(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L483

543: function setEscrowTokenFees(IERC20[] calldata tokens, uint256[] calldata fees) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L543

764: function setAdapterPerformanceFees(address[] calldata adapters) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L764

804: function setAdapterHarvestCooldowns(address[] calldata adapters) external onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L804

<a href="#Summary">[NC‑14]</a><a name="NC&#x2011;14"> Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

<ins>Proof Of Concept</ins>

30: constructor(address _owner, address _feeRecipient) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L30

16: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/AdminProxy.sol#L16

23: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L23

22: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneRegistry.sol#L22

35: constructor(
    address _owner,
    ICloneFactory _cloneFactory,
    ICloneRegistry _cloneRegistry,
    ITemplateRegistry _templateRegistry
  ) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L35

20: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L20

24: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L24

53: constructor(
    address _owner,
    IAdminProxy _adminProxy,
    IDeploymentController _deploymentController,
    IVaultRegistry _vaultRegistry,
    IPermissionRegistry _permissionRegistry,
    IMultiRewardEscrow _escrow
  ) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L53

21: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L21

<a href="#Summary">[NC‑15]</a><a name="NC&#x2011;15"> NatSpec comments should be increased in contracts

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

<ins>Proof Of Concept</ins>

All in-scope contracts

i.e. missing NATSPEC

  function _getClaimableAmount(Escrow memory escrow) internal view returns (uint256) {
    if (
      escrow.lastUpdateTime == 0 ||
      escrow.end == 0 ||
      escrow.balance == 0 ||
      block.timestamp.safeCastTo32() < escrow.start
    ) {
      return 0;
    }
    return
      Math.min(
        (escrow.balance * (block.timestamp - uint256(escrow.lastUpdateTime))) /
          (uint256(escrow.end) - uint256(escrow.lastUpdateTime)),
        escrow.balance
      );
  }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L170-L185

<ins>Recommended Mitigation Steps</ins>

NatSpec comments should be increased in contracts

<a href="#Summary">[NC‑16]</a><a name="NC&#x2011;16"> Use a more recent version of Solidity

<a href="https://blog.soliditylang.org/2022/08/08/solidity-0.8.16-release-announcement/">0.8.16</a>:

Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

<a href="https://blog.soliditylang.org/2022/09/08/solidity-0.8.17-release-announcement/">0.8.17</a>:

Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

<ins>Proof Of Concept</ins>

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IEIP165.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardEscrow.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IAdapter.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IAdminProxy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ICloneFactory.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ICloneRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IDeploymentController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IERC4626.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IPermissionRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IStrategy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ITemplateRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVaultController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVaultRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IWithRewards.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/EIP165.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/AdminProxy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/OnlyStrategy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/IBeefy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/IYearn.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L2

<ins>Recommended Mitigation Steps</ins>

Consider updating to a more recent solidity version.

<a href="#Summary">[NC‑17]</a><a name="NC&#x2011;17"> Open TODOs

An open TODO is present. It is recommended to avoid open TODOs as they may indicate programming errors that still need to be fixed.

<ins>Proof Of Concept</ins>

516: // TODO use deterministic fee recipient proxy

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L516

<a href="#Summary">[NC‑18]</a><a name="NC&#x2011;18"> Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

<ins>Proof Of Concept</ins>

104: bytes32 id = keccak256(abi.encodePacked(token, account, amount, nonce)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L104

<ins>Recommended Mitigation Steps</ins>

Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.

<a href="#Summary">[NC‑19]</a><a name="NC&#x2011;19"> Use of Block.Timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116

<ins>Proof Of Concept</ins>

356: if (rewardsEndTimestamp > block.timestamp)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L356

392: if (rewards.rewardsEndTimestamp > block.timestamp) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L392

454: if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L454

488: if (managementFee > 0) feesUpdatedAt = block.timestamp;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L488

541: if (block.timestamp < proposedFeeTime + quitPeriod)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L541

595: if (block.timestamp < proposedAdapterTime + quitPeriod)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L595

673: if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L673

641: if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L641

<ins>Recommended Mitigation Steps</ins>

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

<a href="#Summary">[NC‑20]</a><a name="NC&#x2011;20"> Incomplete functions

These functions are incomplete, if not used then they should be removed.

<ins>Proof Of Concept</ins>

    /// @notice deposit into the underlying protocol.
    function _protocolDeposit(uint256 assets, uint256 shares) internal virtual {
        // OPTIONAL - convertIntoUnderlyingShares(assets,shares)
    }

    /// @notice Withdraw from the underlying protocol.
    function _protocolWithdraw(uint256 assets, uint256 shares)
        internal
        virtual
    {
        // OPTIONAL - convertIntoUnderlyingShares(assets,shares)
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L593-L604

Summary<a name="Summary">

Gas Optimizations

Total: 384 contexts over 21 issues

Gas Optimizations

<a href="#Summary">[GAS‑1]</a><a name="GAS&#x2011;1"> abi.encode() is less efficient than abi.encodepacked()

See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

<ins>Proof Of Concept</ins>

465: abi.encode(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L465

494: abi.encode(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L494

684: abi.encode(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L684

719: abi.encode(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L719

219: abi.encode(asset, address(adminProxy), IStrategy(strategy), harvestCooldown, requiredSigs, strategyData.data),

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L219

652: abi.encode(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L652

687: abi.encode(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L687

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

<ins>Proof Of Concept</ins>

31: feeRecipient = _feeRecipient

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L31

41: cloneFactory = _cloneFactory

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L41

42: cloneRegistry = _cloneRegistry

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L42

43: templateRegistry = _templateRegistry

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L43

61: adminProxy = _adminProxy

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L61

62: vaultRegistry = _vaultRegistry

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L62

63: permissionRegistry = _permissionRegistry

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L63

64: escrow = _escrow

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L64

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> Setting the constructor to payable

Saves ~13 gas per instance

<ins>Proof Of Concept</ins>

30: constructor(address _owner, address _feeRecipient) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L30

16: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/AdminProxy.sol#L16

23: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L23

22: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneRegistry.sol#L22

35: constructor(
    address _owner,
    ICloneFactory _cloneFactory,
    ICloneRegistry _cloneRegistry,
    ITemplateRegistry _templateRegistry
  ) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L35

20: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L20

24: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L24

53: constructor(
    address _owner,
    IAdminProxy _adminProxy,
    IDeploymentController _deploymentController,
    IVaultRegistry _vaultRegistry,
    IPermissionRegistry _permissionRegistry,
    IMultiRewardEscrow _escrow
  ) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L53

21: constructor(address _owner) Owned(_owner)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L21

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

<ins>Proof Of Concept</ins>

25: uint256 constant DEGRADATION_COEFFICIENT = 10**18;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L25

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> Empty Blocks Should Be Removed Or Emit Something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

<ins>Proof Of Concept</ins>

7: function supportsInterface(bytes4 interfaceId) public view virtual returns (bool) {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/EIP165.sol#L7

477: {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L477

258: function _totalAssets() internal view virtual returns (uint256) {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L258

265: function _underlyingBalance() internal view virtual returns (uint256) {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L265

258: {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L258

265: {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L265

276: {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L276

13: function rewardTokens() external view virtual returns (address[] memory) {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L13

15: function claim() public virtual onlyStrategy {}

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L15

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> Using delete statement can save gas

<ins>Proof Of Concept</ins>

186: accruedRewards[user][_rewardTokens[i]] = 0;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L186

577: underlyingBalance = 0;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L577

<a href="#Summary">[GAS‑7]</a><a name="GAS&#x2011;7"> Use assembly to write address storage values

<ins>Proof Of Concept</ins>

372: _rewardTokens = rewardTokens;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L372

98: _deploymentController = deploymentController;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L98

318: _cloneRegistry = cloneRegistry;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L318

147: _bestVault = yVault;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L147

<a href="#Summary">[GAS‑8]</a><a name="GAS&#x2011;8"> Use hardcode address instead address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry's script.sol and solmate's LibRlp.sol contracts can help achieve this.

References: https://book.getfoundry.sh/reference/forge-std/compute-create-address

https://twitter.com/transmissions11/status/1518507047943245824

<ins>Proof Of Concept</ins>

100: token.safeTransferFrom(msg.sender, address(this), amount);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L100

113: IERC20(asset()).safeTransferFrom(caller, address(this), assets);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L113

259: rewardToken.safeTransferFrom(msg.sender, address(this), amount);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L259

305: uint256 remainder = rewardToken.balanceOf(address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L305

334: rewardToken.safeTransferFrom(msg.sender, address(this), amount);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L334

499: address(this)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L499

153: asset.safeTransferFrom(msg.sender, address(this), assets);
155: adapter.deposit(assets, address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L153

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L155

193: asset.safeTransferFrom(msg.sender, address(this), assets);
195: adapter.deposit(assets, address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L193

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L195

237: adapter.withdraw(assets, receiver, address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L237

275: adapter.withdraw(assets, receiver, address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L275

286: return adapter.convertToAssets(adapter.balanceOf(address(this)));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L286

599: adapter.balanceOf(address(this)),
600: address(this),
599: address(this)
612: adapter.deposit(asset.balanceOf(address(this)), address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L599

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L600

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L599

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L612

726: address(this)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L726

170: asset.safeTransferFrom(msg.sender, address(this), initialDeposit);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L170

526: rewardTokens[i].transferFrom(msg.sender, address(this), amounts[i]);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L526

153: IERC20(asset()).safeTransferFrom(caller, address(this), assets);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L153

250: ? IERC20(asset()).balanceOf(address(this))

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L250

694: address(this)

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L694

118: return beefyBalanceCheck.balanceOf(address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L118

199: beefyBooster.stake(beefyVault.balanceOf(address(this)));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L199

85: return yVault.balanceOf(address(this));

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L85

<ins>Recommended Mitigation Steps</ins>

Use hardcoded address

<a href="#Summary">[GAS‑9]</a><a name="GAS&#x2011;9"> internal functions only called once can be inlined to save gas

<ins>Proof Of Concept</ins>

137: function _transfer

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L137

191: function _lockToken

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L191

120: function _deployVault

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L120

141: function _registerCreatedVault

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L141

154: function _handleVaultStakingRewards

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L154

225: function __deployAdapter

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L225

242: function _encodeAdapterData

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L242

256: function _deployStrategy

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L256

390: function _registerVault

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L390

692: function _verifyAdapterConfiguration

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L692

479: function _verifyAndSetupStrategy

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L479

101: function _freeFunds

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L101

109: function _yTotalAssets

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L109

114: function _calculateLockedProfit

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L114

<a href="#Summary">[GAS‑10]</a><a name="GAS&#x2011;10"> Multiple Address Mappings Can Be Combined Into A Single Mapping Of An Address To A Struct, Where Appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

<ins>Proof Of Concept</ins>

28: mapping(address => VaultMetadata) public metadata;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L28

31: mapping(address => address[]) public vaultsByAsset;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L31

<ins>Recommended Mitigation Steps</ins>

Instead can use:

struct structExample{
    VaultMetadata metadata;
    address[] vaultsByAsset;
}



### <a href="#Summary">[GAS&#x2011;11]</a><a name="GAS&#x2011;11"> Optimize names to save gas

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. 

See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>

#### <ins>Proof Of Concept</ins>

All in-scope contracts

#### <ins>Recommended Mitigation Steps</ins>
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 Gauge.sol contract will be the most used; A lower method ID may be given.



### <a href="#Summary">[GAS&#x2011;12]</a><a name="GAS&#x2011;12"> `<x> += <y>` Costs More Gas Than `<x> = <x> + <y>` For State Variables

#### <ins>Proof Of Concept</ins>


```solidity
110: amount -= fee;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L110

162: escrows[escrowId].balance -= claimable;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L162

357: amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L357

408: rewardInfos[_rewardToken].index += deltaIndex;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L408

431: accruedRewards[_user][_rewardToken] += supplierDelta;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L431

228: shares += feeShares;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L228

343: shares += shares.mulDiv(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L343

365: assets += assets.mulDiv(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L365

387: assets -= assets.mulDiv(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L387

158: underlyingBalance += _underlyingBalance() - underlyingBalance_;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L158

225: underlyingBalance -= underlyingBalance_ - _underlyingBalance();

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L225

178: assets -= assets.mulDiv(

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L178

<a href="#Summary">[GAS‑13]</a><a name="GAS&#x2011;13"> Public Functions To External

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

<ins>Proof Of Concept</ins>

function supportsInterface(bytes4 interfaceId) public view virtual returns (bool) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/EIP165.sol#L7

function name() public view override(ERC20Upgradeable, IERC20Metadata) returns (string memory) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L59

function symbol() public view override(ERC20Upgradeable, IERC20Metadata) returns (string memory) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L63

function decimals() public view override(ERC20Upgradeable, IERC20Metadata) returns (uint8) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L67

function permit(
    address owner,
    address spender,
    uint256 value,
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
  ) public virtual {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L445

function DOMAIN_SEPARATOR() public view returns (bytes32) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L487

function decimals() public view override returns (uint8) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L100

function deposit(uint256 assets) public returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L124

function withdraw(uint256 assets) public returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L200

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public nonReentrant syncFeeCheckpoint returns (uint256 shares) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L200

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L242

function totalAssets() public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L285

function convertToShares(uint256 assets) public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L294

function convertToAssets(uint256 shares) public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L308

function previewMint(uint256 shares) public view returns (uint256 assets) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L340

function maxDeposit(address caller) public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L399

function accruedManagementFee() public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L429

function accruedPerformanceFee() public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L447

function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L664

function DOMAIN_SEPARATOR() public view returns (bytes32) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L709

function addStakingRewardsTokens(address[] memory vaults, bytes[] memory rewardTokenData) public {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L433

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L173

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L193

function totalAssets() public view override returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L247

function maxMint(address) public view virtual override returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L419

function harvest() public takeFees {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L438

function accruedPerformanceFee() public view returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L529

function setPerformanceFee(uint256 newFee) public onlyOwner {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L549

function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L632

function DOMAIN_SEPARATOR() public view virtual returns (bytes32) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L677

function claim() public virtual onlyStrategy {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L15

function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L21

function claim() public override onlyStrategy {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L221

function maxDeposit(address) public view override returns (uint256) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L144

<a href="#Summary">[GAS‑14]</a><a name="GAS&#x2011;14"> Help The Optimizer By Saving A Storage Variable's Reference Instead Of Repeatedly Fetching It

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

<ins>Proof Of Concept</ins>

157: Escrow memory escrow = escrows[escrowId];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L157

172: uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L172

186: accruedRewards[user][_rewardTokens[i]] = 0;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L186

375: RewardInfo memory rewards = rewardInfos[rewardToken];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L375

<a href="#Summary">[GAS‑15]</a><a name="GAS&#x2011;15"> Superfluous event fields

block.number and block.timestamp are added to the event information by default, so adding them manually will waste additional gas.

<ins>Proof Of Concept</ins>

512: event NewFeesProposed(VaultFees newFees, uint256 timestamp);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L512

569: event NewAdapterProposed(IERC4626 newAdapter, uint256 timestamp);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L569

<a href="#Summary">[GAS‑16]</a><a name="GAS&#x2011;16"> Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a 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

<ins>Proof Of Concept</ins>

11: uint32 start;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardEscrow.sol#L11

13: uint32 end;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardEscrow.sol#L13

15: uint32 lastUpdateTime;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardEscrow.sol#L15

16: uint64 ONE;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L16

18: uint160 rewardsPerSecond;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L18

21: uint32 rewardsEndTimestamp;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L21

23: uint224 index;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L23

25: uint32 lastUpdatedTimestamp;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L25

30: uint192 escrowPercentage;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L30

32: uint32 escrowDuration;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L32

34: uint32 offset;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L34

9: uint64 deposit;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L9

10: uint64 withdrawal;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L10

11: uint64 management;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L11

12: uint64 performance;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L12

114: uint32 start = block.timestamp.safeCastTo32() + offset;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L114

33: uint8 private _decimals;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L33

274: uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L274

307: uint32 prevEndTime = rewards.rewardsEndTimestamp;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L307

340: uint32 rewardsEndTimestamp = rewards.rewardsEndTimestamp;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L340

404: uint224 deltaIndex;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L404

38: uint8 internal _decimals;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L38

645: uint8 len = uint8(vaults.length);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L645

583: uint8 len = uint8(templateCategories.length);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L583

805: uint8 len = uint8(adapters.length);

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L805

38: uint8 internal _decimals;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L38

238: interfaceId == type(IAdapter).interfaceId;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L238

<a href="#Summary">[GAS‑17]</a><a name="GAS&#x2011;17"> ++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

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

<ins>Proof Of Concept</ins>

53: for (uint256 i = 0; i < escrowIds.length; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L53

155: for (uint256 i = 0; i < escrowIds.length; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L155

210: for (uint256 i = 0; i < tokens.length; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L210

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L171

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L373

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L42

319: for (uint8 i = 0; i < len; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L319

337: for (uint8 i = 0; i < len; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L337

357: for (uint8 i = 0; i < len; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L357

374: for (uint8 i = 0; i < len; i++) {

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L374

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L437

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L494

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L523

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L564

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L587

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L607

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L620

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L633

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L646

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L766

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

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L806

<a href="#Summary">[GAS‑18]</a><a name="GAS&#x2011;18"> Use v4.8.1 OpenZeppelin contracts

The upcoming v4.8.1 version of OpenZeppelin provides many small gas optimizations. https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.8.1

<ins>Proof Of Concept</ins>

    "openzeppelin-upgradeable": "npm:@openzeppelin/contracts-upgradeable@4.7.1"

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/../package.json#L26

<ins>Recommended Mitigation Steps</ins>

Update OpenZeppelin Contracts Usage in package.json and require at least version of 4.8.1

<a href="#Summary">[GAS‑19]</a><a name="GAS&#x2011;19"> Use solidity version 0.8.17 to gain some gas boost

Upgrade to the latest solidity version 0.8.17 to get additional gas savings.

<ins>Proof Of Concept</ins>

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IEIP165.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardEscrow.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/IMultiRewardStaking.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IAdapter.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IAdminProxy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ICloneFactory.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ICloneRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IDeploymentController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IERC4626.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IPermissionRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IStrategy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/ITemplateRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVault.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVaultController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IVaultRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/interfaces/vault/IWithRewards.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/EIP165.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/AdminProxy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneFactory.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/CloneRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/DeploymentController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/PermissionRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/TemplateRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/Vault.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultController.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/VaultRegistry.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/AdapterBase.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/OnlyStrategy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/abstracts/WithRewards.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/BeefyAdapter.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/beefy/IBeefy.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/IYearn.sol#L2

pragma solidity ^0.8.15;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L2

<a href="#Summary">[GAS‑20]</a><a name="GAS&#x2011;20"> Using storage instead of memory saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

<ins>Proof Of Concept</ins>

157: Escrow memory escrow = escrows[escrowId];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardEscrow.sol#L157

176: EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L176

254: RewardInfo memory rewards = rewardInfos[rewardToken];
297: RewardInfo memory rewards = rewardInfos[rewardToken];
328: RewardInfo memory rewards = rewardInfos[rewardToken];
375: RewardInfo memory rewards = rewardInfos[rewardToken];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L254

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L297

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L328

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L375

254: RewardInfo memory rewards = rewardInfos[rewardToken];
297: RewardInfo memory rewards = rewardInfos[rewardToken];
328: RewardInfo memory rewards = rewardInfos[rewardToken];
375: RewardInfo memory rewards = rewardInfos[rewardToken];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L254

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L297

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L328

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L375

254: RewardInfo memory rewards = rewardInfos[rewardToken];
297: RewardInfo memory rewards = rewardInfos[rewardToken];
328: RewardInfo memory rewards = rewardInfos[rewardToken];
375: RewardInfo memory rewards = rewardInfos[rewardToken];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L254

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L297

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L328

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L375

254: RewardInfo memory rewards = rewardInfos[rewardToken];
297: RewardInfo memory rewards = rewardInfos[rewardToken];
328: RewardInfo memory rewards = rewardInfos[rewardToken];
375: RewardInfo memory rewards = rewardInfos[rewardToken];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L254

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L297

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L328

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L375

414: RewardInfo memory rewards = rewardInfos[_rewardToken];

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/utils/MultiRewardStaking.sol#L414

<a href="#Summary">[GAS‑21]</a><a name="GAS&#x2011;21"> Using 10**X for constants isn't gas efficient

In Solidity, a constant expression in a variable will compute the expression everytime the variable is called. It's not the result of the expression that is stored, but the expression itself.

As Solidity supports the scientific notation, constants of form 10**X can be rewritten as 1eX to save the gas cost from the calculation with the exponentiation operator **.

<ins>Proof Of Concept</ins>

25: uint256 constant DEGRADATION_COEFFICIENT = 10**18;

https://github.com/code-423n4/2023-01-popcorn/tree/main/src/vault/adapter/yearn/YearnAdapter.sol#L25

<ins>Recommended Mitigation Steps</ins>

Replace 10**X with 1eX