Popcorn contest - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

The claimRewards function in the MultiRewardStaking contract is used by users to claim token rewards, but because the function does not contain a nonReentrant modifier and does not implement the CEI standard (check-effect-interact) it can be subject to reentrancy attacks.

In fact any user can call the function to claim a given amount of reward tokens and then immediately reenter the function to claim more tokens than his actual accrued rewards amount and he can even drain the whole contract token balance.

Proof of Concept

The issue occurs in the claimRewards function :

File: utils/MultiRewardStaking.sol Line 170-188

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];
    
      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);
    
      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];
    
      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }
        
      /** @audit
        should be placed before the token transfer
      */    
      accruedRewards[user][_rewardTokens[i]] = 0;
    }
}

As you can see the function updates the user's accrued balance value after the rewards transfer thus the user can reenter the function multiple times to claim more rewards than he is supposed to and he can by doing so drain the contract balance for a given token.

Tools Used

Manula review

Recommended Mitigation Steps

To avoid this issue add a nonReentrant modifier to the claimRewards function and follow the CEI standard :

function claimRewards(address user, IERC20[] memory _rewardTokens) external nonReentrant accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];
    
      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);
    
      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];
      
      /** @audit
        update the user accrued rewards immediately
      */    
      accruedRewards[user][_rewardTokens[i]] = 0;
      
      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }
    }
}

Judge has assessed an item in Issue #664 as 2 risk. The relevant finding follows:

2- Vault fees can be set greater than 1e18 in the initialize function : The Vaut contract implements 4 types of fees (deposit, withdrawal, management, performance) collected when the user deposits or withdraw tokens, those fees are calculated in basis point (BPS) where 1e18 is equivalent to 100% of the given amount.

The function proposeFees is used to set the value for new fees and it ensures that their respective values are always less than 1e18, but when the vault is initialized during deployment the initialize function (of the Vault contract) does not check the value of the fees set and thus it allows fees that are greater than 1e18.

This does not have a big impact as users can't call the deposit or mint functions (as they both underflow in this case) but this will block the vault until the owner proposes new fees and then you need to wait until the quitPeriod period has passed to update the fees and open the vault.

Risk : Low Proof of Concept The issue occurs in the initialize which does not contain any check for the value of fees :

File: vault/Vault.sol Line 57-98

function initialize( IERC20 asset_, IERC4626 adapter_, VaultFees calldata fees_, address feeRecipient_, address owner ) external initializer { ERC20_init( string.concat( "Popcorn ", IERC20Metadata(address(asset)).name(), " Vault" ), string.concat("pop-", IERC20Metadata(address(asset)).symbol()) ); __Owned_init(owner);

if (address(asset_) == address(0)) revert InvalidAsset();
if (address(asset_) != adapter_.asset()) revert InvalidAdapter();

asset = asset_;
adapter = adapter_;

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

_decimals = IERC20Metadata(address(asset_)).decimals();

INITIAL_CHAIN_ID = block.chainid;
INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();

feesUpdatedAt = block.timestamp;
fees = fees_;

if (feeRecipient_ == address(0)) revert InvalidFeeRecipient();
feeRecipient = feeRecipient_;

contractName = keccak256(
    abi.encodePacked("Popcorn", name(), block.timestamp, "Vault")
);

emit VaultInitialized(contractName, address(asset));

} As you can see the fees are set immediately without any check on their values.

Mitigation Add a check in the initialize function of the Vault contract to ensure that the fees are always less than 1e18.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L243-L288 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L178-L181 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L191-L202

Vulnerability details

Impact

In the MultiRewardStaking contract when the addRewardToken function is called by the owner he has the option to set an escrowPercentage which represent the percentage of rewards locked in the escrow contract when the claimRewards function is called, the value of escrowPercentage is supposed to be in basis point where 1e18 represents 100%.

The issue occurs if the owner by accident (or intentionnally) sets the escrowPercentage value greater than 1e18, this will cause the function _lockToken to always revert due to an underflow which will block the rewards withdrawal in the contract.

And because the escrowPercentage can not be updated after the call to addRewardToken, the rewards withdrawal process will remain blocked forever.

Proof of Concept

The issue occurs when the escrowPercentage value is set in the addRewardToken function :

File: utils/MultiRewardStaking.sol Line 243-288

