August 1, 2023
This security assessment was prepared by OpenZeppelin.
We were asked to audit the comet_wrapper repository at commit 13f2081. In scope were the following contracts:
src/
├─ CometHelpers.sol
└─ CometWrapper.sol
The Comet Wrapper is an ERC-4626 vault which wraps around a Comet token like cUSDCv3. It is designed to provide a non-rebasing token, due to the rebasing nature of the Comet token making it inconvenient to use, while still being able to track the interest accrued. The design anchors the wrapper's "share" balance to the Comet token's "principal" since the principal is not affected by the rebasing nature of the Comet token. This design allows the Comet Wrapper to rely on Comet to calculate the conversion between the share and the asset amount.
Another major function of the Comet Wrapper is to track individual users' Comet reward balance and allow them to withdraw rewards directly through it. This is achieved by tracking each user's reward accrued through trackingSupplyIndex from Comet as well as the amount claimed with a rewardClaimed mapping.
The codebase uses dependencies from the Solmate library, which we assume works as documented. We also assume Comet works as documented since the wrapper relies on it.
The wrapper allows depositors to claim Comet rewards directly, by keeping an internal track of Comet rewards for each depositor through userBasic[]. The internal tracking accrues users' rewards with the latest trackingSupplyIndex from Comet whenever a user's wrapper balance changes, which happens during deposit, mint, withdraw and redeem.
However, the wrapper does not accrue users' rewards during a transfer, which means the sender could lose the unaccrued rewards while the receiver could gain extra rewards depending on how long it has been since the last time rewards were accrued.
Consider accruing rewards for both the sender and receiver in transferInternal.
Update: Resolved in pull request #2 at commit 1f2899d.
redeem May Lead to InsolvencyDue to how Comet computes transfers, it is possible that the intended transfer in redeem reduces the Comet "principal" of the wrapper by more than shares.
During a transfer within Comet, the sender's "principal" balance is computed by converting it to "underlying" value, subtracting the transfer amount, and converting back to "principal". During these conversions, the result is rounded down. This may result in a "lost unit" of principal, where the sender loses one more unit of principal than the receiver gets.
In the case of redeem, this means that the Comet Wrapper's principal balance decreases by shares + 1, while only shares is burned. This can lead to the insolvency of the Comet Wrapper.
Consider computing assets in redeem using some method other than convertToAssets. Consider implementing the various calculations which happen in Comet to ensure that the change in principal of the Comet Wrapper is always equal to shares for whatever assets value is computed. Consider that a loss of 1 unit is possible each time redeem is called in its current state.
Update: Resolved in pull request #6 at commit 0bea271.
redeem Accrues Rewards IncorrectlyThe redeem function burns shares before accruing rewards, which results in an incorrect reward calculation for the user.
Rewards owed on the amount being redeemed will not be accounted for, as the reward accrual will use the balanceOf the user after their shares have been burned.
Consider moving the accrual actions to the beginning of the redeem function to properly account for the accrual of rewards.
Update: Resolved in pull request #4 at commit 0bea271.
redeemWithin mint, deposit, and withdraw, there are checks enforcing that the user's balance change equals the change in Comet's principal of the wrapper.
However, this invariant is not enforced in the redeem function.
Consider enforcing this rule in the redeem function to ensure accurate accounting within the wrapper. See related issues "redeem May Lead to Insolvency" and "Repeated code".
Update: Resolved in pull request #6 at commit 0bea271.
mint Does Not Follow ERC-4626The ERC-4626 specification states that the mint function "mints exactly shares Vault shares to receiver by depositing assets of underlying tokens."
Within the Comet Wrapper, this is not the case. The function re-computes shares and thus does not mint exactly the user-specified number as defined in the specification.
Consider a different computation of assets so that the principal balance of the wrapper will change by exactly shares as specified by the user or caller. Examine the balance-changing computations in Comet to assist. Also, see the related issue "redeem May Lead to Insolvency".
Update: Resolved in pull request #7 at commit 52fb0ef. Note that the fix is simply documenting the discrepancy between the behavior and the ERC-4626 definition.
Within the CometWrapper contract, there are many instances of repeated code that should be removed.
accrueRewards and updateTrackingIndex implement nearly identical logic, with the exception of the call to comet.accrueAccount within accrueRewards. All calls to updateTrackingIndex and their preceding accrueInternal calls can be replaced by a call to accrueRewards. updateTrackingIndex and accrueInternal could be removed.claimTo and getRewardOwed implement similar logic. The duplication between these functions can be removed by having claimTo make a call to getRewardOwed instead of implementing its logic a second time.principal before and after transfers to or from it (present in the deposit, mint, and withdraw functions) can be functionalized.This duplicated code should be removed as it greatly increases the surface for error, as well as gas consumption. Slight differences in duplicated code may be overlooked by developers or reviewers resulting in errors or vulnerabilities. Duplicated code also makes the entire codebase harder to comprehend and document.
Update: Partially resolved in pull request #5 at commit 3dd46f2. The third bullet point has been acknowledged, but a change was decided against for code readability reasons.
The ERC-20 standard spec states that ERC-20 tokens MUST not revert on zero-value transfers. The ERC-4626 Standard states that the ERC-4626 vault MUST implement ERC-20. transfer and transferFrom may revert if an ERC-4626 vault is non-transferrable, but this is not the case with the Comet Wrapper.
Therefore, transfer and transferFrom calls should not revert on transfers of 0 in order to be ERC-4626 compliant.
Currently, transferInternal disallows transfers where amount == 0. Consider removing this check to be compliant with ERC-4626. Understand that integrations may compute a transfer amount of 0 and still expect the call to succeed. Alternatively, consider documenting the discrepancy versus the ERC-4626 specification for external developers to be aware.
Update: Resolved in pull request #3 at commit e43b148.
CometWrapper.sol's function convertToAssets() is missing docstrings.
Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #7 at commit e43b148.
Three high and many lower-severity issues were discovered. Overall, the codebase's design is compact and well thought out, but lacking in consistency and somewhat scattered. Some recommendations were made to improve the code's quality and efficiency.
We recommend increasing the test coverage, utilizing fuzzing and formal verification to enforce invariants such as balance guarantees for the wrapper, and checking that rewards accrual works after users perform various other actions with the wrapper. We also recommend writing extensive formal documentation, indicating the various guarantees of the wrapper and the effects that rounding within the Comet protocol may have on its functionality.
After deployment, monitoring the Comet Wrapper system can assist in diagnosing bugs or exploits and stopping them as quickly as possible. It may also prove a useful source of data for analytics at a later time. Here, we recommend some monitoring strategies to ensure the healthy operation of the protocol after deployment.
mint/deposit/redeem/withdraw). This may indicate a bug in the front end or misinformation in the documentation.Deposit and Withdraw events along with the principal and balance changes of the sender and recipient. Ensure the changes in the balances of the sender and recipient match, and ensure that the conversion between assets and shares is reasonable.accrueRewards or claimTo as they may indicate an issue related to rewards accounting.claimTo with the same to address, as it may indicate a malicious frontend exploiting users for their rewards.Deposit and Withdraw events from the same address within a short timeframe. It may indicate a mathematical exploit or abuse of the system.