Gas Optimizations

[code423n4]

Title: State variables that could be set immutable
Severity: GAS

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

    operator in TestableOperatorCaller.sol
    resolver in MixinOperatorResolver.sol
    operatorStorage in ZeroExOperator.sol

Title: Unused state variables
Severity: GAS

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.

    TestableMixingOperatorResolver.sol, addressesToCache

Title: Unused declared local variables
Severity: GAS

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.

    TestableOperatorCaller.sol, performSwap, data

Title: Unnecessary array boundaries check when loading an array element twice
Severity: GAS

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: 

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

Title: Caching array length can save gas
Severity: 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++) { ... }


    FeeSplitter.sol, _tokens, 148
    MixinOperatorResolver.sol, requiredOperators, 55
    NestedFactory.sol, operatorsCache, 103
    NestedFactory.sol, orders._batchedOrders, 369
    FeeSplitter.sol, shareholders, 261
    FeeSplitter.sol, shareholdersCache, 280
    FeeSplitter.sol, shareholders, 318
    OperatorResolver.sol, names, 60
    FeeSplitter.sol, _tokens, 165
    OperatorResolver.sol, destinations, 75
    MixinOperatorResolver.sol, requiredOperators, 36
    NestedFactory.sol, _batchedOrders, 581

Title: Prefix increments are cheaper than postfix increments
Severity: GAS

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:

    change to prefix increment and unchecked: NestedFactory.sol, i, 153
    change to prefix increment and unchecked: MixinOperatorResolver.sol, i, 55
    change to prefix increment and unchecked: NestedFactory.sol, i, 103
    change to prefix increment and unchecked: NestedFactory.sol, i, 327
    change to prefix increment and unchecked: NestedRecords.sol, i, 196
    change to prefix increment and unchecked: FeeSplitter.sol, i, 148
    change to prefix increment and unchecked: FeeSplitter.sol, i, 318
    change to prefix increment and unchecked: OperatorResolver.sol, i, 40
    change to prefix increment and unchecked: OperatorResolver.sol, i, 60
    change to prefix increment and unchecked: FeeSplitter.sol, i, 280
    change to prefix increment and unchecked: FeeSplitter.sol, i, 165
    change to prefix increment and unchecked: FeeSplitter.sol, i, 261
    change to prefix increment and unchecked: MixinOperatorResolver.sol, i, 36
    change to prefix increment and unchecked: NestedFactory.sol, i, 273
    change to prefix increment and unchecked: NestedFactory.sol, i, 213
    change to prefix increment and unchecked: NestedFactory.sol, i, 369
    change to prefix increment and unchecked: NestedFactory.sol, i, 581
    change to prefix increment and unchecked: OperatorResolver.sol, i, 75
    change to prefix increment and unchecked: FeeSplitter.sol, i, 126
    change to prefix increment and unchecked: NestedFactory.sol, i, 113
    change to prefix increment and unchecked: NestedFactory.sol, i, 291

Title: Unnecessary index init
Severity: GAS

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:

    MixinOperatorResolver.sol, 36
    NestedFactory.sol, 153
    OperatorResolver.sol, 75
    NestedFactory.sol, 273
    OperatorResolver.sol, 60
    NestedFactory.sol, 213
    FeeSplitter.sol, 318
    MixinOperatorResolver.sol, 55
    FeeSplitter.sol, 261
    NestedFactory.sol, 291
    NestedFactory.sol, 113
    OperatorResolver.sol, 40
    NestedFactory.sol, 369
    NestedFactory.sol, 581
    FeeSplitter.sol, 126
    FeeSplitter.sol, 280
    NestedFactory.sol, 103
    FeeSplitter.sol, 165
    NestedFactory.sol, 327
    FeeSplitter.sol, 148
    NestedRecords.sol, 196

Title: Internal functions to private
Severity: GAS