function addRewardToken(
    IERC20 rewardToken,
    uint160 rewardsPerSecond,
    uint256 amount,
    bool useEscrow,
    uint192 escrowPercentage,
    uint32 escrowDuration,
    uint32 offset
  ) external onlyOwner {
    if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken();

    RewardInfo memory rewards = rewardInfos[rewardToken];
    if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);

    if (amount > 0) {
      if (rewardsPerSecond == 0) revert ZeroRewardsSpeed();
      rewardToken.safeTransferFrom(msg.sender, address(this), amount);
    }

    // Add the rewardToken to all existing rewardToken
    rewardTokens.push(rewardToken);

    if (useEscrow) {
      escrowInfos[rewardToken] = EscrowInfo({
        escrowPercentage: escrowPercentage,
        escrowDuration: escrowDuration,
        offset: offset
      });
      rewardToken.safeApprove(address(escrow), type(uint256).max);
    }

    uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
    uint32 rewardsEndTimestamp = rewardsPerSecond == 0
      ? block.timestamp.safeCastTo32()
      : _calcRewardsEnd(0, rewardsPerSecond, amount);

    rewardInfos[rewardToken] = RewardInfo({
      ONE: ONE,
      rewardsPerSecond: rewardsPerSecond,
      rewardsEndTimestamp: rewardsEndTimestamp,
      index: ONE,
      lastUpdatedTimestamp: block.timestamp.safeCastTo32()
    });

    emit RewardInfoUpdate(rewardToken, rewardsPerSecond, rewardsEndTimestamp);
  }

As you can see if useEscrow is set to true the escrowPercentage value is set directly without any check on its value which then allows the owner to set it greater than 1e18.

When later a user tries to claim its rewards for that given token he will call the claimRewards function which contain the following line of code :

File: utils/MultiRewardStaking.sol Line 178-181

if (escrowInfo.escrowPercentage > 0) {
    _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
    emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
} 

So if escrowInfo.escrowPercentage is greater than zero the internal function _lockToken is called which in turn contains the following code :

File: utils/MultiRewardStaking.sol Line 191-202

function _lockToken(
    address user,
    IERC20 rewardToken,
    uint256 rewardAmount,
    EscrowInfo memory escrowInfo
  ) internal {
    uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
    /** @audit
        This operation will underflow if escrowPercentage > 1e18
    */
    uint256 payout = rewardAmount - escrowed;

    rewardToken.safeTransfer(user, payout);
    escrow.lock(rewardToken, user, escrowed, escrowInfo.escrowDuration, escrowInfo.offset);
  }

The operation which calculate the final payout amount payout will underflow because when we have escrowInfo.escrowPercentage > 1e18 the value of escrowed will be greater than the rewardAmount.

And thus the function will revert and the user won't be able to claim his rewards.

Finally, because the MultiRewardStaking contract does not contain any function to update the value of escrowPercentage for a given token, the users will not be able to claim their rewards forever as the call to claimRewards function will always revert.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue add a check in the addRewardToken function to ensure that the value of escrowPercentage is always less or equal to 1e18 :

function addRewardToken(
    IERC20 rewardToken,
    uint160 rewardsPerSecond,
    uint256 amount,
    bool useEscrow,
    uint192 escrowPercentage,
    uint32 escrowDuration,
    uint32 offset
  ) external onlyOwner {
    if (asset() == address(rewardToken)) revert RewardTokenCantBeStakingToken();

    RewardInfo memory rewards = rewardInfos[rewardToken];
    if (rewards.lastUpdatedTimestamp > 0) revert RewardTokenAlreadyExist(rewardToken);

    if (amount > 0) {
      if (rewardsPerSecond == 0) revert ZeroRewardsSpeed();
      rewardToken.safeTransferFrom(msg.sender, address(this), amount);
    }

    // Add the rewardToken to all existing rewardToken
    rewardTokens.push(rewardToken);

    if (useEscrow) {
      /** @audit
         Add a check on the escrowPercentage value
      */
      if (escrowPercentage > 1e18) revert InvalidPercentage(escrowPercentage);
      
      escrowInfos[rewardToken] = EscrowInfo({
        escrowPercentage: escrowPercentage,
        escrowDuration: escrowDuration,
        offset: offset
      });
      rewardToken.safeApprove(address(escrow), type(uint256).max);
    }

    uint64 ONE = (10**IERC20Metadata(address(rewardToken)).decimals()).safeCastTo64();
    uint32 rewardsEndTimestamp = rewardsPerSecond == 0
      ? block.timestamp.safeCastTo32()
      : _calcRewardsEnd(0, rewardsPerSecond, amount);

    rewardInfos[rewardToken] = RewardInfo({
      ONE: ONE,
      rewardsPerSecond: rewardsPerSecond,
      rewardsEndTimestamp: rewardsEndTimestamp,
      index: ONE,
      lastUpdatedTimestamp: block.timestamp.safeCastTo32()
    });

    emit RewardInfoUpdate(rewardToken, rewardsPerSecond, rewardsEndTimestamp);
  }

