Nested Finance contest - joestakey's results

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Constants instead of magic numbers
• Events indexing
• Public functions can be external
• Scientific notation
• Typos
• Immutable addresses lack zero-address check
• Payable functions when using ERC20

summary

Few vulnerabilities were found examining the contracts. The main concerns are with:

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Parameters missing a natspec comment include:

BeefyZapBiswapLPVaultOperator.sol

uint256 reserveA
uint256 reserveB
IBiswapRouter02 router
uint256 swapAmount\

BeefyZapUniswapLPVaultOperator.sol

uint256 reserveA
uint256 reserveB
IUniswapV2Router02 router
uint256 swapAmount

TimelockControllerEmergency.sol

bytes32 id
bytes32 id
bytes32 id
bytes32 id
bytes32 id
address target,uint256 value,bytes calldata data,bytes32 predecessor,bytes32 salt
address[] calldata targets,uint256[] calldata values,bytes[] calldata datas,bytes32 predecessor,bytes32 salt
address target,uint256 value,bytes calldata data,bytes32 predecessor,bytes32 salt,uint256 delay
address[] calldata targets,uint256[] calldata values,bytes[] calldata datas,bytes32 predecessor,bytes32 salt,uint256 delay
bytes32 id, uint256 delay
bytes32 id
address target,uint256 value,bytes calldata data,bytes32 predecessor,bytes32 salt
address target,uint256 value,bytes calldata data
address[] calldata targets,uint256[] calldata values,bytes[] calldata datas,bytes32 predecessor,bytes32 salt
bytes32 id, bytes32 predecessor
bytes32 id
bytes32 id,uint256 index,address target,uint256 value,bytes calldata data
uint256 newDelay

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

10000
10000
10000
10000
10000
10000

BeefyZapBiswapLPVaultOperator.sol

1000
1000

BeefyZapUniswapLPVaultOperator.sol

1000
1000

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MixinOperatorResolver.sol

event CacheUpdated(bytes32 name, IOperatorResolver.Operator destination)

BeefyVaultStorage.sol

event VaultAdded(address vault, address tokenOrZapper)
event VaultRemoved(address vault)

YearnVaultStorage.sol

event VaultAdded(address vault, CurvePool pool)
event VaultRemoved(address vault)

TimelockControllerEmergency.sol

event MinDelayChange(uint256 oldDuration, uint256 newDuration)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NestedFactory.sol

10000
10000
10000
10000
10000
10000

BeefyZapBiswapLPVaultOperator.sol

1000
1000

BeefyZapUniswapLPVaultOperator.sol

1000
1000

TOOLS USED

Manual Analysis

MITIGATION

Replace the numbers aforementioned with their scientific notation

Typos

PROBLEM

There are some typos/misspelt words in the contracts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BeefyVaultOperator.sol

WITHDRAWED

BeefyZapBiswapLPVaultOperator.sol

WITHDRAWED

BeefyZapUniswapLPVaultOperator.sol

WITHDRAWED

TOOLS USED

Manual Analysis

MITIGATION

Replace with WITHDRAWN

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Withdrawer.sol

weth = _weth

YearnCurveVaultOperator.sol

eth = _eth
weth = IWETH(_weth)
withdrawer = _withdrawer

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for these parameters.

Payable functions when using ERC20

PROBLEM

Some functions have the payable modifier but their logic does not make use of msg.value. These contracts do not have any way to withdraw ETH, meaning any ETH sent would get locked.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ParaswapOperator.sol

scope: performSwap

• payable

BeefyVaultOperator.sol

scope: deposit

• payable

BeefyZapBiswapLPVaultOperator.sol

scope: deposit

• payable

BeefyZapUniswapLPVaultOperator.sol

scope: deposit

• payable

YearnCurveVaultOperator.sol

scope: deposit

• payable

scope: withdraw128

• payable

scope: withdraw256

• payable

TOOLS USED

Manual Analysis

MITIGATION

There should be a require(0 == msg.value) in these functions to ensure no Ether is being sent.

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constructor parameters should be avoided when possible
• Default value initialization
• Mathematical optimizations
• Require instead of AND
• Revert strings length
• Shifting cheaper than division
• Tight variable packing
• unchecked arithmetic
• unnecessary computation

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()

• feeSplitter is read 3 times

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()

• bytes memory _swapCallData

OwnerProxy.sol

scope: execute()

• bytes memory _data

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;