Open code423n4 opened 2 years ago
We do not understand the problem
We do not understand the problem
If in a lib, it will cost more to retrieve the data It is less gas efficient, hence we prefer in contract byte code
We prefer to name our return variables even if they are not used, this allows to add an additional information about the nature of the returned variable by giving it a name.
Duplicate point 3 of qa report https://github.com/code-423n4/2022-06-nested-findings/issues/61
Quality assurance confirmed
Missing occurances (finded in #61):
Duplicate from https://github.com/code-423n4/2022-06-nested-findings/issues/11#issuecomment-1165618378
We want to be able to receive ether this way. But if neither a receive Ether nor a payable fallback function is present, the contract cannot receive Ether through regular transactions and throws an exception.
https://github.com/code-423n4/2022-06-nested-findings/issues/84#issuecomment-1165712399
This is invalids per comment pointed out: Contract might receive/hold ETH as part of the maintenance process.
Non-critical.
Invalid, this is not an upgradeable contract.
Invalid. Not upgradeable contracts.
Valid, but prefer not making changes, redundant return is better for readability.
Valid, no need to make changes.
Valid, best practices, make changes when you see fit.
Invalid.
Seems invalid.
Non-critical.
Non-critical.
Seems fine with me, not sure why this is an issue.
Note: The README lists a bunch of QA/Gas reports from prior findings, but doesn't list the specific issues found therein. Having each warden look at each report and de-dupe is just like adding more files to the scope, and I think is a bit unfair. For the ones that the sponsor has explicitly listed I've still included them because the sponsor may end up finding them useful, especially after seeing the gas amounts involved, but have the lines with strike-through, so the sponsor and judge can ignore them if they wish
Summary
Low Risk Issues
receive()
/fallback()
functionaddress(0x0)
when assigning values toaddress
state variablesTotal: 3 instances over 2 issues
Non-critical Issues
initializer
modifier on constructorinitializer
modifierreturn
statement when the function defines a named return variable, is redundantpublic
functions not called by the contract should be declaredexternal
insteadconstant
s should be defined rather than using magic numbers6Expressions for constant values such as a call tokeccak256()
, should useimmutable
rather thanconstant
4indexed
fieldsTotal: 70 instances over 10 issuesTotal: 66 instances over 9 issuesLow Risk Issues
1. Unused/empty
receive()
/fallback()
functionIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g.
require(msg.sender == address(weth))
)There is 1 instance of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L113
2. Missing checks for
address(0x0)
when assigning values toaddress
state variablesThere are 2 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L48
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L65
Non-critical Issues
1. Missing
initializer
modifier on constructorOpenZeppelin recommends that the
initializer
modifier be applied to constructorsThere are 2 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/Withdrawer.sol#L16
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L57-L65
2. Missing
initializer
modifierThe contract extends
ReentrancyGuard
/ReentrancyGuardUpgradeable
but does not use theinitializer
modifier anywhereThere are 2 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/Withdrawer.sol#L13
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L20
3. Adding a
return
statement when the function defines a named return variable, is redundantThere are 4 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L25
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L45
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L65
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L89
4.
public
functions not called by the contract should be declaredexternal
insteadContracts are allowed to override their parents' functions and change the visibility from
external
topublic
.There are 2 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/OwnerProxy.sol#L16
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L295-L299
5.
constant
s should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 25 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L271
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L269
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/OwnerProxy.sol#L21
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/scripts/OperatorScripts.sol#L59
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L161
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L38
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/CurveHelpers/CurveHelpers.sol#L41
6. Expressions for constant values such as a call tokeccak256()
, should useimmutable
rather thanconstant
There are 4 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L25
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L26
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L27
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L28
7. Constant redefined elsewhere
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an
internal constant
in alibrary
. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.There are 5 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L19
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L19
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L20
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L17
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L39
8. NatSpec is incomplete
There are 10 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L230-L237
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L230-L237
9. Event is missing
indexed
fieldsEach
event
should use threeindexed
fields if there are three or more fieldsThere are 8 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultStorage.sol#L12
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L17
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L37-L45
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/MixinOperatorResolver.sol#L14
10. Not using the named return variables anywhere in the function is confusing
Consider changing the variable to be an unnamed one
There are 8 instances of this issue:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L119