QA Report

Summary

Findings

1- Existing clone can be added multiple times in CloneRegistry :

The addClone function from the CloneRegistry contract does not check if the input clone address already exists or not and it proceeds immediatly to adding the clone address to the clones and allClones arrays thus creating duplicates addresses inside them, in the current protocol implementation this does not cause any issue as only the DeploymentController is allowed to call the addClone function which actually ensures that only unique clones are added but if the contracts get upgraded in the future and the function caller (other parties get allowed to call the function) changes this may create a surface of attack or could lead to wrong protocol behaviour.

Risk : Low

Proof of Concept

The issue occurs in the addClone below :

File: vault/CloneRegistry.sol Line 41-51

function addClone(
    bytes32 templateCategory,
    bytes32 templateId,
    address clone
) external onlyOwner {
    cloneExists[clone] = true;
    clones[templateCategory][templateId].push(clone);
    allClones.push(clone);

    emit CloneAdded(clone);
}

The function does not check if the clone already exists meaning that cloneExists[clone] == true and thus can add the same clone multiple times.

Mitigation

Add a clone existance check in the addClone function :

function addClone(
    bytes32 templateCategory,
    bytes32 templateId,
    address clone
) external onlyOwner {
    if (cloneExists[clone]) revert AlreadyExists(clone);

    cloneExists[clone] = true;
    clones[templateCategory][templateId].push(clone);
    allClones.push(clone);

    emit CloneAdded(clone);
}

2- Vault fees can be set greater than 1e18 in the initialize function :

The Vaut contract implements 4 types of fees (deposit, withdrawal, management, performance) collected when the user deposits or withdraw tokens, those fees are calculated in basis point (BPS) where 1e18 is equivalent to 100% of the given amount.

The function proposeFees is used to set the value for new fees and it ensures that their respective values are always less than 1e18, but when the vault is initialized during deployment the initialize function (of the Vault contract) does not check the value of the fees set and thus it allows fees that are greater than 1e18.

This does not have a big impact as users can't call the deposit or mint functions (as they both underflow in this case) but this will block the vault until the owner proposes new fees and then you need to wait until the quitPeriod period has passed to update the fees and open the vault.

Risk : Low

Proof of Concept

The issue occurs in the initialize which does not contain any check for the value of fees :

File: vault/Vault.sol Line 57-98

function initialize(
    IERC20 asset_,
    IERC4626 adapter_,
    VaultFees calldata fees_,
    address feeRecipient_,
    address owner
) external initializer {
    __ERC20_init(
        string.concat(
            "Popcorn ",
            IERC20Metadata(address(asset_)).name(),
            " Vault"
        ),
        string.concat("pop-", IERC20Metadata(address(asset_)).symbol())
    );
    __Owned_init(owner);

    if (address(asset_) == address(0)) revert InvalidAsset();
    if (address(asset_) != adapter_.asset()) revert InvalidAdapter();

    asset = asset_;
    adapter = adapter_;

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

    _decimals = IERC20Metadata(address(asset_)).decimals();

    INITIAL_CHAIN_ID = block.chainid;
    INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();

    feesUpdatedAt = block.timestamp;
    fees = fees_;

    if (feeRecipient_ == address(0)) revert InvalidFeeRecipient();
    feeRecipient = feeRecipient_;

    contractName = keccak256(
        abi.encodePacked("Popcorn", name(), block.timestamp, "Vault")
    );

    emit VaultInitialized(contractName, address(asset));
}

As you can see the fees are set immediately without any check on their values.

Mitigation

Add a check in the initialize function of the Vault contract to ensure that the fees are always less than 1e18.

3- harvestCooldown value not verified in the __AdapterBase_init function :

