Summary
Timeline: From 2024-07-23 To 2024-08-07
Total Issues: 20 (10 resolved, 2 partially resolved)
Critical Severity Issues: 2 (1 resolved)
High Severity Issues: 2 (1 resolved)
Medium Severity Issues: 2 (0 resolved)
Low Severity Issues: 9 (5 resolved, 1 partially resolved)
Notes & Additional Information: 5 (3 resolved, 1 partially resolved)
Scope
We started the audit at pull request #843 of the compound-finance/comet repository at commit 2e26fd5. Some of the issues in this report were added after further reviewing the contracts at commit 8291a94.
In scope were the following files:
contracts
├── CometRewardsInterface.sol
├── CometRewardsV2.sol
└── MerkleProof.sol
System Overview
The Comet protocol currently tracks user interactions to determine rewards, which can be claimed through the CometRewards.sol contract. In the current implementation, rewards can only be accrued in a single token. The new version, CometRewardsV2.sol
, is under review in this audit. This updated system allows users to redeem rewards in multiple tokens through campaigns. Campaigns are recurring reward periods that enable users to claim their rewards in a variety of tokens. They are designed to be inclusive of all users and are both initiated and concluded with a governance proposal.
For the bookkeeping of reward accruals for all users, a Merkle tree is created for all the users in the Comet protocol. The root of this tree is uploaded when starting or finishing a campaign. These roots should be calculated by the proposer of the starting or ending proposal and verified by the community to ensure correctness.
Security Model and Trust Assumptions
There are certain trust assumptions that must be upheld for the protocol to function correctly:
- The proposer of each proposal must accurately calculate the root of each Merkle tree and the community should then verify the correctness of the Merkle root.
- The
setRewardsClaimed
function should be used very cautiously by governance, especially when decreasing the claimed amount of a user. - Governance should ensure that
CometRewardsV2
has enough tokens to pay out all the rewards.
Since the calculation of the correct Merkle tree root is pivotal to the CometRewardsV2
contract working correctly, certain points should be highlighted:
- The root proposer needs to calculate the updated value of each user in the comet correctly. The
baseTrackingAccrued
function only returns each user’s reward tracker at the last point of accrual. It is essential that the root proposer correctly calculates this value when creating the Merkle root. - The data availability and full visibility at each step of the Merkle tree root creation is important upon proposal creation. When publishing the proposal, the root proposer should make all relevant data available, including the block number which the Merkle tree root is created with.
Critical Severity
All Campaign Rewards Can Be Stolen
The claimInternal
function is intended to be used to claim rewards for a user who was part of the startRoot
in the campaign. This function accrues the earned rewards for each user based on their tracking scale in Comet. However, in the case that the campaign is still underway and finishRoot == 0
, the proofs.finishAccrued
, which is provided by the user in the input, is not validated. Therefore, a malicious user can set the finishAccrued
variable in such a way that they can get all the rewards in the contract.
Conditions:
- A user is part of the campaign start Merkle tree.
- The user performs the exploit before the campaign ends. Meaning that the
compaigns[comet][compaignId].finishRoot == bytes32(0)
.
Steps:
- The attacker calls the
claim
function withProof.finishAccrued
maliciously set to a high number. - The
claim
function passes all values toclaimInternal
. - The attacker’s start Merkle proof would be successfully verified.
- Since the campaign has not ended, the verification of the finish Merkle proof will be skipped.
- The maliciously high
finishAccrued
would be passed to thegetRewardAccrued
function without any checks thatfinishAccrued
needs to be zero ifcompaigns[comet][compaignId].finishRoot == bytes32(0)
. - The attacker drains the contract.
Consider adding logic such that, if a campaign is not finished, the finishAccrued
amount is verified against the current amount in the Comet itself (e.g., the logic in claimInternalForNewMember
).
Update: Resolved in commit 401582d.
Reward Payment Ignores doTransferOut
Result
The functions for paying rewards to Comet users (i.e., claimInternal
and claimInternalForNewMember
) use the doTransferOut
function to pay the owed amounts. However, the owed amounts are updated regardless of the results of the transfer. If the transfer fails (e.g., insufficient token balance in the rewards contract), the payment would be considered successful by the protocol, and the debt fulfilled.
Consider adding a return value to the doTransferOut
function and only updating the claimed
amount if the transaction is successful.
High Severity
Finish Root Set by the Proposal is Not the Final Root
Whenever a campaign concludes, a proposal should be created to set the finishRoot
for the campaign, allowing users to claim the difference between their accruals at the start and end of the campaign. However, since setting a finish root must be proposed to governance, about a week will elapse before the proposal is executed and the finish root set, if the proposal is enacted. During this period, all users’ accrual indices either remain the same or increase, enabling users to claim more than the amount allowed by the finish root. This can happen if users call the claim function just before the proposal executes, as opposed to after the finish root is set. Therefore, users might claim more than expected by the finish root set at the time of proposing the conclusion of a campaign.
Consider having a finish time for each campaign that can be increased by governance. The users will be able to claim real time rewards only before the set deadline. If there are any unclaimed rewards after that, they will need to wait until the finish root is set by the proposal.
Update: Resolved in commit 401582d.
Some New Members Cannot Join Ongoing Campaigns
Whenever a campaign starts, the governor
role posts the starting root that aggregates all the up-to-date data to the contract. The root is created by forming a Merkle tree, with all the addresses in Comet and their relevant information hashed as the leaves of the tree sorted according to users’ addresses. Users that join the Comet after the starting root is posted claim rewards from CometRewardsV2
by proving that their address is not already a part of the campaign’s start Merkle tree by providing two neighbor nodes and their inclusion proofs in the tree.
However, users with addresses that are out of the range of the smallest to largest addresses as of the start of the campaign cannot join. This is because the VerifyNewMember
function needs two neighbors, but such accounts can only have one neighbor. As an example, if the smallest address that is included in the Merkle tree is address(0x2)
and the biggest address is address(0x8)
, then none of the addresses, address(0x01)
or address(0x9)
, can claim their rewards from an ongoing campaign if they joined after the startRoot
is created.
This issue can be mitigated in two ways:
- The preferred method: By handling such situations on-chain, so that users out of the range of the first and last users can join.
- By including the two addresses,
address(0x1)
andaddress(uint160(int160(-1)));
, in the leaves of the address while creating the root.
Medium Severity
Campaign Root Verification
The campaigns on CometRewardsV2
need the governor to provide the contract with a start and a finish Merkle root. These roots aggregate all the users’ tracking accruals at the time of start or finish of the campaign.
Take Alice and Bob as Comet users. If one of the roots is incorrect in favor of Bob, or against Alice, there is currently no mechanism in place for Alice to prove the incorrectness of the root. In this situation, Alice’s best hope is that governance will not enact a proposal to start or finish a campaign with the incorrect root.
Consider adding a mechanism such that any user can prove the incorrectness of a submitted root. This may be non-trivial but would be a positive step towards the decentralization of the system.
Lack Of Fork Tests
The current state of tests is only an expansion of the CometRewards
smart contract test suite. However, the CometRewardsV2
has a much more sophisticated structure and therefore needs to be tested thoroughly.
Consider adding fork tests that simulate real situations and verify the rewards contract works correctly with a comprehensive set of valid tests.
Low Severity
Incorrect Campaign ID is Emitted and Returned in setCompaignExt
In lines 152 and 155 of the setCompaignExt
function, the last index of the reward assets array of a campaign is returned and emitted as the campaign ID. The last index of the campaign array should be treated as the campaign ID. This bug would cause the off-chain indexer to store incorrect campaign IDs. Furthermore, two campaigns with the same number of assets will have the same ID.
Consider using the last index of the campaign array as the ID of the newly created campaign.
Update: Resolved in commit 401582d.
Some Events Lack Important Parameters
The ConfigUpdated
event has three parameters: address indexed comet
, address indexed token
, and uint256 multiplier
. However, tokens can be the same across different campaigns of a market. As such, the campaignId
should also be part of this event. The RewardsClaimedSet
event similarly lacks the campaignId
parameter while the RewardClaimed
event lacks the campaignId
and comet
parameters.
To enable indexers to better maintain the off-chain contract state, consider adding the aforementioned parameters to the respective events.
Update: Resolved in commit 401582d.
Gas savings
Throughout the codebase, multiple opportunities for gas savings were identified:
- In the
getRewardOwedBatch
function, thecomet.accrueAccount(address)
call can be made outside the loop. - The calls to
comet.accrueAccount(address)
inclaimInternalForNewMember
andclaimInternal
can be moved to the external functions so that when these functions are invoked from batch claim functions, theaccrueAccount()
call is not repeated.
Consider implementing the above suggestions to make the code more gas-efficient.
Update: Resolved in commit 401582d.
Missing Validations
Throughout the codebase, multiple instances of missing or insufficient validation of function arguments were identified:
- In the
claimBatchForNewMember
andclaimToBatchForNewMember
functions, the length of theFinisProof
array is not ensured to be equal to that of the other arrays. - In the
setNewCampaignWithCustomTokenMultiplier
function, campaigns can be started with 0 assets. - In the
setCampaignFinishRoot
function, it is not checked if the campaign is finished.
In order to reduce the attack surface of the codebase, consider finding all instances of missing/insufficient validations such as the ones mentioned above and validating the user-provided inputs.
Update: Partially resolved in commit 401582d. The last two instances were added in the fix review. Also, claimToBatchForNewMember
is still missing the finishProof
check.
Lack of Event Emission
The following functions do not emit relevant events after performing sensitive actions:
Consider emitting events after sensitive changes take place to facilitate off-chain tracking and monitoring.
Update: Resolved in commit 401582d.
ERC-20 Reward Tokens DoS
When users attempt to claim
their rewards, either the claimInternal
or the claimInternalForNewMember
function is triggered. These functions loop through each asset in the campaign and attempt to transfer them via the doTransferOut
function. If any of the reward token transfers fail, the doTransferOut
function reverts, preventing users from claiming any rewards from that campaign.
Instead of reverting on failure, consider modifying the doTransferOut
function to emit an event that logs the failure. This change would prevent the entire claim from being blocked by a single failing transfer. In addition, consider adding a function that allows users to claim each reward token individually. This could significantly enhance the robustness of the reward-claiming process, allowing users to bypass failed transfers and successfully claim other tokens.
Update: Resolved in commit 401582d.
Tokens With >=20
Asset Decimals Will Overflow
Whenever a new campaign is being started, the reward tokens are fed into the contract and a configuration object is created for each of them. However, the tokenScale
variable’s type is uint64
and it cannot store more than the value 18,446,744,073,709,551,615
which is roughly 1.8e19
. This limitation disables the protocol from giving out rewards in tokens that have more than 19 decimals. Also, note that rescaleFactor
in each configuration is also uint64
, but since it only stores the distance between the baseAccrualScale
and the tokenScale
, it can store a wider range of tokens.
Consider increasing the limit of both tokenScale
and rescaleFactor
to more than uint64
as the AssetConfig
struct has extra space available in its second slot.
Non-Standard ERC-20 Tokens Cannot Be Used By the Protocol
The current version of CometRewardsV2
expects all tokens to follow the ERC-20
standard. However, certain popular tokens like USDT might not follow this standard.
Consider supporting non-standard ERC-20 tokens by including the IERC20NonStandard
interface and handling the return value in areas of external calls like the doTransferOut
function in Comet.
Lack of Documentation to Explain the Architecture
The architecture of the CometRewardsV2
allows the use of multiple tokens in various campaigns as rewards. Given the non-trivial nature of the protocol architecture, the current documentation does not paint the complete picture.
Considering adding documentation and explaining the main intended flows of the CometRewardsV2
contract. It would also be helpful to have external documentation for users and security researchers in the future.
Notes & Additional Information
Typographical Errors
Throughout the codebase, multiple instances of typographical errors were identified:
Compaign
=>Campaign
Mercle
=>Merkle
multiplierues
=>multipliers
FinisProof
=>FinishProof
To improve code readability, consider addressing the aforementioned instances typographical errors and thoroughly examining the codebase to rectify any additional ones.
Update: Resolved in commit 401582d.
Naming Suggestions
Throughout the codebase, multiple opportunities for naming improvements were identified:
setCompaign
=>newCampaign
orsetNewCompaign
setCompaignExt
=>newCampaignWithCustomTokenMultiplier
orsetNewCampaignWithCustomTokenMultiplier
error InmultiplieridUInt64(uint256)
=>error InvalidUint64(uint256)
verifyMembership
=>verifyNewMember
setCompaignFinish
=>setCampaignFinishRoot
orfinishCampaign
Consider implementing the above renaming suggestions to improve code clarity and readability.
Update: Resolved in commit 401582d.
Missing Docstrings
-
Throughout the codebase, multiple functions have missing docstrings. For instance:
setCompaignExt
-
Several functions have incomplete docstrings. For instance:
getRewardOwedBatch
-
The structs and their parameters could use descriptive comments.
-
The mappings in the RewardsV2 contract can use keys. For instance:
claimed
Consider finding all such instances of a lack of documentation and adding descriptive comments to improve the readability of code.
Update: Resolved in commit 401582d.
Incorrect Comments
Throughout the codebase, multiple instances of incorrect or misleading comments were identified:
-
The docstring for the
setCompaign
function readsSet the reward token for a Comet instance
, whereas it adds a new campaign to a comet. -
The docstring for the
setCompaignFinish
function readsSet the reward token for a Comet instance
, whereas it is used to set thefinishRoot
of a campaign.
Consider correcting the aforementioned comments to improve the overall clarity of the codebase.
Update: Partially resolved in commit 401582d. The setCampaignFinishRoot
docstring is incorrect.
Code Redundancies
Throughout the codebase, multiple instances of code redundancies were identified:
- In lines 366 and 404, the accrue call to Comet does not need to be called if the
finishAccrued
has been set to a non-zero value. - The code in lines 773-790 can be replaced with the
getRewardsAccrued
function.
To improve the clarity and maintainability of the codebase, consider addressing the aforementioned instances of redundancies.
Conclusion
After reviewing the contracts, our team at OpenZeppelin concluded that the CometRewardsV2
contract requires additional development to ensure that it functions correctly. We identified critical and high vulnerabilities and, as mentioned in the report, the campaign’s finish requires further consideration due to the governance delay. We also recommend implementing structural changes to the protocol before it can be considered ready for deployment.