Platform: Code4rena
Start Date: 26/05/2023
Pot Size: $100,000 USDC
Total HM: 0
Participants: 33
Period: 14 days
Judge: leastwood
Id: 241
League: ETH
Rank: 9/33
Findings: 1
Award: $813.40
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: rbserver
Also found by: 0x73696d616f, 0xTheC0der, 0xdeadbeef, 0xhacksmithh, Bauchibred, GalloDaSballo, KKat7531, Madalad, MohammedRizwan, Rolezn, SAAJ, SanketKogekar, Sathish9098, VictoryGod, brgltd, btk, codeslide, descharre, hunter_w3b, jauvany, kaveyjoe, ladboy233, nadin, niser93, shealtielanz, souilos, trysam2003, yongskiws
813.4016 USDC - $813.40
| ID | Title |
|---|---|
| 01 | Prevent duplicated output roots |
| 02 | Prevent SUBMISSION_INTERVAL from being zero |
| 03 | Add a check to prevent out of bounds L2 output indexes |
| 04 | Validation for withdrawal transaction in OptimismPortal.proveWithdrawalTransaction() |
| 05 | Outdated solidity version |
| 06 | Outdated OpenZeppelin version |
| 07 | Floating pragma |
| 08 | Missing zero address checks on the constructor |
| 09 | Emit events before external calls |
| 10 | Avoid large functions |
| 11 | Lack of CEI |
| 12 | Use function modifier for validating input arguments |
| 13 | Critical setter functions should use two step pattern |
| 14 | Missing event for parameter changes |
| 15 | Lack of old and new value for events in setter functions |
| 16 | Check for stale values on setter functions |
| 17 | Allow max length to be modifiable in RLPReader |
| 18 | Move input arguments validation to the top of the function |
| 19 | Declare each contract/interface on a separate filed. |
| 20 | Replace conditionals with ternary |
| 21 | Convert internal function to private where possible |
| 22 | Use scientific notation |
| 23 | Convert formula to match comments |
| 24 | Usage of require statements |
| 25 | Use descriptive names for function parameter names |
| 26 | Array cache length and prefix increment for loops |
| 27 | Order of functions |
| 28 | Missing description for return variable on NATSPEC |
| 29 | External library contracts should be imported at the top |
| 30 | Boolean expressions can be checked directly |
| 31 | Constants in comparisons should appear on the left side |
| 32 | Assigning the default values |
| 33 | Use delete instead of assigning zero |
Consider preventing duplicated outputRoots in L2OutputOracle.proposeL2Outputs(). The impact of having duplicated roots could be inconsistent behavior when deleting/replacing roots.
One possible solution would be to store the roots into an auxiliary set/mapping and checking if the new item is not present in the set before populating the l2Outputs array.
If SUBMISSION_INTERVAL gets set with the value zero, the proposer could submit identical _l2BlockNumbers in L2OutputOracle.proposeL2Output() since the function nextBlockNumber() won't behave as expected.
Consider adding input validation when setting SUBMISSION_INTERVAL.
Consider adding a check in L2OutputOracle.getL2Output() when _l2OutputIndex is bigger or equal the length of the l2Outputs array.
Accessing elements for non-existing indexes will panic disregardless, but adding a check helps prevent accessing invalid memory and provides better security and stability for the contract since this function will be called throughout all withdrawals.
OptimismPortal.proveWithdrawalTransaction()Consider validating that _tx.target and _tx.sender are not address(0) to prevent inconsistent behavior when processing transactions payloads. If possible, consider also validating that tx_value is within an expected range.
Consider using the latest stable version of solidity (0.8.19) to ensure the compiler contains the latest security fixes.
The project is currently using OpenZeppelin version 4.7.3. Consider updating to the latest stable version - v.4.9.1 to ensure the project is using the up-to-date contracts from OpenZeppelin.
Consider avoiding locking the pragma when possible.
Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.
Consider adding a validation for _guardian in OptimismPortal. If this variable gets configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.
Wherever possible, consider emitting events before external calls. In case of reentrancy, funds are not at risk (for external call + event ordering), however emitting events after external calls can damage frontends and monitoring tools in case of reentrancy attacks.
Consider limiting the max number of lines per function. For example, NASA recommends not going beyond 60 lines per function (25th rule of NASA coding standard). Reference.
Wherever possible, consider making state updates before external calls to follow the checks-effects-interactions, even when the external calls are made to trusted contracts.
Consider converting manual validation with a function modifier where possible to improve code composability and abstraction.
Lack of two-step procedure for critical operations leaves them error-prone.
Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critical functionalities to the wrong address.
Adding events will facilitate offchain monitoring.
Events that mark critical parameter changes should contain both the old and the new value.
Add a check ensuring that the new value if different than the current value to avoid emitting unnecessary events.
RLPReader.sol will be used by the MerkleTrie.sol which will be used by OptimismPortal to validate withdrawal proofs. Consider allowing the array RLPItem[] to have a modifiable max length. One improvement could modify the contract to dynamically resize the array based on the actual number of RLP items encountered in the input. This would ensure that all elements are properly stored without exceeding the array bounds.
Consider validation input arguments on the top of the function body. Converting to a function modifier where possible is also recommended. Validating arguments on the top of the function will consume less execution gas for the failed calls.
For example, for L2OutputOracle.depositTransaction(), it could be helpful to validate _gasLimit and _data.length before validating _isCreation since there will be more calls for non contract creation instead of contract creation.
Consider avoiding multiple contracts/interfaces on the same file.
Consider refactoring conditionals with ternary expressions where possible. Ternary expressions will make the code more compact.
Internal functions not called by child contracts should be market private.
Consider converting values with 4+ zeros to use scientific notation to improve the readability of the codebase, e.g. 40000 => 40e3.
Consider refactoring the following equation in SafeCall.hasMinGas().
lt(mul(gas(), 63), add(mul(_minGas, 64), mul(add(40000, _reservedGas), 63)))
With
lt(mul(gas(), 63), add(mul(_minGas, 64), mul(63, add(40000, _reservedGas))))
To match the comment. Alternatively consider switch the values in the comment, e.g. gas × 63 ≥ minGas × 64 + (40_000 + reservedGas) × 63.
Consider refactoring require statements to custom errors where possible.
Custom errors are recommended when using the latest versions of solidity and using custom errors instead of require statements can also save gas.
Consider declaring names correlated to the intended usage and avoid generic names when possible.
Consider caching the array length outside the loop and applying an unchecked prefix increment where possible.
The solidity documentation recommends the following order for functions:
constructor receive function (if exists) fallback function (if exists) external public internal private
The instance bellow shows public above external.
Consider documenting all parameters and return values to improve the documentation present in the codebase.
External dependencies/libraries (e.g. OpenZeppelin) should be imported at the top. This is a common rule for linters on most programming languages.
Consider refactoring from:
require(deposits[_localToken][_remoteToken][_tokenId] == true)
To:
require(deposits[_localToken][_remoteToken][_tokenId] == true)
Doing so will prevent typo bugs.
In some cases it might be beneficial to let the default value be computed instead of initializing with the default value.
Consider using the delete keyword when resetting an integer.
#0 - c4-judge
2023-06-16T13:48:00Z
0xleastwood marked the issue as grade-b