Date: April 4, 2023
The compound-finance/comet repository at the 730d2d87 commit was reviewed. The following files were in scope:
contracts/CometRewards.solcontracts/bridges/arbitrum/AddressAliasHelper.solcontracts/bridges/arbitrum/ArbitrumBridgeReceiver.sol
This deployment of Comet to Arbitrum is the most recent one among a series of previous L2 deployments. The code for this deployment is based on the code used in the earlier deployments, which were also audited by OpenZeppelin. Consequently, numerous issues that were reported previously have already been resolved or improved upon in the current codebase.
Compound plans to continue deploying the lending market protocol Comet to more and more EVM-compatible networks. The next target is Arbitrum, an optimistic rollup protocol which inherits its security from Ethereum mainnet. The deployment follows an identical architecture to their other deployments, with the L1 Governor having privileged access to all the deployed contracts on L2. In order for the L1 Compound Governor contract to communicate with the Arbitrum contracts, a proposed execution needs to be submitted, voted on, and passed in the usual manner. Then, it will be sent through the Arbitrum bridge to the ArbitrumBridgeReceiver contract. This contract will then process the message and relay it to a 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 is responsible for executing transactions within and only within the correct execution period.
Arbitrum is a network that finalizes transactions via an optimistic rollup mechanism, posting all transactions to the Ethereum network for anyone to dispute and correct. The ecosystem involves numerous operators and roles, such as the sequencer, validators, the Arbitrum DAO, and others. By deploying to this network, there is an implicit trust in all of these entities to behave as expected. Hence, Compound must remain informed about the network's status and developments to prepare for any potential security issues that may arise. This would ensure that Compound stays up-to-date with the network's happenings and promptly responds to any unfavorable changes in the network's security posture.
Compound's security model is to have their mainnet Governor be the origin of all governance transactions on Arbitrum's deployments. This means that if Arbitrum's bridge were ever inoperable there would be no way to perform governance actions on the deployed Arbitrum contacts.
The ArbitrumBridgeReceiver contract implements a payable fallback function allowing ETH to be received. Furthermore, Arbitrum also implements a depositEth method in its inbox, allowing anyone to send ETH from L1 to an address/contract in L2 without triggering a contract's fallback function. Any ETH erroneously sent to the receiver in these ways will be inexorably lost without a function to sweep it.
Consider adding a way for the governor to sweep the contract of any ETH that may be stuck in it.
Update: Resolved in pull request 733 at commit 6fc6be6. A sweeping mechanism was added for both ERC-20s and native ETH for the local timelock.
The setRewardConfigWithMultiplier function from the CometRewards contract attempts to cast a uint to a uint64 and assign it to the variable named tokenScale. This cast attempt will revert if the number being cast is larger than the maximum for the uint64 type. This casting will always fail if the decimals() of the ERC-20 token being configured is twenty or greater. If any other ERC-20 tokens besides COMP are to be used as rewards, consider casting accrualScales from uint64 to uint256 to allow direct comparison with tokenScale and be able to utilize ERC-20 tokens with more decimals.
Update: Acknowledged, not resolved. The Compound team stated:
This is an edge case we do not expect to hit as almost all ERC-20 tokens have at most 18 decimals. We also want to avoid changing the sizes of any storage variables to keep the new implementation backwards-compatible with the old one..
In the AddressAliasHelper.sol file, consider pinning the version of the Solidity compiler to 0.8.15. This matches the rest of the files in scope and should help prevent introducing unexpected bugs as well as make verification of on-chain code considerably easier. Pay special attention to both the compiler's features and the list of known bugs associated with a chosen Solidity compiler version.
Update: Resolved in pull request 731 at commit ab02281.
While audits help in identifying potential security risks, the Compound team is encouraged to also incorporate automated monitoring of on-chain contract activity into their operations. Ongoing monitoring of deployed contracts helps in identifying potential threats and issues affecting the production environment.
Due to technical challenges in operating protocols across networks, we would suggest monitoring all events on Arbitrum to ensure that only expected activities are occurring. Particularly, consider monitoring:
- The
ProposalCreatedevent from theBaseBridgeRecievercontract to ensure that transactions are correctly submitted through the bridge. - The
ExecuteTransactionevent from theTimelockcontract to ensure only expected transactions are being executed. - The administrative events
NewPendingAdmin,NewDelay, andNewAdminfrom theTimelockcontract to ensure the correct ownership model is being handled. - The address calling the
executeProposalfunction, searching for previous deployments or code inside that address that could affect the execution of the proposal. - The retryable ticket queue in Arbitrum to make sure that bridge messages are handled correctly.