Comet Rewards V2 Audit

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:

  1. The attacker calls the claim function with Proof.finishAccrued maliciously set to a high number.
  2. The claim function passes all values to claimInternal.
  3. The attacker’s start Merkle proof would be successfully verified.
  4. Since the campaign has not ended, the verification of the finish Merkle proof will be skipped.
  5. The maliciously high finishAccrued would be passed to the getRewardAccrued function without any checks that finishAccrued needs to be zero if compaigns[comet][compaignId].finishRoot == bytes32(0).
  6. 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:

  1. The preferred method: By handling such situations on-chain, so that users out of the range of the first and last users can join.
  2. By including the two addresses, address(0x1) and address(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, the comet.accrueAccount(address) call can be made outside the loop.
  • The calls to comet.accrueAccount(address) in claimInternalForNewMember and claimInternal can be moved to the external functions so that when these functions are invoked from batch claim functions, the accrueAccount() 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 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:

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:

Consider implementing the above renaming suggestions to improve code clarity and readability.

Update: Resolved in commit 401582d.

Missing Docstrings

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 reads Set the reward token for a Comet instance, whereas it adds a new campaign to a comet.

  • The docstring for the setCompaignFinish function reads Set the reward token for a Comet instance, whereas it is used to set the finishRoot 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.

1 Like

Hey. Thanks for the feedback. I would like to highlight, that the audit is outdated for 6 months (2024-07-23 To 2024-08-07).

The updated scope with all the described issues is updated and provided to OZ in December.

Document with updates - Report on Changes Made Following the Audit of Comet Rewards V2 - Google Docs
Testnet deployment - CometRewardsV2 | Address 0x8d88c1eb48e8549beac11b696944599db7b60520 | Etherscan
PR - Woof software/rewards v2 multiple rewards by dmitriy-woof-software · Pull Request #94 · woof-software/comet · GitHub

Currently, the scope is in OZ backlog.

1 Like