The harvestCooldown variable is used to determine the time delay between two harvest operation and by the protocol specifications it is supposed to always be less than 1 day and this is ensured in the function setHarvestCooldown, but when the adapter contract is initially deployed the initializer function __AdapterBase_init does not verify that the value of harvestCooldown provided is in fact less than 1 day and when deployed the value of harvestCooldown can have any value from 0 to 2^256(uint256).

This issue has low impact on the protocol as the owner can immediatly call setHarvestCooldown to update the value but it can still cause problems if the error is not noticed at first.

Risk : Low

Proof of Concept

The issue occurs in the __AdapterBase_init which does not contain any check for the value of harvestCooldown :

File: vault/adapter/abstracts/AdapterBase.sol Line 55-87

function __AdapterBase_init(bytes memory popERC4626InitData)
    internal
    onlyInitializing
{
    (
        address asset,
        address _owner,
        address _strategy,
        uint256 _harvestCooldown,
        bytes4[8] memory _requiredSigs,
        bytes memory _strategyConfig
    ) = abi.decode(
            popERC4626InitData,
            (address, address, address, uint256, bytes4[8], bytes)
        );
    __Owned_init(_owner);
    __Pausable_init();
    __ERC4626_init(IERC20Metadata(asset));

    INITIAL_CHAIN_ID = block.chainid;
    INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();

    _decimals = IERC20Metadata(asset).decimals();

    strategy = IStrategy(_strategy);
    strategyConfig = _strategyConfig;
    harvestCooldown = _harvestCooldown;

    if (_strategy != address(0)) _verifyAndSetupStrategy(_requiredSigs);

    highWaterMark = 1e18;
    lastHarvest = block.timestamp;
}

Mitigation

Add a check in the __AdapterBase_init function to ensure that harvestCooldown < 1 days or call the function setHarvestCooldown directly inside the __AdapterBase_init function.

4- feeRecipient can be set to address(0) :

The address of feeRecipient in the MultiRewardEscrow contract can be set by accident to address(0) in the constructor.

Risk : Low

Proof of Concept

Instances include:

File: utils/MultiRewardEscrow.sol Line 31

feeRecipient = _feeRecipient;

Mitigation

Add non-zero address checks in the constructor of the MultiRewardEscrow contract.

5- The function getSubmitter should return the vault creator address :

In the VaultRegistry contract the function getSubmitter is supposed to return the address of the vault creator when it is called but instead it returns the whole vault struct which doesn't make sense as there is already the getVault function which is responsible for that, this issue is not really critical for the protocol as the function is marked as view and is not used in other contracts but this error could cause problems for others parties who tries to integrate with the protocol as the getSubmitter function put in the IVaultRegistry interface actually returns an address.

Risk : Non Critical

Proof of Concept

Issue occurs in the instance below :

File: vault/VaultRegistry.sol Line 75-77

/** @audit
    should return metadata[vault].creator
*/
function getSubmitter(address vault) external view returns (VaultMetadata memory) {
    return metadata[vault];
}

And the IVaultRegistry interface also confirms that the function should return an address :

File: interfaces/vault/IVaultRegistry.sol Line 27

function getSubmitter(address vault) external view returns (address);

Mitigation

Return the address of the vault creator metadata[vault].creator in the getSubmitter function.

6- Duplicated input check statements should be refactored to a modifier :

Input check statements used multiple times inside a contract should be refactored to a modifier for better readability and also to save gas.

Risk : Non Critical

Proof of Concept

There are 4 instances of this issue :

File: vault/Vault.sol

141     if (receiver == address(0)) revert InvalidReceiver();
177     if (receiver == address(0)) revert InvalidReceiver();
216     if (receiver == address(0)) revert InvalidReceiver();
258     if (receiver == address(0)) revert InvalidReceiver();

Those checks should be replaced by a ValidReceiver modifier as follow :

modifier ValidReceiver(address receiver){
    if (receiver == address(0)) revert InvalidReceiver();
    _;
}

7- constant should be used instead of immutable :

The type constant should be used for variables that have values set immediately in the contract code and the immutable type should be used for variables declared in the constructor.

Risk : Non Critical

Proof of Concept

There are 4 instances of this issue :

File: vault/VaultController.sol Line 36-40

bytes32 public immutable VAULT = "Vault";
bytes32 public immutable ADAPTER = "Adapter";
bytes32 public immutable STRATEGY = "Strategy";
bytes32 public immutable STAKING = "Staking";
bytes4 internal immutable DEPLOY_SIG = bytes4(keccak256("deploy(bytes32,bytes32,bytes)"));

Mitigation

