Nested Finance contest - c3phas's results

FINDINGS

No need to initialize variables with their default values

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied

File:NestedFactory.sol line 124

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

Other instances to modify File:OperatorResolver.sol line 40 File: TimelockControllerEmergency.sol line 84 File: TimelockControllerEmergency.sol line 89 File: TimelockControllerEmergency.sol line 234 File: TimelockControllerEmergency.sol line 324 File:NestedFactory.sol line 136 File:NestedFactory.sol line 196 File:NestedFactory.sol line 256

For the loops my suggestion would be to modify them as follows

        uint256 length = _tokens.length;
        for (uint256 i; i < length; i++) {

Similar thing to my proposal was implemented in this contract: File: BeefyVaultOperator.sol line 18

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

File: BeefyZapBiswapLPVaultOperator.sol line 27

Using unchecked blocks to save gas - Increments can be unchecked

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

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code File:OperatorResolver.sol line 75

    function rebuildCaches(MixinOperatorResolver[] calldata destinations) public onlyOwner {
        for (uint256 i = 0; i < destinations.length; i++) {
            destinations[i].rebuildCache();
        }
    }

The above should be modified to:

    function rebuildCaches(MixinOperatorResolver[] calldata destinations) public onlyOwner {
        for (uint256 i = 0; i < length; i++) {
            destinations[i].rebuildCache();
            unchecked { i++; }
        }
    }

Other Instances to modify File:OperatorResolver.sol line 40

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

File:NestedFactory.sol line 123-124

        bytes32[] memory operatorsCache = operators;
        for (uint256 i = 0; i < operatorsCache.length; i++) {

File:NestedFactory.sol line 136

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

File:NestedFactory.sol line 196

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

File:NestedFactory.sol line 256

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

see resource

Cache the length of arrays in loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: NestedFactory.sol line 124

    function addOperator(bytes32 operator) external override onlyOwner {
        require(operator != bytes32(""), "NF: INVALID_OPERATOR_NAME");
        bytes32[] memory operatorsCache = operators;
        for (uint256 i = 0; i < operatorsCache.length; i++) {
            require(operatorsCache[i] != operator, "NF: EXISTENT_OPERATOR");
        }
        operators.push(operator);
        rebuildCache();
        emit OperatorAdded(operator);
    }

We can cache the length of operatorsCache in the above

The above should be modified to

    function addOperator(bytes32 operator) external override onlyOwner {
        require(operator != bytes32(""), "NF: INVALID_OPERATOR_NAME");
        bytes32[] memory operatorsCache = operators;
             uint256 length = operatorsCache.length;
        for (uint256 i = 0; i < length; i++) {
            require(operatorsCache[i] != operator, "NF: EXISTENT_OPERATOR");
        }
        operators.push(operator);
        rebuildCache();
        emit OperatorAdded(operator);
    }

Other instances to modify

File: NestedFactory.sol line 649

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

File: OperatorResolver.sol line 57 & 60

    function importOperators(
        bytes32[] calldata names,
        Operator[] calldata operatorsToImport,
        MixinOperatorResolver[] calldata destinations
    ) external override onlyOwner {
        require(names.length == operatorsToImport.length, "OR: INPUTS_LENGTH_MUST_MATCH");
        bytes32 name;
        Operator calldata destination;
        for (uint256 i = 0; i < names.length; i++) {
            name = names[i];
            destination = operatorsToImport[i];
            operators[name] = destination;
            emit OperatorImported(name, destination);
        }


        // rebuild caches atomically
        // see. https://github.com/code-423n4/2021-11-nested-findings/issues/217
        rebuildCaches(destinations);
    }

names.length is being used in the require statement(1 mload-3 gas) and also in the loop(6 gas for every iteration),caching this should save some gas

File: OperatorResolver.sol line 75

    function rebuildCaches(MixinOperatorResolver[] calldata destinations) public onlyOwner {
        for (uint256 i = 0; i < destinations.length; i++) {
            destinations[i].rebuildCache();
        }
    }

File: MixinOperatorResolver.sol line 37

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

File: MixinOperatorResolver.sol line 56

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

A similar approach to my proposal has already been implemented on the following: File: NestedFactory.sol line 135

    function removeOperator(bytes32 operator) external override onlyOwner {
        bytes32[] storage operatorsCache = operators;
        uint256 operatorsLength = operatorsCache.length;
        for (uint256 i = 0; i < operatorsLength; i++) {
            if (operatorsCache[i] == operator) {
                operatorsCache[i] = operators[operatorsLength - 1];
                operatorsCache.pop();
                if (operatorCache[operator].implementation != address(0)) {
                    delete operatorCache[operator]; // remove from cache
                }
                rebuildCache();
                emit OperatorRemoved(operator);
                return;
            }
        }
        revert("NF: NON_EXISTENT_OPERATOR");
    }

Also implemented here: File: OperatorResolver.sol line 38

Splitting require() statements that use && saves gas

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save roughly 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File: BeefyVaultOperator.sol line 54

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

File: BeefyZapBiswapLPVaultOperator.sol line 64-65

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

File: ParaswapOperator.sol line 16

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

Proof The following tests were carried out in remix with both optimization turned on and off

    require ( a > 1 && a < 5, "Initialized");
    return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

    require (a > 1 ,"Initialized");
    require (a < 5 , "Initialized");
    return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

use shorter revert strings(less than 32 bytes)

You can (and should) attach error reason strings along with require statements to make it easier to understand why a contract call reverted. These strings, however, take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

File:TimelockControllerEmergency.sol line 256

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

File:TimelockControllerEmergency.sol line 359

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

File:TimelockControllerEmergency.sol line 375

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

Other instances to modify

File:TimelockControllerEmergency.sol line 334-335

File:TimelockControllerEmergency.sol line 319-320

File:TimelockControllerEmergency.sol line 243-244

File:TimelockControllerEmergency.sol line 229-20

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

see Source

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Use Shift Right/Left instead of Division/Multiplication

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

relevant source

File: BeefyZapBiswapLPVaultOperator.sol line 275

        uint256 halfInvestment = investmentA / 2;

The above should be modified to:

        uint256 halfInvestment = investmentA >> 1;

File: BeefyZapUniswapLPVaultOperator.sol line 273

        uint256 halfInvestment = investmentA / 2;

The above should be modified to:

        uint256 halfInvestment = investmentA >> 1;