Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #84

Open
code423n4 opened this issue Jun 18, 2022 · 7 comments
Open

QA Report #84

code423n4 opened this issue Jun 18, 2022 · 7 comments
Assignees
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") valid

Comments

@code423n4
Copy link
Contributor

QA Report

Table of Contents

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

BeefyVaultOperator.sol

scope: deposit

BeefyZapBiswapLPVaultOperator.sol

scope: deposit

BeefyZapUniswapLPVaultOperator.sol

scope: deposit

YearnCurveVaultOperator.sol

scope: deposit

scope: withdraw128

scope: withdraw256

TOOLS USED

Manual Analysis

MITIGATION

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

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 18, 2022
code423n4 added a commit that referenced this issue Jun 18, 2022
@maximebrugel maximebrugel self-assigned this Jun 21, 2022
@obatirou
Copy link
Collaborator

Scientific notation (disputed)

In our opinion it is not better for readability

@obatirou
Copy link
Collaborator

Payable functions when using ERC20 (disputed)

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

@Yashiru
Copy link
Collaborator

Yashiru commented Jun 24, 2022

Constants instead of magic numbers (Duplicated)

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

@obatirou
Copy link
Collaborator

obatirou commented Jun 24, 2022

Events indexing (duplicate)

Duplicate from #11 (comment)

@Yashiru
Copy link
Collaborator

Yashiru commented Jun 24, 2022

Typos (Duplicated)

Duplicated of #45 at Typos

@Yashiru
Copy link
Collaborator

Yashiru commented Jun 24, 2022

Immutable addresses lack zero-address check (Duplicated)

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

@obatirou
Copy link
Collaborator

obatirou commented Jun 24, 2022

Comment Missing function parameter (confirmed)

Missed occurences:

This was referenced Jun 24, 2022
@Yashiru Yashiru added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") valid
Projects
None yet
Development

No branches or pull requests

5 participants