Use constant instead of immutable in the instances aforementioned.

Gas Optimizations

Summary

Findings

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

There are 8 instances of this issue:

File: vault/DeploymentController.sol Line 23-25

ICloneFactory public cloneFactory;
ICloneRegistry public cloneRegistry;
ITemplateRegistry public templateRegistry;

File: utils/MultiRewardEscrow.sol Line 191

address public feeRecipient;

File: vault/VaultController.sol Line 387

IVaultRegistry public vaultRegistry;

File: vault/VaultController.sol Line 535

IMultiRewardEscrow public escrow;

File: vault/VaultController.sol Line 717

IAdminProxy public adminProxy;

File: vault/VaultController.sol Line 822

IPermissionRegistry public permissionRegistry;

2- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

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. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 3 instances of this issue in TemplateRegistry :

File: vault/TemplateRegistry.sol

31      mapping(bytes32 => mapping(bytes32 => Template)) public templates;
32      mapping(bytes32 => bytes32[]) public templateIds;
35      mapping(bytes32 => bool) public templateCategoryExists;

These mappings could be refactored into the following struct and mapping for example :

struct TemplateCategory {
    bool templateCategoryExists;
    bytes32[] templateIds;
    mapping(bytes32 => Template) templates;
}
    
mapping(bytes32 => TemplateCategory) public templateCategoryMap;

There are 2 instances of this issue in MultiRewardEscrow :

File: utils/MultiRewardEscrow.sol

67      mapping(address => bytes32[]) public userEscrowIds;
69      mapping(address => mapping(IERC20 => bytes32[])) public userEscrowIdsByToken;

These mappings could be refactored into the following struct and mapping for example :

struct UserEscrow {
    bytes32[] userEscrowIds;
    mapping(IERC20 => bytes32[]) userEscrowIdsByToken;
}
    
mapping(address => UserEscrow) public userEscrows;

There are 4 instances of this issue in MultiRewardStaking :

File: utils/MultiRewardStaking.sol

211     mapping(IERC20 => RewardInfo) public rewardInfos;
213     mapping(IERC20 => EscrowInfo) public escrowInfos;

These mappings could be refactored into the following struct and mapping for example :

struct TokenInfo {
    RewardInfo rewardInfos;
    EscrowInfo escrowInfos;
}
    
mapping(IERC20 => TokenInfo) public infos;

The others instances are :

File: utils/MultiRewardStaking.sol

216     mapping(address => mapping(IERC20 => uint256)) public userIndex;
218     mapping(address => mapping(IERC20 => uint256)) public accruedRewards;

These mappings could be refactored into the following struct and mapping for example :

struct UserInfo {
    uint256 userIndex;
    uint256 accruedRewards;
}
    
mapping(address => mapping(IERC20 => UserInfo)) public userIndex;

3- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

There is 1 instance of this issue:

File: interfaces/vault/IVaultRegistry.sol Line 7-22

struct VaultMetadata {
  /// @notice Vault address
  address vault;
  /// @notice Staking contract for the vault
  address staking;
  /// @notice Owner and Vault creator
  address creator;
  /// @notice IPFS CID of vault metadata
  string metadataCID;
  /// @notice OPTIONAL - If the asset is an Lp Token these are its underlying assets
  address[8] swapTokenAddresses;
  /// @notice OPTIONAL - If the asset is an Lp Token its the pool address
  address swapAddress;
  /// @notice OPTIONAL - If the asset is an Lp Token this is the identifier of the exchange (1 = curve)
  uint256 exchange;
}

As the exchange variable represents an id for different exchanges it can't really overflow 2^96, so its value should be packed with the variable swapAddress (which only takes 20 bytes) in order to save gas, the struct should be modified as follow :

struct VaultMetadata {
  /// @notice Vault address
  address vault;
  /// @notice Staking contract for the vault
  address staking;
  /// @notice Owner and Vault creator
  address creator;
  /// @notice IPFS CID of vault metadata
  string metadataCID;
  /// @notice OPTIONAL - If the asset is an Lp Token these are its underlying assets
  address[8] swapTokenAddresses;
  /// @notice OPTIONAL - If the asset is an Lp Token its the pool address
  address swapAddress;
  /// @notice OPTIONAL - If the asset is an Lp Token this is the identifier of the exchange (1 = curve)
  uint96 exchange;
}

4- Using storage instead of memory for struct/array 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.

There are 2 instances of this issue :

