Comet Asset Extension Audit

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:

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.

1 Like