The following functions could be set private to save gas and improve code quality:

    MixinOperatorResolver.sol, callOperator
    NestedAsset.sol, _setTokenURI
    ExchangeHelpers.sol, setMaxAllowance
    ExchangeHelpers.sol, fillQuote
    MixinOperatorResolver.sol, requireAndGetAddress

Title: Public functions to external
Severity: GAS

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

    DeflationaryMockERC20.sol, transferFrom
    TestableMixingOperatorResolver.sol, resolverOperatorsRequired
    NestedRecords.sol, tokenHoldings
    NestedAsset.sol, originalOwner
    NestedRecords.sol, getAssetTokensLength
    NestedRecords.sol, freeHolding
    NestedAsset.sol, tokenURI
    OwnableProxyDelegation.sol, renounceOwnership
    OwnableProxyDelegation.sol, owner
    OwnableProxyDelegation.sol, transferOwnership

Title: Unnecessary payable
Severity: GAS

The following functions are payable but msg.value isn't used - therefore the function payable state modifier isn't necessary.
Payable functions are more gas expensive than others, and it's danger the users if they send ETH by mistake.

    ZeroExOperator.sol, performSwap is payable but doesn't use msg.value
    FlatOperator.sol, transfer is payable but doesn't use msg.value

Title: Rearrange state variables
Severity: GAS

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

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

Title: Short the following require messages
Severity: GAS

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:

    Solidity file: OwnableProxyDelegation.sol, In line 56, Require message length to shorten: 38, The message: Ownable: new owner is the zero address

Title: Unused imports
Severity: GAS

In the following files there are contract imports that aren't used
Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    IFlatOperator.sol, line 3, import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
    FlatOperator.sol, line 3, import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
    INestedFactory.sol, line 4, import "../NestedReserve.sol";

Title: Unused inheritance
Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    NestedAsset.sol; the inherited contracts OwnableFactoryHandler not used
    NestedReserve.sol; the inherited contracts OwnableFactoryHandler not used
    NestedRecords.sol; the inherited contracts OwnableFactoryHandler not used

Title: Use != 0 instead of > 0
Severity: GAS

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

    NestedFactory.sol, 489: change 'balance > 0' to 'balance != 0'

Title: Unnecessary constructor
Severity: GAS

The following constructors are empty.
(A similar issue code-423n4/2021-11-fei-findings#12)

    TestableMixingOperatorResolver.sol.constructor
    NestedAsset.sol.constructor

Title: Unnecessary functions
Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    MixinOperatorResolver.sol, callOperator
    ExchangeHelpers.sol, fillQuote

Title: Unnecessary cast
Severity: Gas

    IERC20 NestedFactory.sol._transferInputTokens - unnecessary casting IERC20(_inputToken)

Title: Use calldata instead of memory
Severity: GAS

Use calldata instead of memory for function parameters
In some cases, having function arguments in calldata instead of
memory is more optimal.

    NestedAsset.backfillTokenURI (_metadataURI)
    ExchangeHelpers.fillQuote (_swapCallData)
    NestedAsset._setTokenURI (_metadataURI)
    DeflationaryMockERC20.constructor (_name)
    NestedAsset.mintWithMetadata (_metadataURI)
    MockERC20.constructor (_symbol)
    DeflationaryMockERC20.constructor (_symbol)
    MockERC20.constructor (_name)

Title: Consider inline the following functions to save gas
Severity: 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.)


    FeeSplitter.sol, _computeShareCount, { return (_amount * _weight) / _totalWeights; }

Title: Inline one time use functions
Severity: GAS

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

    FeeSplitter.sol, _addShareholder
    MixinOperatorResolver.sol, requireAndGetAddress
    ExchangeHelpers.sol, setMaxAllowance

Title: Check if amount is not zero to save gas
Severity: GAS

