Date: March 17, 2023
We were asked to review the Comet repository on branch silver/optimism-deployment
at commit 303315178a59120c6458b4e6ec35555d55b3f295. The following files were in scope:
contracts/IGovernorBravo.solcontracts/bridges/optimism/OptimismBridgeReceiver.solcontracts/bridges/BaseBridgeReceiver.solcontracts/vendor/Timelock.solcontracts/ITimelock.soldeployments/optimism/usdc/configuration.json
This deployment is currently marked as a "work-in-progress" project, which means that it may have further changes and that parameters are still under revision.
From the scope above, issues related to all contracts except for OptimismBridgeReceiver and BaseBridgeReceiver were documented in the Compound Polygon Bridge Receiver Audit, as the same codebase is shared by both deployments. As a result, several findings from the previous report affect the current scope and will be addressed as part of remediation of that audit.
Update: The Compound team applied several fixes based on our recommendations. We have reviewed and addressed each fix submitted by the team and updated the report accordingly.
Similar to the previous Polygon deployment report, Compound also plans to release Comet into the Optimism network which shares a similar architecture.
In order for the Compound Governor contract from Ethereum Mainnet to communicate with the Optimism deployment, a proposal needs to be submitted, voted on, and passed in the usual manner. Then, it will simply be sent through the Optimism bridge to the OptimismBridgeReceiver contract. This contract will then process the message and relay it to Optimism's deployed Timelock contract.
The receiver and the timelock are interlinked but have different responsibilities, which allows either of them to be replaced with a different contract in the future. The receiver is meant to receive messages from the bridge, enqueue the transactions, and initiate their execution. The timelock serves the purpose of ensuring transactions can only be executed during the correct execution period and it handles execution when called upon by the receiver.
The Optimism network has been actively developing the protocol. This means that projects that develop on top of it must take into consideration that low-level changes may be applied in the near future, and the usage of certain instructions in the contracts could output a different result or the reversion of the transactions. For instance, relying on the block number, its rate or the transaction origin may result in unexpected behavior.
A new Bedrock update will be implemented soon and more changes to the network will be made. Deployments in this network may need to be more closely monitored.
Additionally, synchronization between L1 and L2 is not guaranteed. This can result in critical actions being executed at different times across networks. If a proposal depends on such actions, it should account for this by incorporating a buffer, to ensure all networks are in sync.
The Optimism network enables the replaying of a message, which will resend a transaction with a higher gas limit. This would make it possible to send the same transaction twice, which will have a different eta. So it must be assumed that identical transactions can be part of a single proposal.
Currently, the sequencer is centralized and can order the transactions in any way that they see fit. This means that proposals can have a different order from how they were submitted. Once the sequencers become decentralized, the Compound team must assume that MEV could happen.
At the moment, the maximum block gas limit in Ethereum is 30M (30,000,000). However, the upcoming Optimism Bedrock update will propose a "target gas limit of 2.5M with an elasticity multiplier of 8", which will have an upper bound of 20M.
As a proposal queues and executes several transactions at the same time, the execution in L2 may fail due to this smaller limit (compared to mainnet).
Consider documenting this limit so it is taken into consideration when creating the proposals. This will prevent transactions from unexpectedly hitting the gas limit in L2 and reverting.
Update: Acknowledged, not resolved. The Compound team stated:
We should note the limits of the bridge transaction needing to be able to execute on the L2 chain. This is also mitigated by simulating the full proposal, its effects, and the scenario suite on top. Also, it seems like Optimism is increasing their block gas limit to 30M from 20M (ethereum-optimism/optimism#2911 (comment))
The initialize function from the BaseBridgeReceiver contract has external visibility and can be called by anyone. This allows any user to call the initialize function after deployment and set the contract's governor and local time-lock addresses at will. In the case that a malicious user front runs the Compound team to call the initialization function, the deployment would have to be abandoned and a new set of BridgeReceiver and Timelock contracts would have to be deployed. While this is manageable, this situation could be exploited by a malicious actor who can keep calling the initialization function and causing a denial of service for that network deployment.
Consider either renaming the initialize function and only allowing the original deployer of the contract to call the function, or encapsulating the entire deployment under a single transaction to prevent someone else from calling the initialize function.
Update: Acknowledged, not resolved. The Compound team stated:
The BridgeReceiver contract is deployed and then initialized shortly thereafter (these events occur in the same deploy script).
It is true that the contract could be maliciously initialized by anyone. In the event that this happened, we would simply abandon the deployment. There would be no borrow/supply activity in the protocol at that point in time, so there would be no harm. We would simply need to redeploy.
Limiting initialization to a particular address (e.g. the contract deployer) would require storing an
adminorinitializervariable on the contract. Even if that address has a single, one-time capability, it gives the appearance of an address with special privileges.
The OptimismBridgeReceiver.sol file defines the OvmL2CrossDomainMessengerInterface interface and the OptimismBridgeReceiver contract. However, it is recommended to have a single definition per file and import the dependencies when needed.
Consider splitting the file to have a single asset per file.
Update: Resolved in pull request #699 at commit 32bf496.
The deployments/optimism/usdc/deploy.ts script has a comment referring to the PolygonBridgeReceiver contract, which instead should be referring to the OptimismBridgeReceiver contract.
Consider updating the comment to reflect the correct contract.
Update: Resolved in pull request #699 at commit 32bf496.
No critical or high severity issues were found.
Due to the shared contracts between the Optimism and Polygon deployments, all monitoring recommendations documented in the Compound Polygon Bridge Receiver Audit report should be considered.
In addition, specifically for the Optimism deployment, consider monitoring the mempool for any call to the replayMessage function from the L1CrossDomainMessenger contract that passes the data from a Compound submitted proposal, to detect potential inclusions of a similar proposal.