Scroll Alpha Comet Deployment Audit

OpenZeppelin Security | January 26, 2024

Table of Contents

Summary

Type
DeFi
Timeline
From 2023-12-11
To 2023-12-15
Languages
Solidity
Total Issues
5 (2 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
0 (0 resolved)
Low Severity Issues
3 (2 resolved)
Notes & Additional Information
2 (0 resolved)
 

Audit Scope

We audited the compound-finance/comet repository at commit 1492840, part of pull request #813.

In scope were the following files:

 contracts
├── Comet.sol
└── bridges  
    └── scroll
        ├── IScrollMessenger.sol
        └── ScrollBridgeReceiver.sol

Audit Context

Pull request #813 is part of Scroll's initiative to deploy a USDC Comet market to the Scroll network. The purpose behind this particular deployment is to test such a deployment on Scroll's deprecated Alpha network which has bridges to Ethereum's deprecated Goerli network. Scroll had expressed a desire to deploy to their own "Sepolia" testnet which has bridges to Ethereum's "Sepolia" testnet. However, Compound has not yet deployed the testing infrastructure to ETH "Sepolia" which is necessary to test cross-chain governance.

Following a successful Alpha network deployment, Scroll wants to use this same code to deploy a USDC Comet instance to the Scroll mainnet. A proposal would then be created to hand control and ownership to the Compound DAO at that time. Thus, this report not only summarizes our review of the contracts in scope but also aims to summarize the state of the Scroll mainnet. This is so that the Compound DAO is armed with all the relevant security information they need to be able to make an informed decision with a Scroll mainnet deployment.

Scoped Files Overview

The scoped files themselves are built off of the BaseBridgeReceiver contract. This particular contract has been audited by us in three previous audits and is utilized by Compound's current governance model to manage L2 Comet markets. Our previous audits go into significant detail on how this model works.

A brief summary would be that any admin functions that are to be called in L2 markets start as proposals on ETH mainnet through the master governor contract. These get wrapped in proposals to then send these transactions through a message-passing service, which will in turn send the data to the receiver contract on L2. The receiver works in tandem with an L2 timelock contract, just like L1's governor, to cache transactions and allow anyone on L2 to execute the transactions when the timelock allows it.

This model can be thought of as a dispatch model, whereby messages that fail to get through the message service can simply be re-proposed and dispatched again. There are several interesting network failure scenarios and other security considerations with such a model. These concerns have been raised in previous audits, with the community accepting the design and its security implications. We refer the reader to those previous audits for more details. Building off of this model, the code in scope is simply a rendition meant to work with the Scroll-specific messaging service contracts.

Scroll Network Overview

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 path similar to that of projects like Polygon zkEVM and ConsenSys' Linea.

Scroll's system architecture can be conceptualized as having three "layers":

  • Settlement Layer: Provides data availability and ordering for the canonical Scroll chain, verifies validity proofs, and allows users and dapps to send messages and assets between Ethereum and Scroll. Scroll uses Ethereum as the Settlement Layer and has the bridge and rollup contract deployed to Ethereum.
  • Sequencing Layer: Contains an Execution Node that executes the transactions submitted to the Scroll sequencer and the transactions submitted to the L1 bridge contract, and also produces L2 blocks. The layer also contains a Rollup Node that batches transactions, posts transaction data and block information to Ethereum for data availability, and submits validity proofs to Ethereum for finality.
  • Proving Layer: Consists of a pool of provers that are responsible for generating the zkEVM validity proofs that verify the correctness of L2 transactions, and a coordinator that dispatches the proving tasks to provers and relays the proofs to the Rollup Node to finalize on Ethereum.

EVM Equivalence

Scroll's sequencer is based on the London EVM version with some custom changes and EVM Shanghai inclusions:

Among these changes, the removal of the BASEFEE opcode is the only one that affects the Comet contract as the block.basefee is used to track the approximate gas of callers of the absorb function. To make the Comet contract compatible with the Scroll network, this line of code was simply removed. The potential impact of this change is limited to an economic one as it might change incentives for executing absorbs. Core contract functionality should not be affected.

Another noteworthy difference is Scroll's block gas limit being ten million, compared to ETH mainnet's thirty million. As we have stated before in these L2 deployment audits, large enough proposals would have to be broken up in order to be executed correctly.

Rollup and Bridging

The Scroll system connects to Ethereum primarily through its Rollup and Messenger contracts. The Rollup contract is responsible for "committing" and "finalizing". Committing is the act of receiving and storing the L2 transactions that have occurred, while finalizing is the act of receiving an L2 state root and verifying its correctness. The Messenger contracts enable users to pass arbitrary messages between L1 and L2, as well as bridge assets in both directions. The gateway contracts make use of the messages to operate on the appropriate layer.

The standard ERC-20 token bridge automatically deploys tokens on L2, using a standardized token implementation. There is also a custom token bridge which enables protocols to deploy their L1 token on L2, in case they want to maintain control over their deployment on L2. In such scenarios, the Scroll team would need to manually set the mapping for these tokens. Since any user can bridge ERC-20 tokens - including the COMP token - via the L1StandardERC20Gateway, it might happen that there are two versions of COMP deployed on L2. This would be the case if users bridge COMP before the Scroll team has the opportunity to set the token mapping of the custom ERC-20 gateway.

In deciding whether to use Scroll's standardized token implementation, both functionality and security should be considered. Since COMP on L2s is merely used in conjunction with CometRewards, protocol functionality should not be affected by using Scroll's ERC-20. From a security standpoint, it should be pointed out that one inherits the security risks of their implementation, if used. However, we consider the risks of doing so minimal. For further details, we refer to our previous audit of the ScrollStandardERC20 token. These considerations are intended to help in assessing and deciding which bridging strategy to use as part of a Scroll mainnet deployment.

Security Model and Trust Assumptions

Trust Assumptions

Scroll is under development and as such is tightly centralized by the development team. Scroll has expressed the desire to decentralize and has tentatively committed to decentralizing the proof generation first and the sequencer role second. Although, it could be years before full decentralization is achieved. Therefore, during the course of the audit, several assumptions about the Scroll protocol were made and considered to be inherently trusted. These assumptions and the context surrounding them include:

  • EVM node and relayer implementation: It is assumed that the EVM node implementation will work as described in the Scroll documentation, particularly the opcodes and their expected behavior. The relayer implementation is trusted to act in the best interest of the users.
  • Censoring: The sequencer and the prover have the ability to censor L2 messages and transactions. L1-to-L2 messages are appended to a message queue that is checked against when finalizing, but the sequencer can currently choose to skip any message from this queue during finalization. This allows the chain to finalize even if a message is not provable. Therefore, it is worth noting that L1-to-L2 messages from the L1ScrollMessenger or EnforcedTxGateway can be ignored and skipped. There are plans to remove this message-skipping mechanism after the mainnet launch, once the prover is more capable.
  • No escape hatch: The Scroll protocol does not feature an escape hatch mechanism. This, combined with the potential for transaction censorship by the relayer, introduces a trust assumption in the protocol. As such, in the event of the network going offline, users would not be able to recover their funds.
  • Whitelist ownership: The whitelist contract has an owner who can update the whitelist status of different addresses. This implies trusting the whitelist owner to manage this list correctly and in the best interest of the system and its users.
  • zk circuitry: Scroll's zk circuits are forked from Privacy Scaling Exploration's zkevm-circuits repo. These are used by provers to provide proof of transaction correctness. They have been audited five times and it is assumed that they work correctly for this audit.

Privileged Roles

The Scroll network has several privileged roles:

  • Access Control: The access control manager is a contract in which there is an address with default admin privileges that can perform the critical administrative action of giving and revoking roles for different addresses. This mechanism is used in the following contracts:
    • ScrollOwner: The default admin role can grant roles to addresses so that they can execute functions through the contract. Every role is associated with a set of functions, and any address with a given role will be able to call all the functions associated with that role. Moreover, existing roles will come with execution delays ranging from 0 days for instant execution to 1 day, 7 days, and 14 days. This provides a dynamic control mechanism over the timing of function execution based on their respective impact levels.
    • TokenRateLimiter: The default admin role can update the total token amount limit. The admin can also grant a token spender role for the Scroll gateways and messengers to ensure a rate limit when depositing or withdrawing funds.
  • Proxy Admins: Most of the contracts are upgradeable. Hence, most of the logic can be changed by the proxy admin. The following contracts are upgradeable:
    • The deposit and withdraw gateway contracts
    • L1MessageQueue
    • L1ScrollMessenger
    • L2GasPriceOracle
    • ScrollChain
    • L2ScrollMessenger
  • Implementation Owners: Most contracts are also ownable. The following actions describe what the owner can do in each contract.
    • L1ScrollMessenger: Pause the relay of L2-to-L1 messages and L1 to L2 message requests.
    • EnforcedTxGateway: Pause L1-to-L2 transaction requests and change the fee vault.
    • L1{CustomERC20|ERC721|ERC1155}Gateway: Change the token mapping containing information about which L1 token is bridged to which L2 token.
    • L1GatewayRouter: Set the respective gateway for ETH, custom ERC-20 tokens, and default ERC-20 tokens.
    • L1MessageQueue: Update the maximum allowed gas limit for L2 transactions, the gas price oracle to calculate the L2 fee, and the EnforcedTxGateway address that may append unaliased messages to the queue.
    • L2GasPriceOracle: Set the whitelist contract address that defines who may change gas-related settings.
    • ScrollChain: Revert previously committed batches that have not yet been finalized, set addresses as sequencers, change the verifier, and update the maximum amount of L2 transactions that are allowed in a chunk (bundle of blocks).
    • FeeVault: Change the messenger address that is used to withdraw funds from L2 to L1, the recipient address of the collected fees, and update the minimum amount of funds to withdraw.
    • ScrollMessengerBase: Change the fee vault address which collects fees for message relaying.
    • ScrollStandardERC20Factory: Use the factory to deploy another instance of a standard ERC-20 token on L2.
    • L2ScrollMessenger: Pause the relaying of L1-to L2-messages and L2-to-L1 message requests.
    • L2{CustomERC20|ERC721|ERC1155}Gateway: Change the token mapping containing information about which L2 token is bridged to which L1 token.
    • L2GatewayRouter: Set the respective gateway for ETH, custom ERC-20 tokens and default ERC-20 tokens.
    • L2MessageQueue: Update the address of the messenger.
    • L2TxFeeVault: Change the messenger address that is used to withdraw the funds from L1 to L2, the recipient address of the collected fees, and update the minimum amount of funds to withdraw.
    • L1BlockContainer: Initialize the starting block hash, block height, block timestamp, block base fee, and state root.
    • L1GasPriceOracle: Update the gas price and whitelist.
    • Fallback: Withdraw ERC-20 tokens and ETH, and execute arbitrary messages.
    • Whitelist: Accounts can be whitelisted to change the L2 base fee on L1 as well as the intrinsic gas parameters.
    • GasSwap: Withdraw stuck ERC-20 tokens and ETH, update the fees, and set the approved targets to call.
    • MultipleVersionRollupVerifier: Set the new verifier and their starting batch index.
    • ETHRateLimiter: Update the total ETH amount limit.
  • Sequencer: The sequencer role can interact with the ScrollChain contract to commit to new batches that bundle multiple L2 blocks in chunks that can then be finalized along with a proof.
  • Prover: The prover role can interact with the ScrollChain contract to prove batches that have already been committed by the sequencer, thereby finalizing them.

Further Details

OpenZeppelin has previously audited the following for Scroll:

We refer the interested reader to these reports to see how the system has developed and what the details of our security discussions with Scroll have been.

 

Low Severity

Missing Docstrings

Neither IScrollMessenger.sol nor ScrollBridgeReceiver.sol have docstrings.

Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Update: Resolved in pull request #822.

Function Is Updating the State Without an Event Emission

The ScrollBridgeReceiver contract sets the l2Messenger variable during construction. However, it does so without firing the relevant event for such an update.

Consider emitting the NewL2Messenger event during ScrollBridgeReceiver construction.

Update: Resolved in pull request #822.

Unused Variable

In the Comet contract's absorb function, the gasUsed variable is instantiated but never used.

In order to maintain cohesive, easily understood code, consider removing the unused variable entirely.

Update: Acknowledged, not resolved. The Scroll team stated:

Leaving gasUsed in the code to minimize code changes from the original Comet contracts

Notes & Additional Information

Lack of Security Contact

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions. However, neither IScrollMessenger.sol nor ScrollBridgeReceiver.sol contain such a contact.

Consider adding a NatSpec comment containing a security contact above the contract definitions. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Acknowledged, not resolved. The Scroll team stated:

Leaving as is to follow conventions of the Comet repo

Non-Explicit Imports Are Used

The use of non-explicit imports in the codebase can decrease the clarity of the code, and may create naming conflicts between local and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity files or when inheritance chains are long.

In ScrollBridgeReceiver.sol, global imports are being used:

Following the principle that clearer code is better code, consider using named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.

 

Update: Acknowledged, not resolved. The Scroll team stated:

Leaving non-explicit imports to comply with existing conventions in the Comet repo

Conclusion

The objective of the Comet deployment to the Scroll Alpha network is to hook into the community's testing framework and show its viability in preparation for a mainnet market deployment. The further into the future such a mainnet deployment would happen, the more stale this report will become. We exhort the community to prioritize deliberations around deploying to Scroll, taking into consideration the innovations of the network and the security concerns of such a centralized system.

Related Posts