The following functions could skip other steps if the amount is 0. (A similar issue: code-423n4/2021-10-badgerdao-findings#82)

    DeflationaryMockERC20.sol, transferFrom
    FeeSplitter.sol, sendFees

[adrien-supizet]

Title: State variables that could be set immutable

Already done

Title: Unused state variables

invalid, mock file out of scope as stated in the readme

Title: Unused declared local variables

invalid, mock file out of scope as stated in the readme

Title: Unnecessary array boundaries check when loading an array element twice

Before:

After:

Title: Caching array length can save gas

duplicate from the last report mentioned in the readme, it's already done where it was useful.

Title: Prefix increments are cheaper than postfix increments

duplicate from the last report mentioned in the readme and in #3 and we don't want to do this

Title: Unnecessary index init

invalid, this makes no difference in loops where the variables must be inited to 0 during the first iteration

Title: Internal functions to private

This wouldn't work.

Title: Public functions to external

Confirmed

Title: Unnecessary payable

This is untrue.

Title: Rearrange state variables

I see no difference whatsoever.

Title: Short the following require messages

Confirmed

Title: Unused imports

Invalid, this does not affect interfaces

Title: Unused inheritance

invalid, they are

Title: Use != 0 instead of > 0

The code mentioned does not exist

Title: Unnecessary constructor

invalid, It is necessary.

Title: Unnecessary functions

invalid, They are necessary.

Title: Unnecessary cast

confirmed, reserve.withdraw(IERC20(_inputToken), _inputTokenAmount);

Title: Use calldata instead of memory

confirmed for non-mock files

Title: Consider inline the following functions to save gas

confirme

Title: Inline one time use functions

True. Acknowledge or confirm?

Title: Check if amount is not zero to save gas

True but this adds an extra check for 99.999% of cases, so we don't want to do this

[harleythedogC4]

My personal judgments:

1. "State variables that could be set immutable". Agree with sponsor, e.g. operatorStorage is already set as immutable. Invalid.
2. "Unused state variables". Agree with sponsor. Invalid.
3. "Unused declared local variables". Agree with sponsor. Invalid.
4. "Unnecessary array boundaries check when loading an array element twice". Valid and small-optimization.
5. "Caching array length can save gas". Agree with sponsor. Invalid.
6. "Prefix increments are cheaper than postfix increments". Agree with sponsor. Invalid.
7. "Unnecessary index init". Agree with sponsor, and this was already raised in previous reports in the previous contest. Invalid.
8. "Internal functions to private". Invalid.
9. "Public functions to external". Valid and small-optimization.
10. "Unnecessary payable". Just because msg.value is not read does not mean payable can be removed. Invalid.
11. "Rearrange state variables". Disagree with sponsor. Valid and small-optimization.
12. "Short the following require messages". Valid and small-optimization.
13. "Unused imports". Agree with sponsor. Invalid.
14. "Unused inheritance". Invalid.
15. "Use != 0 instead of > 0". Agree with sponsor, the referenced code doesn't even exist. Invalid.
16. "Unnecessary constructor". Agree with sponsor, one is a test file and the other uses the inherited constructor. Invalid.
17. "Unnecessary functions". Of course these are necessary, e.g. callOperator... Invalid.
18. "Unnecessary cast". Valid and small-optimization.
19. "Use calldata instead of memory". Most examples given are test files, but still some valid for actual files. Valid and small-optimization.
20. "Consider inline the following functions to save gas". Valid and small-optimization.
21. "Inline one time use functions". Technically true. Valid and small-optimization.
22. "Check if amount is not zero to save gas". Agree with sponsor that this increases gas consumption in most cases. Invalid.

[harleythedogC4]

Now, here is the methodology I used for calculating a score for each gas report. I first assigned each submission to be either small-optimization (1 point), medium-optimization (5 points) or large-optimization (10 points), depending on how useful the optimization is. The score of a gas report is the sum of these points, divided by the maximum number of points achieved by a gas report. This maximum number was 10 points, achieved by #67.

The number of points achieved by this report is 8 points.
Thus the final score of this gas report is (8/10)*100 = 80.