code423n4 commented 2 years ago

Note: The README lists a bunch of QA/Gas reports from prior findings, but doesn't list the specific issues found therein. Having each warden look at each report and de-dupe is just like adding more files to the scope, and I think is a bit unfair. For the ones that the sponsor has explicitly listed I've still included them because the sponsor may end up finding them useful, especially after seeing the gas amounts involved, but have the lines with strike-through, so the sponsor and judge can ignore them if they wish

Summary

Gas Optimizations

	Issue	Instances
1	Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas	3
2	Using `storage` instead of `memory` for structs/arrays saves gas	1
3	Multiple accesses of a mapping/array should use a local variable cache	4
4	Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()` or `if`-statement	2
5	~~`<array>.length` should not be looked up in every loop of a `for`-loop~~	10
6	`++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	28
7	`require()`/`revert()` strings longer than 32 bytes cost extra gas	12
8	Optimize names to save gas	2
9	Using `bool`s for storage incurs overhead	1
10	It costs more gas to initialize non-`constant`/non-`immutable` variables to zero than to let the default of zero be applied	18
11	~~`++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)~~	24
12	Splitting `require()` statements that use `&&` saves gas	7
13	Usage of `uints`/`ints` smaller than 32 bytes (256 bits) incurs overhead	6
14	Using `private` rather than `public` for constants, saves gas	4
15	Duplicated `require()`/`revert()` checks should be refactored to a modifier or function	14
16	Division by two should use bit shifting	2
17	`require()` or `revert()` statements that check input arguments should be at the top of the function	1
18	Empty blocks should be removed or emit something	2
19	~~Use custom errors rather than `revert()`/`require()` strings to save gas~~	~~132~~
20	Functions guaranteed to revert when called by normal users can be marked `payable`	27
21	Don't use `_msgSender()` if not supporting EIP-2771	13

~~Total: 313 instances over 21 issues~~ Total: 147 instances over 18 issues

Gas Optimizations

1. Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

There are 3 instances of this issue:

File: contracts/governance/scripts/OperatorScripts.sol   #1

28:       function addOperator(IOperatorResolver.Operator memory operator, bytes32 name) external {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L28

File: contracts/governance/scripts/OperatorScripts.sol   #2

52:       function deployAddOperators(bytes memory bytecode, tupleOperator[] memory operators) external {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L52

File: contracts/governance/scripts/OperatorScripts.sol   #3

52:       function deployAddOperators(bytes memory bytecode, tupleOperator[] memory operators) external {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L52

2. Using `storage` instead of `memory` for structs/arrays 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 is 1 instance of this issue:

File: contracts/NestedFactory.sol   #1

123:          bytes32[] memory operatorsCache = operators;

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L123

3. Multiple accesses of a mapping/array should use a local variable cache

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~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. Caching an array's struct avoids recalculating the array offsets into memory

There are 4 instances of this issue:

File: contracts/operators/Yearn/YearnVaultStorage.sol   #1

/// @audit vaults[vault] on line 33
34:           require(vaults[vault].lpToken == address(0), "YVS: VAULT_ALREADY_HAS_LP");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L34

File: contracts/governance/scripts/OperatorScripts.sol   #2

/// @audit operators[i] on line 68
69:               operatorsToImport[i] = IOperatorResolver.Operator(deployedAddress, operators[i].selector);

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L69

File: contracts/OperatorResolver.sol   #3

/// @audit operators[<etc>] on line 42
43:                   operators[names[i]].selector != destinations[i].selector

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L43

File: contracts/OperatorResolver.sol   #4

/// @audit destinations[i] on line 42
43:                   operators[names[i]].selector != destinations[i].selector

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L43

4. Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()` or `if`-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

There are 2 instances of this issue:

File: contracts/NestedFactory.sol   #1

/// @audit require() on line 428
431:                  uint256 underSpentAmount = _inputTokenAmount - amountSpent;

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L431

File: contracts/NestedFactory.sol   #2

/// @audit require() on line 495
497:                  uint256 underSpentAmount = _amountToSpend - amounts[1];

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L497

5. `<array>.length` should not be looked up in every loop of a `for`-loop

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas)
memory arrays use MLOAD (3 gas)
calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

~~There are 10 instances of this issue:~~

File: contracts/governance/TimelockControllerEmergency.sol

