search

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

0 stars 1 forks source link

Gas Optimizations #82

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas Report

Table of Contents

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

scope: _transferFeeWithRoyalty()

line 573\ line 575\ line 577

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

ExchangeHelpers.sol

scope: fillQuote()

OwnerProxy.sol

scope: execute()

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

_entryFees <= 10000\ _entryFees <= 10000\ amountSpent <= _inputTokenAmount - feesAmount\ amountSpent <= _inputTokenAmount\ amounts[1] <= _amountToSpend\ address(this).balance >= _inputTokenAmount\ nestedRecords.getAssetHolding(_nftId, address(_inputToken)) >= _inputTokenAmount

BeefyVaultOperator.sol

vaultAmount >= minVaultAmount

BeefyZapBiswapLPVaultOperator.sol

vaultAmount >= minVaultAmount\ amountToDeposit >= depositedAmount\ tokenAmount >= minTokenAmount

BeefyZapUniswapLPVaultOperator.sol

vaultAmount >= minVaultAmount\ amountToDeposit >= depositedAmount\ tokenAmount >= minTokenAmount

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-vaultAmount >= minVaultAmount;
+vaultAmount > minVaultAmount - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters. With the compilers parameters in hardhat.config.ts, deployment costs approximately 400 more gas per variable written via a constructor parameter.

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

constructor

nestedAsset = _nestedAsset;
nestedRecords = _nestedRecords;
reserve = _reserve;
feeSplitter = _feeSplitter;
weth = _weth;
withdrawer = _withdrawer

Withdrawer.sol

weth = _weth

Paraswap.sol

tokenTransferProxy = _tokenTransferProxy\ augustusSwapper = _augustusSwapper

YearnCurveVaultOperator.sol

eth = _eth\ withdrawer = _withdrawer

TimelockControllerEmergency.sol

_minDelay = minDelay

OperatorScripts.sol

nestedFactory = _nestedFactory\ resolver = _resolver

TOOLS USED

Manual Analysis, hardhat

MITIGATION

Hardcode storage variables with their initial value instead of writing it during contract deployment with constructor parameters.

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0

OperatorResolver.sol

uint256 i = 0\ uint256 i = 0\ uint256 i = 0

MixinOperatorResolver.sol

uint256 i = 0\ uint256 i = 0

TimelockControllerEmergency.sol

uint256 i = 0\ uint256 i = 0\ uint256 i = 0\ uint256 i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

amountBought -= amountFees\ amountSpent += _submitOrder(address(tokenSold),_batchedOrders.orders[i].token,_nftId,_batchedOrders.orders[i],true // always to the reserve)\ ethNeeded += _batchedOrders[i].amount

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

require(address(_nestedAsset) != address(0) && address(_nestedRecords) != address(0) &&address(_reserve) != address(0) &&address(_feeSplitter) != address(0) &&address(_weth) != address(0) && _operatorResolver != address(0) && address(_withdrawer) != address(0), "NF: INVALID_ADDRESS" )

BeefyVaultOperator.sol

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

BeefyZapBiswapLPVaultOperator.sol

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

BeefyZapUniswapLPVaultOperator.sol

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

ParaswapOperator.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Break down the statements in multiple require statements.

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

You can also improve gas savings by using custom errors

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes. It costs 9,500 gas upon deployment per string exceeding that 32-byte size.

PROOF OF CONCEPT

Revert strings exceeding 32 bytes include:

TimelockControllerEmergency.sol

TimelockController: length mismatch\ TimelockController: length mismatch\ TimelockController: operation already scheduled\ TimelockController: insufficient delay\ TimelockController: operation cannot be cancelled\ TimelockController: length mismatch\ TimelockController: length mismatch\ TimelockController: operation is not ready\ TimelockController: missing dependency\ TimelockController: operation is not ready\ TimelockController: underlying transaction reverted\ TimelockController: caller must be timelock

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using custom errors.

Shifting cheaper than division

IMPACT

A division by 2 can be calculated by shifting one to the right. 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.

PROOF OF CONCEPT

Instances include:

BeefyZapBiswapLPVaultOperator.sol

uint256 halfInvestment = investmentA / 2

BeefyZapUniswapLPVaultOperator.sol

uint256 halfInvestment = investmentA / 2

TOOLS USED

Manual Analysis

MITIGATION

-investmentA / 2;
+investmentA >> 1;

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As bool type variables are of size 1 byte, there's a slot here that can get saved by moving one bool closer to an address

PROOF OF CONCEPT

Instances include:

OwnableProxyDelegation.sol

address private _owner; @audit - slot 1

/// @dev Storage slot with the proxy admin (see TransparentUpgradeableProxy from OZ)
bytes32 internal constant _ADMIN_SLOT = bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1);  @audit - slot 2

/// @dev True if the owner is setted
bool public initialized;  @audit - slot 3

TOOLS USED

Manual Analysis

MITIGATION

Place initialized after _owner to save one storage slot

address private _owner; @audit - slot 1

/// @dev True if the owner is setted
+bool public initialized; 

/// @dev Storage slot with the proxy admin (see TransparentUpgradeableProxy from OZ)
bytes32 internal constant _ADMIN_SLOT = bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1);  @audit - slot 2

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas. This is particularly true in for loops, as it saves some gas at each iteration.

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

i++\ i++\ i++\ i++\ i++\ i++\ i++\ i++\ i++

OperatorResolver.sol

i++\ i++\ i++

MixinOperatorResolver.sol

i++\ i++

BeefyVaultOperator.sol

i++

BeefyZapBiswapLPVaultOperator.sol

i++

BeefyZapUniswapLPVaultOperator.sol

i++

YearnCurveVaultOperator.sol

i++

CurveHelpers.sol

i++\ i++\ i++\ i++

OperatorScripts.sol

i++\ i++

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

When emitting an event that includes a new and an old value, it is cheaper in gas to avoid caching the old value in memory. Instead, emit the event, then save the new value in storage.

PROOF OF CONCEPT

Instances include:

OwnableProxyDelegation.sol

function _setOwner

TOOLS USED

Manual Analysis

MITIGATION

Replace

address oldOwner = _owner;
_owner = newOwner;
emit OwnershipTransferred(oldOwner, newOwner)

with

emit OwnershipTransferred(_owner_, newOwner)
_owner = newOwner;
Yashiru commented 2 years ago

Constructor parameters should be avoided when possible (Confirmed)

Yashiru commented 2 years ago

Unnecessary computation (Acknowledged)

Indeed, here we can do it to optimize gas consumption, but we prefer, for readability reasons, to emit the events after doing the storage update.

Yashiru commented 2 years ago

Shifting cheaper than division (Duplicated)

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

Yashiru commented 2 years ago

Calldata instead of memory for RO function parameters

Duplicated of #75 at Using calldata instead of memory for read-only arguments in external functions saves gas.

obatirou commented 2 years ago

Revert strings length (duplicated)

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

obatirou commented 2 years ago

Require instead of AND (duplicate)

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

maximebrugel commented 2 years ago

Tight Variable Packing (Disputed)

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

Yashiru commented 2 years ago

Default value initialization (Duplicated)

Duplicated of #2 at For loop optimizaion

Unchecked arithmetic (Duplicated)

Duplicated of #2 at For loop optimizaion

Yashiru commented 2 years ago

Mathematical optimizations (Confirmed)

Gas optimization confirmed.