QA Report

[code423n4]

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.

[obatirou]

Scientific notation (disputed)

In our opinion it is not better for readability

[obatirou]

Payable functions when using ERC20 (disputed)

They need to be payable because they are called trough a delegateCall from a payable function.

[Yashiru]

Constants instead of magic numbers (Duplicated)

Duplicated of #76 at 5. constants should be defined rather than using magic numbers

[obatirou]

Events indexing (duplicate)

Duplicate from https://github.com/code-423n4/2022-06-nested-findings/issues/11#issuecomment-1165618378

[Yashiru]

Typos (Duplicated)

Duplicated of #45 at Typos

[Yashiru]

Immutable addresses lack zero-address check (Duplicated)

Duplicated of #61 at 2. Missing address(0) checks

[obatirou]

Comment Missing function parameter (confirmed)

Missed occurences:

• 2022-06-nested\contracts\NestedFactory.sol#L114 from #18
• MixinOperatorResolver.sol#L50 from #43
• OwnableProxyDelegation.sol#L34 from #43
• OwnableProxyDelegation.sol#L55 from #43
• NestedAsset from #64
• FeeSplitter from #64