Open code423n4 opened 2 years ago
Quality assurance confirmed
They are not supposed to be called outside of a delegateCall context
Duplicate in QA report #73
Duplicate in QA report #76
Duplicated of #76 at 5. constants should be defined rather than using magic numbers
Confirmed
Missed occurrences found in QA report #64
TimelockControllerEmergency.sol, hashOperation
TimelockControllerEmergency.sol, hashOperationBatch
Quality assurance confirmed.
Missing occurrences:
Valid, upgrading is suggested.
Non-critical.
Valid and surfaced in previous audit.
Non-critical. Prefer not making changes.
Invalid.
Valid, best practices, make changes when you see fit.
The emit is done earlier to save gas, no?
Valid.
Non-critical. Prefer not making changes.
Valid, but no need to make changes.
Table of Contents
@openzeppelin/contracts
versionOwnableProxyDelegation.initialize()
is front-runnable in the solutionconstant
instead of duplicating the same stringconstant
insteadreturn
statement when the function defines a named return variable, is redundantpublic
functions not called by the contract should be declaredexternal
insteadLow Risk Issues
1. Known vulnerabilities exist in currently used
@openzeppelin/contracts
versionAs some known vulnerabilities exist in the current
@openzeppelin/contracts
version, consider updatingpackage.json
with at least@openzeppelin/contracts@4.4.2
here:https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/package.json#L65
While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)
2. Missing address(0) checks
Consider adding an
address(0)
check for immutable variables:3.
OwnableProxyDelegation.initialize()
is front-runnable in the solutionI suggest adding some access control or atomically initializing the contract:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/abstracts/OwnableProxyDelegation.sol#L24-L32
4. Use a
constant
instead of duplicating the same string5. Funds can be locked
There isn't a withdraw mechanism and several payable methods are implemented:
6. A magic number should be documented and explained. Use a
constant
insteadSimilar issue in the past: here
I suggest using
constant
variables as this would make the code more maintainable and readable while costing nothing gas-wise (constants are replaced by their value at compile-time).Non-Critical Issues
1. It's better to emit after all processing is done
2. Typos
3. Adding a
return
statement when the function defines a named return variable, is redundantWhile not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.
Affected code:
4.
public
functions not called by the contract should be declaredexternal
instead