84:           for (uint256 i = 0; i < proposers.length; ++i) {

89:           for (uint256 i = 0; i < executors.length; ++i) {

234:          for (uint256 i = 0; i < targets.length; ++i) {

324:          for (uint256 i = 0; i < targets.length; ++i) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L84

File: contracts/abstracts/MixinOperatorResolver.sol

37:           for (uint256 i = 0; i < requiredOperators.length; i++) {

56:           for (uint256 i = 0; i < requiredOperators.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L37

File: contracts/OperatorResolver.sol

60:           for (uint256 i = 0; i < names.length; i++) {

75:           for (uint256 i = 0; i < destinations.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L60

File: contracts/NestedFactory.sol

124:          for (uint256 i = 0; i < operatorsCache.length; i++) {

651:          for (uint256 i = 0; i < _batchedOrders.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L124

6. `++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

There are 28 instances of this issue:

File: contracts/operators/Beefy/BeefyVaultOperator.sol

18:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultOperator.sol#L18

File: contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol

27:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L27

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol

27:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L27

File: contracts/operators/Yearn/YearnCurveVaultOperator.sol

42:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L42

File: contracts/governance/TimelockControllerEmergency.sol

84:           for (uint256 i = 0; i < proposers.length; ++i) {

89:           for (uint256 i = 0; i < executors.length; ++i) {

234:          for (uint256 i = 0; i < targets.length; ++i) {

324:          for (uint256 i = 0; i < targets.length; ++i) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L84

File: contracts/governance/scripts/OperatorScripts.sol

67:           for (uint256 i; i < operatorLength; i++) {

80:           for (uint256 i; i < operatorLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L67

File: contracts/abstracts/MixinOperatorResolver.sol

37:           for (uint256 i = 0; i < requiredOperators.length; i++) {

56:           for (uint256 i = 0; i < requiredOperators.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L37

File: contracts/OperatorResolver.sol

40:           for (uint256 i = 0; i < namesLength; i++) {

60:           for (uint256 i = 0; i < names.length; i++) {

75:           for (uint256 i = 0; i < destinations.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L40

File: contracts/NestedFactory.sol

124:          for (uint256 i = 0; i < operatorsCache.length; i++) {

136:          for (uint256 i = 0; i < operatorsLength; i++) {

196:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

256:          for (uint256 i = 0; i < tokensLength; i++) {

315:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

333:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

369:          for (uint256 i = 0; i < batchLength; i++) {

412:          for (uint256 i = 0; i < batchLength; i++) {

651:          for (uint256 i = 0; i < _batchedOrders.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L124

File: contracts/libraries/CurveHelpers/CurveHelpers.sol

22:           for (uint256 i; i < 2; i++) {

42:           for (uint256 i; i < 3; i++) {

62:           for (uint256 i; i < 4; i++) {

86:           for (uint256 i; i < poolCoinAmount; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L22

7. `require()`/`revert()` strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

There are 12 instances of this issue:

File: contracts/governance/TimelockControllerEmergency.sol

229:          require(targets.length == values.length, "TimelockController: length mismatch");

230:          require(targets.length == datas.length, "TimelockController: length mismatch");

243:          require(!isOperation(id), "TimelockController: operation already scheduled");

244:          require(delay >= getMinDelay(), "TimelockController: insufficient delay");

256:          require(isOperationPending(id), "TimelockController: operation cannot be cancelled");

319:          require(targets.length == values.length, "TimelockController: length mismatch");

320:          require(targets.length == datas.length, "TimelockController: length mismatch");

334:          require(isOperationReady(id), "TimelockController: operation is not ready");

335:          require(predecessor == bytes32(0) || isOperationDone(predecessor), "TimelockController: missing dependency");

342:          require(isOperationReady(id), "TimelockController: operation is not ready");

359:          require(success, "TimelockController: underlying transaction reverted");

375:          require(msg.sender == address(this), "TimelockController: caller must be timelock");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L229

8. Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There are 2 instances of this issue:

File: contracts/abstracts/OwnableProxyDelegation.sol   #1

/// @audit initialize()
10:   abstract contract OwnableProxyDelegation is Context {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L10

File: contracts/abstracts/MixinOperatorResolver.sol   #2

/// @audit resolverOperatorsRequired(), rebuildCache(), isResolverCached()
10:   abstract contract MixinOperatorResolver {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L10

9. Using `bool`s for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

There is 1 instance of this issue:

File: contracts/abstracts/OwnableProxyDelegation.sol   #1

18:       bool public initialized;

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L18

10. It costs more gas to initialize non-`constant`/non-`immutable` variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 18 instances of this issue:

File: contracts/governance/TimelockControllerEmergency.sol

84:           for (uint256 i = 0; i < proposers.length; ++i) {

89:           for (uint256 i = 0; i < executors.length; ++i) {

234:          for (uint256 i = 0; i < targets.length; ++i) {

324:          for (uint256 i = 0; i < targets.length; ++i) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L84

File: contracts/abstracts/MixinOperatorResolver.sol

37:           for (uint256 i = 0; i < requiredOperators.length; i++) {

56:           for (uint256 i = 0; i < requiredOperators.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L37

File: contracts/OperatorResolver.sol

40:           for (uint256 i = 0; i < namesLength; i++) {

60:           for (uint256 i = 0; i < names.length; i++) {

75:           for (uint256 i = 0; i < destinations.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L40

File: contracts/NestedFactory.sol

124:          for (uint256 i = 0; i < operatorsCache.length; i++) {

136:          for (uint256 i = 0; i < operatorsLength; i++) {

196:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

256:          for (uint256 i = 0; i < tokensLength; i++) {

315:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

333:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

369:          for (uint256 i = 0; i < batchLength; i++) {

412:          for (uint256 i = 0; i < batchLength; i++) {

651:          for (uint256 i = 0; i < _batchedOrders.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L124

11. `++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)

Saves 6 gas per loop

~~There are 24 instances of this issue:~~

File: contracts/operators/Beefy/BeefyVaultOperator.sol

18:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultOperator.sol#L18

File: contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol

27:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L27

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol

27:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L27

File: contracts/operators/Yearn/YearnCurveVaultOperator.sol

42:           for (uint256 i; i < vaultsLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L42

File: contracts/governance/scripts/OperatorScripts.sol

67:           for (uint256 i; i < operatorLength; i++) {

80:           for (uint256 i; i < operatorLength; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L67

File: contracts/abstracts/MixinOperatorResolver.sol

37:           for (uint256 i = 0; i < requiredOperators.length; i++) {

56:           for (uint256 i = 0; i < requiredOperators.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L37

File: contracts/OperatorResolver.sol

40:           for (uint256 i = 0; i < namesLength; i++) {

60:           for (uint256 i = 0; i < names.length; i++) {

75:           for (uint256 i = 0; i < destinations.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L40

File: contracts/NestedFactory.sol

124:          for (uint256 i = 0; i < operatorsCache.length; i++) {

136:          for (uint256 i = 0; i < operatorsLength; i++) {

196:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

256:          for (uint256 i = 0; i < tokensLength; i++) {

315:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

333:          for (uint256 i = 0; i < batchedOrdersLength; i++) {

369:          for (uint256 i = 0; i < batchLength; i++) {

412:          for (uint256 i = 0; i < batchLength; i++) {

651:          for (uint256 i = 0; i < _batchedOrders.length; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L124

File: contracts/libraries/CurveHelpers/CurveHelpers.sol

22:           for (uint256 i; i < 2; i++) {

42:           for (uint256 i; i < 3; i++) {

62:           for (uint256 i; i < 4; i++) {

86:           for (uint256 i; i < poolCoinAmount; i++) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L22

12. Splitting `require()` statements that use `&&` saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

There are 7 instances of this issue:

File: contracts/operators/Beefy/BeefyVaultOperator.sol

54:           require(vaultAmount != 0 && vaultAmount >= minVaultAmount, "BVO: INVALID_AMOUNT_RECEIVED");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultOperator.sol#L54

File: contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol

64:           require(vaultAmount != 0 && vaultAmount >= minVaultAmount, "BLVO: INVALID_AMOUNT_RECEIVED");

65:           require(depositedAmount != 0 && amountToDeposit >= depositedAmount, "BLVO: INVALID_AMOUNT_DEPOSITED");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L64

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol

64:           require(vaultAmount != 0 && vaultAmount >= minVaultAmount, "BLVO: INVALID_AMOUNT_RECEIVED");

65:           require(depositedAmount != 0 && amountToDeposit >= depositedAmount, "BLVO: INVALID_AMOUNT_DEPOSITED");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L64

File: contracts/operators/Paraswap/ParaswapOperator.sol

16:           require(_tokenTransferProxy != address(0) && _augustusSwapper != address(0), "PSO: INVALID_ADDRESS");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Paraswap/ParaswapOperator.sol#L16

File: contracts/NestedFactory.sol

66            require(
67                address(_nestedAsset) != address(0) &&
68                    address(_nestedRecords) != address(0) &&
69                    address(_reserve) != address(0) &&
70                    address(_feeSplitter) != address(0) &&
71                    address(_weth) != address(0) &&
72                    _operatorResolver != address(0) &&
73                    address(_withdrawer) != address(0),
74                "NF: INVALID_ADDRESS"
75:           );

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L66-L75

13. 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 Use a larger size then downcast where needed

There are 6 instances of this issue:

File: contracts/operators/Yearn/YearnVaultStorage.sol

8:        uint96 poolCoinAmount;

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L8

File: contracts/operators/Yearn/YearnCurveVaultOperator.sol

72:           (address pool, uint96 poolCoinAmount, address lpToken) = operatorStorage.vaults(vault);

122:          (address pool, uint96 poolCoinAmount, address lpToken) = operatorStorage.vaults(vault);

166:          (address pool, uint96 poolCoinAmount, address lpToken) = operatorStorage.vaults(vault);

214:          (address pool, uint96 poolCoinAmount, address lpToken) = operatorStorage.vaults(vault);

262:          (address pool, uint96 poolCoinAmount, address lpToken) = operatorStorage.vaults(vault);

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L72

14. Using `private` rather than `public` for constants, saves gas

If needed, the value can be read from the verified contract source code. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 4 instances of this issue:

File: contracts/governance/TimelockControllerEmergency.sol   #1

25:       bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L25

File: contracts/governance/TimelockControllerEmergency.sol   #2

26:       bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L26

File: contracts/governance/TimelockControllerEmergency.sol   #3

27:       bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L27

File: contracts/governance/TimelockControllerEmergency.sol   #4

28:       bytes32 public constant EMERGENCY_ROLE = keccak256("EMERGENCY_ROLE");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L28

15. Duplicated `require()`/`revert()` checks should be refactored to a modifier or function

Saves deployment costs

There are 14 instances of this issue:

File: contracts/operators/Beefy/BeefyVaultOperator.sol

83:           require(amount != 0, "BVO: INVALID_AMOUNT");

85:           require(address(token) != address(0), "BVO: INVALID_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultOperator.sol#L83

File: contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol

99:           require(router != address(0), "BLVO: INVALID_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L99

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol

99:           require(router != address(0), "BLVO: INVALID_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L99

File: contracts/operators/Yearn/YearnCurveVaultOperator.sol

121:          require(amount != 0, "YCVO: INVALID_AMOUNT");

123:          require(pool != address(0), "YCVO: INVALID_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L121

File: contracts/governance/TimelockControllerEmergency.sol

319:          require(targets.length == values.length, "TimelockController: length mismatch");

320:          require(targets.length == datas.length, "TimelockController: length mismatch");

342:          require(isOperationReady(id), "TimelockController: operation is not ready");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L319

File: contracts/NestedFactory.sol

312:          require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");

289:          require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

406:          require(batchLength != 0, "NF: INVALID_ORDERS");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L312

File: contracts/libraries/StakingLPVaultHelpers.sol

138:          require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L138

File: contracts/libraries/CurveHelpers/CurveHelpers.sol

48:           revert("CH: INVALID_INPUT_TOKEN");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L48

16. Division by two should use bit shifting

<x> / 2 is the same as <x> >> 1. The DIV opcode costs 5 gas, whereas SHR only costs 3 gas

There are 2 instances of this issue:

File: contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol   #1

275:          uint256 halfInvestment = investmentA / 2;

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L275

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol   #2

273:          uint256 halfInvestment = investmentA / 2;

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L273

17. `require()` or `revert()` statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables

There is 1 instance of this issue:

File: contracts/governance/TimelockControllerEmergency.sol   #1

244:          require(delay >= getMinDelay(), "TimelockController: insufficient delay");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L244

18. 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{...}})

There are 2 instances of this issue:

File: contracts/governance/TimelockControllerEmergency.sol   #1

113:      receive() external payable {}

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L113

File: contracts/abstracts/MixinOperatorResolver.sol   #2

29:       function resolverOperatorsRequired() public view virtual returns (bytes32[] memory) {}

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L29

19. Use custom errors rather than `revert()`/`require()` strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

~~There are 132 instances of this issue:~~

File: contracts/operators/Beefy/BeefyVaultOperator.sol

15:           require(vaultsLength == tokens.length, "BVO: INVALID_VAULTS_LENGTH");

41:           require(amount != 0, "BVO: INVALID_AMOUNT");

43:           require(address(token) != address(0), "BVO: INVALID_VAULT");

50:           require(success, "BVO: DEPOSIT_CALL_FAILED");

54:           require(vaultAmount != 0 && vaultAmount >= minVaultAmount, "BVO: INVALID_AMOUNT_RECEIVED");

55:           require(amount == tokenAmount, "BVO: INVALID_AMOUNT_DEPOSITED");

83:           require(amount != 0, "BVO: INVALID_AMOUNT");

85:           require(address(token) != address(0), "BVO: INVALID_VAULT");

91:           require(success, "BVO: WITHDRAW_CALL_FAILED");

95:           require(vaultAmount == amount, "BVO: INVALID_AMOUNT_WITHDRAWED");

96:           require(tokenAmount != 0, "BVO: INVALID_AMOUNT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultOperator.sol#L15

File: contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol

23:           require(vaultsLength == routers.length, "BLVO: INVALID_VAULTS_LENGTH");

52:           require(amountToDeposit != 0, "BLVO: INVALID_AMOUNT");

54:           require(router != address(0), "BLVO: INVALID_VAULT");

64:           require(vaultAmount != 0 && vaultAmount >= minVaultAmount, "BLVO: INVALID_AMOUNT_RECEIVED");

65:           require(depositedAmount != 0 && amountToDeposit >= depositedAmount, "BLVO: INVALID_AMOUNT_DEPOSITED");

97:           require(amount != 0, "BLVO: INVALID_AMOUNT");

99:           require(router != address(0), "BLVO: INVALID_VAULT");

108:          require(vaultAmount == amount, "BLVO: INVALID_AMOUNT_WITHDRAWED");

109:          require(tokenAmount >= minTokenAmount, "BLVO: INVALID_OUTPUT_AMOUNT");

142:          require(token0 == token || token1 == token, "BLVO: INVALID_TOKEN");

187:          require(pair.factory() == biswapRouter.factory(), "BLVO: INVALID_VAULT");

198:          require(isInput0 || cachedToken1 == token, "BLVO: INVALID_INPUT_TOKEN");

271:          require(reserveA > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");

272:          require(reserveB > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L23

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol

23:           require(vaultsLength == routers.length, "BLVO: INVALID_VAULTS_LENGTH");

52:           require(amountToDeposit != 0, "BLVO: INVALID_AMOUNT");

54:           require(router != address(0), "BLVO: INVALID_VAULT");

64:           require(vaultAmount != 0 && vaultAmount >= minVaultAmount, "BLVO: INVALID_AMOUNT_RECEIVED");

65:           require(depositedAmount != 0 && amountToDeposit >= depositedAmount, "BLVO: INVALID_AMOUNT_DEPOSITED");

97:           require(amount != 0, "BLVO: INVALID_AMOUNT");

99:           require(router != address(0), "BLVO: INVALID_VAULT");

108:          require(vaultAmount == amount, "BLVO: INVALID_AMOUNT_WITHDRAWED");

109:          require(tokenAmount >= minTokenAmount, "BLVO: INVALID_OUTPUT_AMOUNT");

142:          require(token0 == token || token1 == token, "BLVO: INVALID_TOKEN");

187:          require(pair.factory() == uniswapRouter.factory(), "BLVO: INVALID_VAULT");

198:          require(isInput0 || cachedToken1 == token, "BLVO: INVALID_INPUT_TOKEN");

269:          require(reserveA > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");

270:          require(reserveB > 1000, "BLVO: PAIR_RESERVE_TOO_LOW");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L23

File: contracts/operators/Beefy/BeefyVaultStorage.sol

25:           require(vault != address(0), "BVS: INVALID_VAULT_ADDRESS");

26:           require(tokenOrZapper != address(0), "BVS: INVALID_UNDERLYING_ADDRESS");

27:           require(vaults[vault] == address(0), "BVS: ALREADY_EXISTENT_VAULT");

35:           require(vaults[vault] != address(0), "BVS: NON_EXISTENT_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultStorage.sol#L25

File: contracts/operators/Yearn/YearnVaultStorage.sol

30:           require(vault != address(0), "YVS: INVALID_VAULT_ADDRESS");

31:           require(curvePool.poolAddress != address(0), "YVS: INVALID_POOL_ADDRESS");

32:           require(curvePool.lpToken != address(0), "YVS: INVALID_TOKEN_ADDRESS");

33:           require(vaults[vault].poolAddress == address(0), "YVS: VAULT_ALREADY_HAS_POOL");

34:           require(vaults[vault].lpToken == address(0), "YVS: VAULT_ALREADY_HAS_LP");

42:           require(vaults[vault].poolAddress != address(0), "YVS: NON_EXISTENT_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L30

File: contracts/operators/Yearn/YearnCurveVaultOperator.sol

39:           require(vaultsLength == pools.length, "YCVO: INVALID_VAULTS_LENGTH");

70:           require(amount != 0, "YCVO: INVALID_AMOUNT");

73:           require(pool != address(0), "YCVO: INVALID_VAULT");

121:          require(amount != 0, "YCVO: INVALID_AMOUNT");

123:          require(pool != address(0), "YCVO: INVALID_VAULT");

164:          require(amount != 0, "YCVO: INVALID_AMOUNT");

167:          require(pool != address(0), "YCVO: INVALID_VAULT");

212:          require(amount != 0, "YCVO: INVALID_AMOUNT");

215:          require(pool != address(0), "YCVO: INVALID_VAULT");

260:          require(amount != 0, "YCVO: INVALID_AMOUNT");

263:          require(pool != address(0), "YCVO: INVALID_VAULT");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L39

File: contracts/operators/Paraswap/ParaswapOperator.sol

16:           require(_tokenTransferProxy != address(0) && _augustusSwapper != address(0), "PSO: INVALID_ADDRESS");

27:           require(sellToken != buyToken, "PSO: SAME_INPUT_OUTPUT");

35:           require(success, "PSO: SWAP_FAILED");

39:           require(amountBought != 0, "PSO: INVALID_AMOUNT_BOUGHT");

40:           require(amountSold != 0, "PSO: INVALID_AMOUNT_SOLD");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Paraswap/ParaswapOperator.sol#L16

File: contracts/governance/OwnerProxy.sol

17:           require(_target != address(0), "OP: INVALID_TARGET");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/OwnerProxy.sol#L17

File: contracts/governance/TimelockControllerEmergency.sol

229:          require(targets.length == values.length, "TimelockController: length mismatch");

230:          require(targets.length == datas.length, "TimelockController: length mismatch");

243:          require(!isOperation(id), "TimelockController: operation already scheduled");

244:          require(delay >= getMinDelay(), "TimelockController: insufficient delay");

256:          require(isOperationPending(id), "TimelockController: operation cannot be cancelled");

319:          require(targets.length == values.length, "TimelockController: length mismatch");

320:          require(targets.length == datas.length, "TimelockController: length mismatch");

334:          require(isOperationReady(id), "TimelockController: operation is not ready");

335:          require(predecessor == bytes32(0) || isOperationDone(predecessor), "TimelockController: missing dependency");

342:          require(isOperationReady(id), "TimelockController: operation is not ready");

359:          require(success, "TimelockController: underlying transaction reverted");

375:          require(msg.sender == address(this), "TimelockController: caller must be timelock");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L229

File: contracts/governance/scripts/OperatorScripts.sol

19:           require(_nestedFactory != address(0), "AO-SCRIPT: INVALID_FACTORY_ADDR");

20:           require(_resolver != address(0), "AO-SCRIPT: INVALID_RESOLVER_ADDR");

29:           require(operator.implementation != address(0), "AO-SCRIPT: INVALID_IMPL_ADDRESS");

54:           require(operatorLength != 0, "DAO-SCRIPT: INVALID_OPERATOR_LEN");

55:           require(bytecode.length != 0, "DAO-SCRIPT: BYTECODE_ZERO");

61:           require(deployedAddress != address(0), "DAO-SCRIPT: FAILED_DEPLOY");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L19

File: contracts/abstracts/OwnableProxyDelegation.sol

25:           require(ownerAddr != address(0), "OPD: INVALID_ADDRESS");

26:           require(!initialized, "OPD: INITIALIZED");

27:           require(StorageSlot.getAddressSlot(_ADMIN_SLOT).value == msg.sender, "OPD: FORBIDDEN");

41:           require(owner() == _msgSender(), "OPD: NOT_OWNER");

57:           require(newOwner != address(0), "OPD: INVALID_ADDRESS");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L25

File: contracts/abstracts/MixinOperatorResolver.sol

23:           require(_resolver != address(0), "MOR: INVALID_ADDRESS");

77:           require(_foundAddress.implementation != address(0), string(abi.encodePacked("MOR: MISSING_OPERATOR: ", name)));

103:              require(tokens[0] == _outputToken, "MOR: INVALID_OUTPUT_TOKEN");

104:              require(tokens[1] == _inputToken, "MOR: INVALID_INPUT_TOKEN");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L23

File: contracts/OperatorResolver.sol

27:           require(_foundOperator.implementation != address(0), reason);

39:           require(namesLength == destinations.length, "OR: INPUTS_LENGTH_MUST_MATCH");

57:           require(names.length == operatorsToImport.length, "OR: INPUTS_LENGTH_MUST_MATCH");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L27

File: contracts/Withdrawer.sol

21:           require(msg.sender == address(weth), "WD: ETH_SENDER_NOT_WETH");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/Withdrawer.sol#L21

File: contracts/NestedFactory.sol

66            require(
67                address(_nestedAsset) != address(0) &&
68                    address(_nestedRecords) != address(0) &&
69                    address(_reserve) != address(0) &&
70                    address(_feeSplitter) != address(0) &&
71                    address(_weth) != address(0) &&
72                    _operatorResolver != address(0) &&
73                    address(_withdrawer) != address(0),
74                "NF: INVALID_ADDRESS"
75:           );

99:           require(nestedAsset.ownerOf(_nftId) == _msgSender(), "NF: CALLER_NOT_OWNER");

107:          require(block.timestamp > nestedRecords.getLockTimestamp(_nftId), "NF: LOCKED_NFT");

122:          require(operator != bytes32(""), "NF: INVALID_OPERATOR_NAME");

125:              require(operatorsCache[i] != operator, "NF: EXISTENT_OPERATOR");

153:          require(address(_feeSplitter) != address(0), "NF: INVALID_FEE_SPLITTER_ADDRESS");

160:          require(_entryFees != 0, "NF: ZERO_FEES");

161:          require(_entryFees <= 10000, "NF: FEES_OVERFLOW");

168:          require(_exitFees != 0, "NF: ZERO_FEES");

169:          require(_exitFees <= 10000, "NF: FEES_OVERFLOW");

191:          require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");

250:          require(_orders.length != 0, "NF: INVALID_ORDERS");

251:          require(tokensLength == _orders.length, "NF: INPUTS_LENGTH_MUST_MATCH");

252:          require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

286:          require(assetTokensLength > _tokenIndex, "NF: INVALID_TOKEN_INDEX");

288:          require(assetTokensLength > 1, "NF: UNALLOWED_EMPTY_PORTFOLIO");

289:          require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

312:          require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");

313:          require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

330:          require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");

331:          require(nestedRecords.getAssetReserve(_nftId) == address(reserve), "NF: RESERVE_MISMATCH");

359:          require(batchLength != 0, "NF: INVALID_ORDERS");

379:          require(amountSpent <= _inputTokenAmount - feesAmount, "NF: OVERSPENT");

406:          require(batchLength != 0, "NF: INVALID_ORDERS");

407:          require(_batchedOrders.amounts.length == batchLength, "NF: INPUTS_LENGTH_MUST_MATCH");

428:              require(amountSpent <= _inputTokenAmount, "NF: OVERSPENT");

469:          require(success, "NF: OPERATOR_CALL_FAILED");

495:              require(amounts[1] <= _amountToSpend, "NF: OVERSPENT");

543:              require(!_fromReserve, "NF: NO_ETH_FROM_RESERVE");

544:              require(address(this).balance >= _inputTokenAmount, "NF: INVALID_AMOUNT_IN");

551               require(
552                   nestedRecords.getAssetHolding(_nftId, address(_inputToken)) >= _inputTokenAmount,
553                   "NF: INSUFFICIENT_AMOUNT_IN"
554:              );

612:              require(success, "NF: ETH_TRANSFER_ERROR");

656:          require(msg.value == ethNeeded, "NF: WRONG_MSG_VALUE");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L66-L75

File: contracts/libraries/StakingLPVaultHelpers.sol

108:          require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");

138:          require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L108

20. Functions guaranteed to revert when called by normal users can be marked `payable`

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 27 instances of this issue:

File: contracts/operators/Beefy/BeefyVaultStorage.sol

24:       function addVault(address vault, address tokenOrZapper) external onlyOwner {

34:       function removeVault(address vault) external onlyOwner {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultStorage.sol#L24

File: contracts/operators/Yearn/YearnVaultStorage.sol

29:       function addVault(address vault, CurvePool calldata curvePool) external onlyOwner {

41:       function removeVault(address vault) external onlyOwner {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L29

File: contracts/governance/OwnerProxy.sol

16:       function execute(address _target, bytes memory _data) public payable onlyOwner returns (bytes memory response) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/OwnerProxy.sol#L16

File: contracts/governance/TimelockControllerEmergency.sol

199       function schedule(
200           address target,
201           uint256 value,
202           bytes calldata data,
203           bytes32 predecessor,
204           bytes32 salt,
205           uint256 delay
206:      ) public virtual onlyRole(PROPOSER_ROLE) {

221       function scheduleBatch(
222           address[] calldata targets,
223           uint256[] calldata values,
224           bytes[] calldata datas,
225           bytes32 predecessor,
226           bytes32 salt,
227           uint256 delay
228:      ) public virtual onlyRole(PROPOSER_ROLE) {

255:      function cancel(bytes32 id) public virtual onlyRole(PROPOSER_ROLE) {

274       function execute(
275           address target,
276           uint256 value,
277           bytes calldata data,
278           bytes32 predecessor,
279           bytes32 salt
280:      ) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) {

295       function executeEmergency(
296           address target,
297           uint256 value,
298           bytes calldata data
299:      ) public payable onlyRole(EMERGENCY_ROLE) {

312       function executeBatch(
313           address[] calldata targets,
314           uint256[] calldata values,
315           bytes[] calldata datas,
316           bytes32 predecessor,
317           bytes32 salt
318:      ) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L199-L206

File: contracts/abstracts/OwnableProxyDelegation.sol

50:       function renounceOwnership() public virtual onlyOwner {

56:       function transferOwnership(address newOwner) public virtual onlyOwner {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L50

File: contracts/OperatorResolver.sol

52        function importOperators(
53            bytes32[] calldata names,
54            Operator[] calldata operatorsToImport,
55            MixinOperatorResolver[] calldata destinations
56:       ) external override onlyOwner {

74:       function rebuildCaches(MixinOperatorResolver[] calldata destinations) public onlyOwner {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/OperatorResolver.sol#L52-L56

File: contracts/NestedFactory.sol

121:      function addOperator(bytes32 operator) external override onlyOwner {

133:      function removeOperator(bytes32 operator) external override onlyOwner {

152:      function setFeeSplitter(FeeSplitter _feeSplitter) external override onlyOwner {

159:      function setEntryFees(uint256 _entryFees) external override onlyOwner {

167:      function setExitFees(uint256 _exitFees) external override onlyOwner {

175:      function unlockTokens(IERC20 _token) external override onlyOwner {

205       function processInputOrders(uint256 _nftId, BatchedInputOrders[] calldata _batchedOrders)
206           external
207           payable
208           override
209           nonReentrant
210           onlyTokenOwner(_nftId)
211:          isUnlocked(_nftId)

219       function processOutputOrders(uint256 _nftId, BatchedOutputOrders[] calldata _batchedOrders)
220           external
221           override
222           nonReentrant
223           onlyTokenOwner(_nftId)
224:          isUnlocked(_nftId)

231       function processInputAndOutputOrders(
232           uint256 _nftId,
233           BatchedInputOrders[] calldata _batchedInputOrders,
234           BatchedOutputOrders[] calldata _batchedOutputOrders
235:      ) external payable override nonReentrant onlyTokenOwner(_nftId) isUnlocked(_nftId) {

243       function destroy(
244           uint256 _nftId,
245           IERC20 _buyToken,
246           Order[] calldata _orders
247:      ) external override nonReentrant onlyTokenOwner(_nftId) isUnlocked(_nftId) {

278       function withdraw(uint256 _nftId, uint256 _tokenIndex)
279           external
280           override
281           nonReentrant
282           onlyTokenOwner(_nftId)
283:          isUnlocked(_nftId)

301:      function updateLockTimestamp(uint256 _nftId, uint256 _timestamp) external override onlyTokenOwner(_nftId) {

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L121

21. Don't use `_msgSender()` if not supporting EIP-2771

Use msg.sender if the code does not implement EIP-2771 trusted forwarder support

There are 13 instances of this issue:

File: contracts/governance/TimelockControllerEmergency.sol

77:           _setupRole(TIMELOCK_ADMIN_ROLE, _msgSender());

105:              _checkRole(role, _msgSender());

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L77

File: contracts/abstracts/OwnableProxyDelegation.sol

41:           require(owner() == _msgSender(), "OPD: NOT_OWNER");

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L41

File: contracts/NestedFactory.sol

99:           require(nestedAsset.ownerOf(_nftId) == _msgSender(), "NF: CALLER_NOT_OWNER");

194:          uint256 nftId = nestedAsset.mint(_msgSender(), _originalTokenId);

269:              _safeTransferAndUnwrap(_buyToken, amountBought, _msgSender());

274:          nestedAsset.burn(_msgSender(), _nftId);

294:          _safeTransferWithFees(IERC20(token), amount, _msgSender(), _nftId);

341:                  _safeTransferAndUnwrap(_batchedOrders[i].outputToken, amountBought - feesAmount, _msgSender());

383:                  SafeERC20.safeTransfer(tokenSold, _fromReserve ? address(reserve) : _msgSender(), underSpentAmount);

499:                      _safeTransferWithFees(IERC20(_inputToken), underSpentAmount, _msgSender(), _nftId);

503:              _safeTransferWithFees(IERC20(_inputToken), _amountToSpend, _msgSender(), _nftId);

558:              SafeERC20.safeTransferFrom(_inputToken, _msgSender(), address(this), _inputTokenAmount);

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L99

Yashiru commented 2 years ago

3. Multiple accesses of a mapping/array should use a local variable cache (Confirmed)

Gas optimization confirmed

obatirou commented 2 years ago

15. Duplicated require()/revert() checks should be refactored to a modifier or function (disputed).

It save deployment cost, but afect the readability

obatirou commented 2 years ago

8. Optimize names to save gas (disputed)

It will reduce the readability of our contracts.

Yashiru commented 2 years ago

9. Using bools for storage incurs overhead

This is only relevant if you want to pack more than one boolean, but none of the specified booleans can be paked.

Yashiru commented 2 years ago

16. Division by two should use bit shifting (Duplicated)

Duplicated of #89 at Use Shift Right/Left instead of Division/Multiplication

obatirou commented 2 years ago

14. Using private rather than public for constants, saves gas (Acknowledge)

We do not want to make the trade of reducing the bytecode to reduce the deployment gas cost and reduce data accessibility for the users.

Yashiru commented 2 years ago

1. Using calldata instead of memory for read-only arguments in external functions saves gas (Confirmed)

Gas optimization confirmed

Missing occurences (found in #82):

maximebrugel commented 2 years ago

4. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement (Disputed)

Already in an unchecked block

maximebrugel commented 2 years ago

19. Use custom errors rather than revert()/require() strings to save gas (Duplicated)

6 (see comment)

obatirou commented 2 years ago

12. Splitting require() statements that use && saves gas (duplicate)

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

Yashiru commented 2 years ago

4. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement (Duplicated)

Duplicated of #2 at For loop optimizaion

5. .length should not be looked up in every loop of a for-loop (Duplicated)

Duplicated of #2 at For loop optimizaion

6. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used (Duplicated)

Duplicated of #2 at For loop optimizaion in for- and while-loops

11. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) (Duplicated)

Duplicated of #2 at For loop optimizaion

maximebrugel commented 2 years ago

18. Empty blocks should be removed or emit something (Disputed)

TimelockControllerEmergency =>

We want to be able to receive ether this way. But if neither a receive Ether nor a payable fallback function is present, the contract cannot receive Ether through regular transactions and throws an exception.

MixinOperatorResolver =>

The function is virtual and the contract abstract

Yashiru commented 2 years ago

13. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead (Disputed)

We are using uint96 to pack an address with a uint in the CurvePool struct.

Here we are retriving a CurvePool from YearnOperatorStorage in variables pool, poolCoinAmount and lpToken.

Gas report testing

Changed the uint96 in deposit() for an uint256 to conserve the struct packing, but reading the packed uint96 as an uint256.

Executed a transaction calling NestedFactory.create() with a batched order calling YearnCurveVaultOperator.deposit().

Results

Before modifications

After modifications

Yashiru commented 2 years ago

21. Don't use _msgSender() if not supporting EIP-2771 (Disputed)

msgSender() should be used for intermediate, library-like contracts. That is why it is used in the NestedFactory.

Already surfaced in previous audits.

Yashiru commented 2 years ago

2. Using storage instead of memory for structs/arrays saves gas (Duplicated)

Duplicated of #58 at Change to storage

code-423n4 / 2022-06-nested-findings

Gas Optimizations #75

Summary

Gas Optimizations

Gas Optimizations

1. Using calldata instead of memory for read-only arguments in external functions saves gas

2. Using storage instead of memory for structs/arrays saves gas

3. Multiple accesses of a mapping/array should use a local variable cache

4. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

5. <array>.length should not be looked up in every loop of a for-loop

6. ++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

7. require()/revert() strings longer than 32 bytes cost extra gas

8. Optimize names to save gas

9. Using bools for storage incurs overhead

10. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

11. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

12. Splitting require() statements that use && saves gas

13. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

14. Using private rather than public for constants, saves gas

15. Duplicated require()/revert() checks should be refactored to a modifier or function

16. Division by two should use bit shifting

17. require() or revert() statements that check input arguments should be at the top of the function

18. Empty blocks should be removed or emit something

19. Use custom errors rather than revert()/require() strings to save gas

20. Functions guaranteed to revert when called by normal users can be marked payable

21. Don't use _msgSender() if not supporting EIP-2771

3. Multiple accesses of a mapping/array should use a local variable cache (Confirmed)

15. Duplicated require()/revert() checks should be refactored to a modifier or function (disputed).

8. Optimize names to save gas (disputed)

9. Using bools for storage incurs overhead

16. Division by two should use bit shifting (Duplicated)

14. Using private rather than public for constants, saves gas (Acknowledge)

1. Using calldata instead of memory for read-only arguments in external functions saves gas (Confirmed)

4. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement (Disputed)

19. Use custom errors rather than revert()/require() strings to save gas (Duplicated)

6 (see comment)

12. Splitting require() statements that use && saves gas (duplicate)

4. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement (Duplicated)

5. .length should not be looked up in every loop of a for-loop (Duplicated)

6. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used (Duplicated)

11. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) (Duplicated)

18. Empty blocks should be removed or emit something (Disputed)

TimelockControllerEmergency =>

MixinOperatorResolver =>

13. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead (Disputed)

Gas report testing

Results

21. Don't use _msgSender() if not supporting EIP-2771 (Disputed)

2. Using storage instead of memory for structs/arrays saves gas (Duplicated)

1. Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas

2. Using `storage` instead of `memory` for structs/arrays saves gas

4. Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()` or `if`-statement

5. `<array>.length` should not be looked up in every loop of a `for`-loop

6. `++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

7. `require()`/`revert()` strings longer than 32 bytes cost extra gas

9. Using `bool`s for storage incurs overhead

10. It costs more gas to initialize non-`constant`/non-`immutable` variables to zero than to let the default of zero be applied

11. `++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)

12. Splitting `require()` statements that use `&&` saves gas

13. Usage of `uints`/`ints` smaller than 32 bytes (256 bits) incurs overhead

14. Using `private` rather than `public` for constants, saves gas

15. Duplicated `require()`/`revert()` checks should be refactored to a modifier or function

17. `require()` or `revert()` statements that check input arguments should be at the top of the function

19. Use custom errors rather than `revert()`/`require()` strings to save gas

20. Functions guaranteed to revert when called by normal users can be marked `payable`

21. Don't use `_msgSender()` if not supporting EIP-2771