Summary
Timeline: From 2024-09-23 To 2024-10-04
Total Issues: 4 (4 resolved)
Low Severity Issues: 2 (2 resolved)
Notes & Additional Information: 2 (2 resolved)
Scope
We audited the compound-finance/comet repository at commit 4f2ba04, part of pull request #904.
In scope were the following files:
contracts
├── AssetList.sol
├── AssetListFactory.sol
├── CometExt.sol
├── CometExtAssetList.sol
├── CometExtendedAssetList.sol
├── IAssetList.sol
├── IAssetListFactory.sol
└── IAssetListFactoryHolder.sol
System Overview
Comet is a monolithic lending market. Each market only has one lendable asset called the base asset. Users can either deposit the base asset and earn interest, or borrow the base asset with interest. The borrower needs to post collateral to be able to borrow the base asset. Each market has a separate set of collateral assets whitelisted by governance. There was a cap of 15 collateral assets for each market before this audit which could not be increased due to the contract size being close to the maximum size allowed by Ethereum.
The Comet Asset Extension scope created a modified version of Comet where the data of the collateral assets has been moved to be stored in a new contract called AssetList
. The AssetList
contract stores data for all the collateral assets of a market. Since the earlier collateral data was stored in the bytecode of the Comet implementation contract, this change would not break the upgradability of Comet. With this upgrade, a market can now have up to 24 collateral assets for users to post and borrow from the market.
Low Severity
IAssetList
Missing numAssets
Function
IAssetList
is an interface for the AssetList
contract. As such, it should contain all the public-facing functions of the AssetList
contract. However, it lacks the numAssets
function.
Consider adding the missing function to the interface to make it fully compatible with the AssetList
contract.
Update: Resolved in pull request #100 at commit f4860cf.
Gas Improvement
The AssetList
contract uses storage to store collateral data. Instead, it should use immutable variables akin to how it is done in the original Comet
contract to significantly reduce gas costs. Accessing storage variables costs considerably more than accessing immutable variables which are stored in contract bytecode.
Consider using immutable variables in the AssetList
contract to keep the costs of interacting with Comet
low.
Update: Resolved in pull request #100 at commit f4860cf.
Notes & Additional Information
Incomplete Docstrings
Throughout the codebase, multiple instances of incomplete or missing docstrings were identified:
- Parameters of the
getPackedAssetInternal
function of theAssetList
contract lack documentation. - The
createAssetList
function of theAssetListFactory
contract lacks documentation. - The
assetListFactory
function of theIAssetListFactoryHolder
contract lacks documentation.
To improve readability, consider thoroughly documenting all functions/events (and their parameters or return values) that are part of any contract’s public API.
Update: Resolved in pull request #100 at commit f4860cf.
Naming Ambiguity
The CometExt
contract is already a part of Comet’s primary contracts. However, new contracts are now using CometExtended
and CometExt
as a namespace. This can confuse readers due to how similar these names are to each other.
Consider selecting a more distinct name for the new contracts. This change will help clarify the distinction between the contracts, thereby improving readability.
Update: Resolved in pull request #100 at commit f4860cf.
Conclusion
The Comet Asset Extension scope introduces changes that will allow governance to whitelist up to 24 collaterals for a market. Previously, the number of collaterals for a market was limited to 12 or 15. OpenZeppelin reviewed the code changes and no major issues were found, while recommendations have been made to improve code clarity and readability. We believe that the audited changes would integrate well with already deployed markets and will not cause any issues when upgrading.