File: utils/MultiRewardStaking.sol Line 372

IERC20[] memory _rewardTokens = rewardTokens;

File: utils/MultiRewardStaking.sol Line 414

RewardInfo memory rewards = rewardInfos[rewardToken];

5- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 3 instances of this issue:

File: utils/MultiRewardStaking.sol Line 392-396

if (rewards.rewardsEndTimestamp > block.timestamp) {
    elapsed = block.timestamp - rewards.lastUpdatedTimestamp;
} else if (rewards.rewardsEndTimestamp > rewards.lastUpdatedTimestamp) {
    elapsed = rewards.rewardsEndTimestamp - rewards.lastUpdatedTimestamp;
}

In code linked above both operation cannot underflow due to the checks that preceeds them so they both should be marked as unchecked.

File: vault/Vault.sol Line 147

shares = convertToShares(assets) - feeShares;

In code linked above the operation cannot underflow because the calculated fee value is always less than the share amount, so the operation should be marked as unchecked.

File: utils/MultiRewardEscrow.sol Line 110

amount -= fee;

In code linked above the operation cannot underflow because the calculated fee value is always less than the value of amount, so the operation should be marked as unchecked.

6- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 5 instances of this issue:

File: vault/PermissionRegistry.sol

158     underlyingBalance += _underlyingBalance() - underlyingBalance_;
225     underlyingBalance -= underlyingBalance_ - _underlyingBalance();

File: utils/MultiRewardEscrow.sol

162     escrows[escrowId].balance -= claimable;

File: utils/MultiRewardStaking.sol

408     rewardInfos[_rewardToken].index += deltaIndex;
431     accruedRewards[_user][_rewardToken] += supplierDelta;

7- Duplicated input check statements should be refactored to a modifier :

Input check statements used multiple times inside a contract should be refactored to a modifier for better readability and also to save gas.

There are 4 instances of this issue :

File: vault/Vault.sol

141     if (receiver == address(0)) revert InvalidReceiver();
177     if (receiver == address(0)) revert InvalidReceiver();
216     if (receiver == address(0)) revert InvalidReceiver();
258     if (receiver == address(0)) revert InvalidReceiver();

Those checks should be replaced by a ValidReceiver modifier as follow :

modifier ValidReceiver(address receiver){
    if (receiver == address(0)) revert InvalidReceiver();
    _;
}

8- Input check statements should be placed at the start of the functions :

The check statements on the functions input values should be placed at the beginning of the functions to avoid too deep stack errors and save gas.

There are 3 instances of this issue:

File: vault/Vault.sol Line 74-75

if (address(asset_) == address(0)) revert InvalidAsset();
if (address(asset_) != adapter_.asset()) revert InvalidAdapter();

File: vault/Vault.sol Line 90

if (feeRecipient_ == address(0)) revert InvalidFeeRecipient();

9- memory values should be emitted in events instead of storage ones :

The values emitted in events shouldn’t be read from storage but the existing memory values should be used instead, this will save ~101 GAS.

There is 1 instance of this issue :

File: vault/Vault.sol Line 635

emit QuitPeriodSet(quitPeriod);

The memory values should be emitted as follow :

emit QuitPeriodSet(_quitPeriod);

10- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 21 instances of this issue:

File: vault/PermissionRegistry.sol

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

File: vault/VaultController.sol

318     for (uint8 i = 0; i < len; i++)
337     for (uint8 i = 0; i < len; i++)
357     for (uint8 i = 0; i < len; i++)
374     for (uint8 i = 0; i < len; i++)
437     for (uint256 i = 0; i < len; i++) 
494     for (uint256 i = 0; i < len; i++)
523     for (uint256 i = 0; i < len; i++)
564     for (uint256 i = 0; i < len; i++)
587     for (uint256 i = 0; i < len; i++)
607     for (uint256 i = 0; i < len; i++)
620     for (uint256 i = 0; i < len; i++)
633     for (uint256 i = 0; i < len; i++)
646     for (uint256 i = 0; i < len; i++)
766     for (uint256 i = 0; i < len; i++)
806     for (uint256 i = 0; i < len; i++)

File: utils/MultiRewardEscrow.sol

53      for (uint256 i = 0; i < escrowIds.length; i++)
155     for (uint256 i = 0; i < escrowIds.length; i++)
210     for (uint256 i = 0; i < tokens.length; i++)

File; utils/MultiRewardStaking.sol

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