Summary
Timeline: From 2025-03-19 To 2025-03-21
Total Issues: 4 (0 resolved)
Medium Severity Issues: 3 (0 resolved)
Low Severity Issues: 1 (0 resolved)
Scope
This report provides a review of the contract’s readiness for audit, identifying concerns with the current design and offering recommendations for improvement.
The previously audited commit was covered in a detailed audit report, which can be found in this forum post. This assessment builds upon the previous audit and examines new changes introduced since then.
The OpenZeppelin team was asked to perform an audit assessment on pull request #4 of the woof-software/comet-wrapper repository at commit 9a5f84d, to review the changes introduced after the last audited commit 06b14b9.
In scope were the following files:
src
├── CometWrapper.sol
└── CometWrapperWithoutMultiplier.sol
Please note that issues identified in the audit-readiness assessment are non-exhaustive, but are surfaced to reduce delays related to code changes.
System Overview
The CometWrapper is an ERC-4626 vault that converts rebasing Comet tokens (e.g., cUSDCv3) into wrapped, non-rebasing tokens, making them easier to integrate with other protocols. The PR under review modifies the vault in order to allow users to deposit the Comet base token (e.g., USDC) instead of the Comet token itself, with the vault supplying the base tokens to the Comet on their behalf.
Medium Severity
mint Returns Share Amount Instead of Asset Amount
The mint function in ERC-4626 must return the asset amount required to mint the requested number of shares. However, the current implementation returns the number of shares minted.
To avoid integration issues with third-party protocols, consider updating mint to return the amount of underlying tokens deposited by the caller to mint the requested number of shares.
Share Minting Rounds Up
In ERC-4626, it is considered more secure to prioritize the Vault itself during calculations rather than its users. Specifically, when determining the number of shares to issue in exchange for a given amount of underlying tokens, the calculation should be rounded down.
However, the current implementation incorrectly rounds up ([1], [2]), which diverges from the previously audited version that correctly rounded down.
To mitigate potential accounting vulnerabilities within the Vault, it is recommended to round down when computing the number of shares to issue for underlying tokens.
previewDeposit and previewMint Do Not Adhere to ERC-4626 Specification
The previewDeposit function in ERC-4626 allows an on-chain or off-chain user to simulate the effects of their deposit at the current block, given current on-chain conditions. According to the specification, previewDeposit must return an amount that is as close as possible to, but no more than, the exact number of Vault shares that would be minted in an actual deposit call within the same transaction, while also accounting for deposit fees.
However, the current previewDeposit implementation does not account for potential transfer fees applied during deposit. This discrepancy could lead to previewDeposit returning a higher number of shares than what would actually be minted in a deposit call.
Similarly, previewMint does not currently account for transfer fees on its returned amount of assets, which could cause mint to transfer fewer shares than expected to the user.
To ensure compliance with the ERC-4626 specification, consider updating previewDeposit and previewMint to properly reflect any applicable transfer fees.
Low Severity
Inconsistent Asset Usage in previewDeposit and deposit
According to the ERC-4626 specification, the deposit function should accept the underlying asset, deposit it into the vault, and mint corresponding shares to the depositor. However, in the customized Compound vault implementation, the asset accepted by the deposit function is the base asset of the underlying asset, rather than the underlying asset itself.
This discrepancy also introduces inconsistencies between the previewDeposit and deposit functions. Specifically, the asset used by the deposit function is the base asset of the underlying token (Comet), whereas the asset used by the previewDeposit function is the underlying token itself. Such inconsistencies deviate from ERC-4626 best practices and may lead to integration issues with other protocols relying on strict adherence to the ERC-4626 standard.
Consider aligning the deposit and previewDeposit functions by consistently using the underlying asset as specified in ERC-4626 to resolve this inconsistency and prevent potential integration problems.
Recommendations
Vault and Comet Contracts Can Be Integrated Through a Router
The current changes to the vault allow users to deposit the Comet base token (e.g., USDC) instead of the Comet token itself (e.g., cUSDCv3), with the vault supplying the base tokens to the Comet on their behalf.
However, this approach introduces unnecessary complexity to the vault and does not integrate well with the ERC-4626 specification.
Instead of modifying the vault directly, consider using a Router contract that supplies the base token to the Comet on behalf of the user, deposits the received cTokens into the vault, and sends the wrapped vault tokens to the user.
supplyFrom Can Be Used to Avoid Intermediate Transfers
When implementing intermediate contracts designed to supply tokens to a Comet on behalf of users, consider using the supplyFrom function to transfer tokens directly to the Comet, avoiding an intermediate transfer.
Note that this approach requires users to allow the intermediate contract to supply tokens on their behalf.
Conclusion
Given the concerns outlined above, we believe the current codebase is not yet fully prepared for a comprehensive security audit.
Consider reviewing the implementation of CometWrapper and evaluating the possibility of using a Router contract instead of modifying the ERC-4626 contract. This approach would help reduce complexity and make the code less prone to errors.