Gas Optimizations

[code423n4]

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instance:

    operator in TestableOperatorCaller.sol

Unused state variables

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

Code instances:

    WETHMock.sol, symbol
    WETHMock.sol, decimals
    TestableMixingOperatorResolver.sol, addressesToCache
    WETHMock.sol, name

Unused declared local variables

Unused local variables are gas consuming, since the initial value assignment costs gas. And are a bad code practice. Removing those variables will decrease the gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

Code instances:

    NestedAssetBatcher.sol, getNfts, amounts
    NestedAssetBatcher.sol, getNfts, nftAssets
    TestableOperatorCaller.sol, performSwap, data

Unnecessary array boundaries check when loading an array element twice

There are places in the code (especially in for-each loops) that loads the same array element more than once. 
In such cases, only one array boundaries check should take place, and the rest are unnecessary.
Therefore, this array element should be cached in a local variable and then be loaded
again using this local variable, skipping the redundant second array boundaries check: 

Code instances:

    NestedFactory.sol._processOutputOrders - double load of _batchedOrders[i]
    NestedFactory.sol._processInputOrders - double load of _batchedOrders[i]

Caching array length can save gas

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }

Code instances:

    MixinOperatorResolver.sol, requiredOperators, 56
    OperatorResolver.sol, destinations, 75
    MixinOperatorResolver.sol, requiredOperators, 37
    FeeSplitter.sol, _tokens, 147
    TimelockControllerEmergency.sol, targets, 234
    FeeSplitter.sol, shareholders, 316
    TimelockControllerEmergency.sol, targets, 324
    NestedFactory.sol, _batchedOrders, 651
    FeeSplitter.sol, _tokens, 164
    TimelockControllerEmergency.sol, proposers, 84
    OperatorResolver.sol, names, 60
    TimelockControllerEmergency.sol, executors, 89
    NestedFactory.sol, operatorsCache, 124
    FeeSplitter.sol, shareholdersCache, 278
    FeeSplitter.sol, shareholders, 259

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    change to prefix increment and unchecked: OperatorScripts.sol, i, 67
    change to prefix increment and unchecked: OperatorScripts.sol, i, 80
    change to prefix increment and unchecked: FeeSplitter.sol, i, 278
    change to prefix increment and unchecked: MixinOperatorResolver.sol, i, 56

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    NestedFactory.sol, 315
    OperatorResolver.sol, 75
    NestedFactory.sol, 333
    NestedFactory.sol, 196
    FeeSplitter.sol, 259

Rearrange state variables

You can change the order of the storage variables to decrease memory uses.

Code instance:

In OwnableProxyDelegation.sol,rearranging the storage fields can optimize to: 2 slots from: 3 slots. The new order of types (you choose the actual variables):

1. bytes32
2. address
3. bool

Use bytes32 instead of string to save gas whenever possible

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.

Code instances:

    WETHMock.sol (L25), string public symbol = "WETH";
    WETHMock.sol (L24), string public name = "Wrapped Ether";

Short the following require messages

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

Code instances:

    Solidity file: TimelockControllerEmergency.sol, In line 320, Require message length to shorten: 35, The message: TimelockController: length mismatch
    Solidity file: TimelockControllerEmergency.sol, In line 244, Require message length to shorten: 38, The message: TimelockController: insufficient delay
    Solidity file: TimelockControllerEmergency.sol, In line 229, Require message length to shorten: 35, The message: TimelockController: length mismatch
    Solidity file: TimelockControllerEmergency.sol, In line 335, Require message length to shorten: 38, The message: TimelockController: missing dependency
    Solidity file: TimelockControllerEmergency.sol, In line 319, Require message length to shorten: 35, The message: TimelockController: length mismatch
    Solidity file: TimelockControllerEmergency.sol, In line 230, Require message length to shorten: 35, The message: TimelockController: length mismatch

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    WETHMock.sol, 71: change 'balance > 0' to 'balance != 0'
    NestedFactory.sol, 544: change 'balance > 0' to 'balance != 0'
    WETHMock.sol, 46: change 'balance > 0' to 'balance != 0'

Unnecessary cast

Code instance:

    IERC20 DummyRouter.sol.dummyswapToken - unnecessary casting IERC20(_inputToken)

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instance:

    TimelockControllerEmergency.sol (L#245) - _timestamps[id] = block.timestamp + delay;

Empty else statement can be removed to save gas

    Empty else statement can be removed to save gas.

Code instance:

    StakingLPVaultHelpers.sol._addLiquidityAndDepositETH

Empty else if statement can be removed to save gas

    Empty else if statement can be removed to save gas by simply doing the following:

    if (a) {
        some code 1
    }
    else if (b) {
        empty
    } else {
        some code 2
    }

    change this pattern to:
    if (a) {
        some code 1
    }
    else if (!b) {
        some code 2
    } 

Code instance:

    StakingLPVaultHelpers.sol._addLiquidityAndDepositETH

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

    NestedAsset.sol, _baseURI, { return baseUri; }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    BeefyZapUniswapLPVaultOperator.sol, _swapAndAddLiquidity
    MixinOperatorResolver.sol, requireAndGetAddress
    NestedBuybacker.sol, trigger
    BeefyZapBiswapLPVaultOperator.sol, _swapAndAddLiquidity
    ExchangeHelpers.sol, setMaxAllowance
    FeeSplitter.sol, _addShareholder
    BeefyZapUniswapLPVaultOperator.sol, _zapAndStakeLp
    BeefyZapUniswapLPVaultOperator.sol, _withdrawAndSwap
    BeefyZapBiswapLPVaultOperator.sol, _zapAndStakeLp
    BeefyZapBiswapLPVaultOperator.sol, _withdrawAndSwap

[Yashiru]

Unnecessary array boundaries check when loading an array element twice (Confirmed)

Gas optimization confirmed

[obatirou]

State variables that could be set immutable (disputed)

Code instances out of scope

Unused state variables (disputed)

Code instances out of scope

Unused declared local variables (disputed)

Code instances out of scope

Use bytes32 instead of string to save gas whenever possible (disputed)

Code instances out of scope

Unnecessary cast (disputed)

Code instances out of scope

[Yashiru]

Empty else statement can be removed to save gas (Disputed)

There is no empty else block in StakingLPVaultHelpers._addLiquidityAndDepositETH

Empty else if statement can be removed to save gas (Disputed)

There is no empty else if block in StakingLPVaultHelpers._addLiquidityAndDepositETH

Inline one time use functions (Disputed)

Writing these functions inline would considerably reduce the readability of the code.

Indeed we would save deployment gas consumption if we do so, but we prefer to keep a good readability at the cost of more expensive smart contract deployment.

[Yashiru]

Consider inline the following functions to save gas (Disputed)

NestedAsset._baseURI() function is never used and only return a public variable. We must delete it and use the public variable baseUri instead.

[maximebrugel]

Use unchecked to save gas for certain additive calculations that cannot overflow (Disputed)

Can overflow, since the delay can be 2**256 – 1

[obatirou]

Short the following require messages (duplicated)

https://github.com/code-423n4/2022-06-nested-findings/issues/62#issuecomment-1165547704

[maximebrugel]

Rearrange state variables (Disputed)

The address _ADMIN_SLOT is a constant, store in the bytecode. So the bool and address variables are already packed.

[Yashiru]

Caching array length can save gas (Duplicated)

Duplicated of #2 at For loop optimizaion

Prefix increments are cheaper than postfix increments (Duplicated)

Duplicated of #2 at For loop optimizaion

Unnecessary index init (Duplicated)

Duplicated of #2 at For loop optimizaion