OpenZeppelin, in its role as Security Partner to the Compound DAO, audited the latest version of the ERC-4626-compliant wrapper contract for the Comet token.
Summary
Timeline: 2024-09-05 - 2024-09-10
Languages: Solidity
Total Issues: 10 (10 resolved)
High Severity Issues: 1 (1 resolved)
Medium Severity Issues: 1 (1 resolved)
Low Severity Issues: 6 (6 resolved)
Notes & Additional Information: 2 (2 resolved)
Scope
OpenZeppelin reviewed pull request #28 of the compound-finance/comet-wrapper repository at commit 26fc656. Under review were the changes introduced since the last audit performed at commit 1c08f3f.
In scope were the following files:
contracts
├── src
│ ├── CometHelpers.sol
│ ├── CometWrapper.sol
│ └── vendor
│ ├── CometConfiguration.sol
│ ├── CometCore.sol
│ ├── CometExtInterface.sol
│ ├── CometInterface.sol
│ ├── CometMainInterface.sol
│ ├── CometMath.sol
│ ├── CometStorage.sol
│ ├── ICometConfigurator.sol
│ └── ICometRewards.sol
System Overview
The CometWrapper contract is an ERC-4626-compliant wrapper around the Comet market, allowing users to interact with a stable balance of shares tied to their principal in the Comet market. This wrapper provides a non-rebasing alternative, addressing potential integration challenges with the wider DeFi ecosystem due to the rebasing nature of Comet tokens.
This audit primarily involved fixes from previous findings and the removal of compliance with EIP-7246. Changes included updating Comet interfaces, downgrading the Solidity version, and refining the CometWrapper functionalities. The removal of EIP-7246 simplifies the interface by eliminating the ability to encumber shares, leaving the contract compliant only with ERC-20 and ERC-4626 standards. This reduces complexity while preserving key functionality, including reward accruals. However, Comet’s permissions do not apply to the CometWrapper. Thus, managers cannot use the wrapper contract on behalf of the users.
Security Model and Trust Assumptions
The CometWrapper contract acts as a wrapper around the Comet and involves no privileged functionality. Users rely on the Compound DAO to manage both the market and the wrapper, with future upgrades and functionality determined through the usual DAO governance process.
High Severity
Anyone Can Withdraw or Redeem Shares, Returning Assets to Owner
The withdraw and redeem functions of the CometWrapper contract can be used to burn the shares of an account and return the assets to its owner. However, these functions can be used by anyone, redeeming the shares and withdrawing the assets to the owner’s account, without the owner’s authorization.
Any malicious user could call the functions, passing in any owner account that has shares, specifying the receiver as the owner. The spendAllowanceInternal function would return successfully since owner == spender, allowing anyone to unwrap another account’s wrapped Comet tokens. A similar issue exists in the transferFrom function, allowing anyone to execute transactions that transfer shares from an account to themselves.
Consider setting the spender parameter of the spendAllowanceInternal to msg.sender instead of receiver.
Update: Resolved in commit cfea9b0.
Medium Severity
Mismatch Between RewardConfig Interface and Implementation
The RewardConfig struct defined in the IRewardConfig interface is outdated. As defined in CometRewards, the RewardConfig struct now includes an additional uint256 multiplier variable. The CometRewards contracts deployed on Arbitrum, Optimism, Base, and Scroll are deployed with this additional variable. The absence of the multiplier factor can lead to incorrect reward calculations in getRewardsOwedInteral for any Comets that use a different multiplier than 1e18.
Consider updating the CometWrapper implementation to incorporate a multiplier in the RewardConfig struct to ensure compatibility with underlying Comets that use multipliers other than 1e18 in reward calculations.
_Update: Resolved in commit cfea9b0 in line 9 of the ICometRewards contract, and line 325 of the CometWrapper.
Low Severity
Commented Code
In the CometWrapper contract, line 615 contains commented-out code.
Consider removing the commented line of code to improve the readability and clarity of the codebase.
Update: Resolved in commit cfea9b0.
Incorrect Validation of CometRewards
In line 94 of the CometWrapper contract, a call is made to the CometRewards contract but its return value is never checked. This call will not revert if the recipient address is an EOA.
Consider checking that the reward token in the returned RewardConfig is not the zero address to ensure that it is the CometRewards contract.
Update: Resolved in commit cfea9b0 in line 92.
Incorrect EIP-2612 Implementation
EIP-2612 defines a standardized way to allow users to provide token spend allowances gaslessly. The Comet Wrapper contract has implemented EIP-2612 but it deviates from the standard in three ways:
- The
permitfunction in the Comet Wrapper contract usesAUTHORIZATION_TYPEHASHinstead of thePERMIT_TYPEHASHdefined in EIP-2612. - The
permitfunction uses different names for the parameters than the ones defined in the EIP. - The EIP states that a signature should be accepted as long as
block.timestamp <= deadlinebut the implementation of this function in the wrapper rejects a signature whenblock.timestamp == deadline.
Consider making changes to comply with EIP-2612. This will help increase the ease of integration of the wrapper token.
Update: Resolved in commits d55ff88 in line 34 and 8d208ed in line 566.
Duplication of Code
The spendAllowanceInternal function essentially has the same functionality as the inherited _spendAllowance function.
The only difference between the two functions is that spendAllowanceInternal checks whether the owner of the tokens is also the spender. If that is the case, it returns without making any changes to the allowances. This extra check is not needed if msg.sender is passed as the spender and the spendAllowance function is called directly.
Consider using the spendAllowance function directly to reduce code complexity and improve readability.
Update: Resolved in commit cfea9b0.
Deviation From EIP-4626
The CometWrapper contract implements ERC-4626 functionality for Compound’s Comet protocol. However, its implementation of the maxDeposit, maxMint, and maxRedeem functions does not align with the EIP-4626 standard’s expectation that these functions should return the maximum amount that can be deposited, minted, or redeemed under existing conditions. Currently, these functions do not consider whether transfers are paused in the Comet contract. This would result in these functions displaying a non-zero value when transfers have been paused in Comet.
Consider adding a check ensuring that transfers are not paused in the above-mentioned functions to maintain compliance with EIP-4626.
Update: Resolved in commit cfea9b0 for all instances in lines 389-393, 395-399, and 401-405.
Incomplete Validation in the isValidSignature Function
The isValidSignature function does not ensure that the data returned from the staticcall is at least 32 bytes long.
Consider ensuring that data is at least 32 bytes long.
Update: Resolved in commit d55ff88 in line 601.
Notes & Additional Information
Users Cannot Claim Rewards Without Accruing Them First
Unlike the claim and claimTo functions in CometRewards, the CometWrapper’s claimTo function does not allow users to choose to accrue rewards before claiming them.
In CometRewards, the claimTo function calls getRewardOwedInternal which, in turn, calls accrueRewards. In accrueRewards, rewards are accrued for the contract by calling comet.accrueAccount(address(this)) and for the user by updating the baseTrackingIndex in the updateTrackingIndex function. However, in CometWrapper’s claimTo function, users do not have the option to specify whether rewards should accrue before being claimed.
Consider aligning CometWrapper with Comet’s standard by adding a shouldAccrue parameter to the claimTo function. If shouldAccrue is set to false, the function should return the current reward based on the existing baseTrackingAccrued of the user without updating it.
_Update: Resolved in commit d55ff88 in line 354.
Unused Constant
In the Wrapper contract, the ENCUMBER_TYPEHASH constant is declared but never used.
In order to improve code clarity and maintainability, consider removing the unused constant.
Update: Resolved in commit cfea9b0.
Conclusion
The recent updates to the CometWrapper contract simplify its interface and address key audit findings. While we identified one high-severity and one medium-severity issue, overall, the changes enhance security and usability, positioning the contract as a valuable tool for the Compound community.