-
Notifications
You must be signed in to change notification settings - Fork 1
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
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
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
Scientific notation (disputed)In our opinion it is not better for readability |
Payable functions when using ERC20 (disputed)They need to be payable because they are called trough a delegateCall from a payable function. |
Constants instead of magic numbers (Duplicated)Duplicated of #76 at 5. constants should be defined rather than using magic numbers |
Events indexing (duplicate)Duplicate from #11 (comment) |
Immutable addresses lack zero-address check (Duplicated)Duplicated of #61 at 2. Missing address(0) checks |
Comment Missing function parameter (confirmed)Missed occurences:
|
This was referenced Jun 24, 2022
Open
Open
Closed
Open
Open
Closed
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
QA Report
Table of Contents
summary
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 ofmsg.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.The text was updated successfully, but these errors were encountered: