Nested Finance contest - robee's results

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