Table of Contents
- Layer 2
- From 2023-09-13
- To 2023-09-15
- Total Issues
- 3 (3 resolved)
- Critical Severity Issues
- 0 (0 resolved)
- High Severity Issues
- 0 (0 resolved)
- Medium Severity Issues
- 0 (0 resolved)
- Low Severity Issues
- 2 (2 resolved)
- Notes & Additional Information
- 1 (1 resolved)
We performed a diff audit of the
scroll-tech/scroll repository for pull request 887 at commit
02bce20, pull request 912 at commit
10743c2, pull request 893 at commit
f8b9da0, and pull request 943 at commit
In scope were the following contracts:
contracts └── src ├── L1/rollup │ ├── IL1MessageQueue.sol │ ├── L1MessageQueue.sol │ └── ScrollChain.sol ├── L2/predeploys │ └── L2TxFeeVaults.sol └── libraries └── FeeVault.sol
Scroll is an EVM-equivalent ZK-rollup designed to be a scaling solution for Ethereum. It achieves this by interpreting EVM bytecode directly at the bytecode level, following a similar path to projects like Polygon's zkEVM and Consensys' Linea.
This audit reviewed the addition of four different pull requests to the Scroll ZK-rollup protocol.
This report presents our findings and recommendations for the new additions to the Scroll ZK-rollup protocol. We urge the Scroll team to consider these findings in their ongoing efforts to provide a secure and efficient Layer 2 solution for Ethereum.
Insufficient Tests When Using BitMaps
Pull request 893 changes the way of popping and tracking skipped messages, using BitMaps and buckets instead. This represents a sensitive change compared to the previous version. However, only a single test case with a fixed random BitMap, count, and starting index was introduced.
In order to verify that the expected behavior of filling the buckets and skipping messages is the expected one from the rest of the code, consider adding more test cases, especially testing edge cases for filling buckets.
Update: Resolved in pull request 956 at commit
Implicit Limitation of Withdrawal
On pull request 912, the
FeeVault contract introduced the possibility to pass a parameter of the value to withdraw. However, this value is implicitly limited to the contract's current balance by the
sendMessage function call.
In order to fail early and return a clear error message (which would help when debugging a reverted transaction), consider adding a requirement that asserts that the value passed is equal to or less than the contract's balance.
Update: Resolved in pull request 954 at commit
Notes & Additional Information
Throughout the codebase, there are some instances of incorrect or misleading documentation. In particular:
- Pull request 943 refactored the functionality of the
FeeVaultcontract under the
L2TxFeeVaultcontract. However, the NatSpec was copied from the
FeeVaultcontract, stating: "The L2TxFeeVault contract contains the basic logic for the various different vault contracts used to hold fee revenue generated by the L2 system". Unless the
L2TxFeeVaultcontract is going to be used as a base contract for other future contracts, consider adjusting the documentation to reflect the current behavior.
- The comment in line 67 of
L1MessageQueue.solshould say "for dropped messages" instead of "skipped messages".
Consider resolving these instances of incorrect documentation to improve the clarity and readability of the codebase.
Update: Resolved in pull request 955 at commit
Throughout this 3-day audit, we reviewed the mentioned pull requests and identified two low-severity issues, as well as one note to improve the documentation of the codebase.