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
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.
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 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":
Scroll's sequencer is based on the London EVM version with some custom changes and EVM Shanghai inclusions:
BASEFEE opcode.COINBASE).PUSH0 opcode.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.
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.
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:
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.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.The Scroll network has several privileged roles:
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.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.ScrollChain contract to commit to new batches that bundle multiple L2 blocks in chunks that can then be finalized along with a proof.ScrollChain contract to prove batches that have already been committed by the sequencer, thereby finalizing them.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.
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.
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.
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
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
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:
ScrollBridgeReceiver.solScrollBridgeReceiver.solFollowing 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
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.