Comet Wrapper Audit

Protocol Development
1

OpenZeppelin, in its role as Security Partner to the Compound DAO, audited an ERC-4626-compliant wrapper contract for the Comet token commissioned by the community through the Compound Grants Program.

Summary

Timeline: From 2023-10-30 – To 2023-11-10

Total Issues: 16 (0 resolved)

Critical Severity Issues: 1 (0 resolved)

Medium Severity Issues: 2 (0 resolved)

Low Severity Issues: 6 (0 resolved)

Notes & Additional Information: 7 (0 resolved)

Scope

We audited the compound-finance/comet-wrapper repository at the 1c08f3f commit.

In scope were the following contracts:

src
├── CometHelpers.sol
├── CometWrapper.sol
└── vendor
    ├── CometExtInterface.sol
    ├── CometInterface.sol
    ├── CometMainIntefrace.sol
    ├── CometMath.sol
    ├── ICometConfigurator.sol
    ├── ICometRewards.sol
    └── IERC7246.sol

System Overview

A Comet market is a rebasing, ERC-20-compliant contract with user balances adjusted by the corresponding indexes tracking the overall state of supplying and borrowing. The rebasing nature of the Comet token may cause issues when integrating into the wider Defi ecosystem. The community commissioned, through their grants program, an ERC-4626-compliant wrapper contract which allows users to have a non-rebased balance of shares that correspond to the principal balance of the corresponding Comet market.

The CometWrapper conforms to the proposed EIP-7246 Encumber Interface. Backwards-compatible with ERC-20, EIP-7246 allows a user to encumber tokens to other addresses which cannot be revoked. This is motivated by the often necessary need to know that an amount of tokens is available to take with the ability to claim them at a future point when specific conditions arise. The CometWrapper thus conforms to ERC-20, ERC-4626, and EIP-7246.

This may lead to unanticipated behavior for users. An example is the convention of using balanceOf or allowance in ERC-20 contracts. Unscrupulous users who call these functions may be misled into using those values when the new ‘availableBalanceOf’ and encumbrances should be used instead. Users and developers should review the external/public functions carefully to ensure they are using the contract correctly. Of course, should EIP-7246 become more broadly used, its conventions and norms will be better understood and practiced.

Like a Comet market, the proposed CometWrapper allows users to accrue rewards.

Security Model and Trust Assumptions

Unlike Comet, there are no privileged functions. The usual governor or pause guardian has no special functions they can invoke. Since it is a wrapper of a Comet market contract, users are taking on the assumption that the Compound DAO will manage the market and the wrapper. Wrappers, like markets, are meant to be upgradeable, so future functionality will be determined by the usual DAO proposal process.

Critical Severity

Encumbered Amount Can Be Withdrawn by Self

The CometWrapper implements the IERC7246 Encumber Interface, thus the shares in the CometWrapper can be encumbered to other accounts. According to the specification, the encumbered amount cannot be accessed by the token holder until it is released to guarantee token availability to the encumbered.

However, an account can withdraw its entire balance without any consideration of encumbered amount via the withdraw and redeem functions if the caller is also the owner.

This has some consequences to other functions, particularly, any call to availableBalanceOf can underflow if the outstanding encumbered amount is more than the balance available. Any calls to availableBalanceOf will revert, for instance,
encumberInternal, transfer, and spendEncumbranceThenAllowanceInternal. Thus, the core mechanism for encumbering will not work for accounts with higher encumbered amount than the actual balance.

This also means that by EIP-4626, the maxWithdraw and maxRedeem functions should return the availableBalanceOf instead of the actual balanceOf.

Consider only allowing the withdrawal of availableBalanceOf by the owner and also taking into account any outstanding encumbered amount in the max limit for maxWithdraw and maxRedeem.

Medium Severity

Underflows in previewWithdraw and previewRedeem

In previewWithdraw, if the argument assets is greater than totalAssets(), this line will revert due to underflow. This can happen to any positive assets value when totalAssets() is zero.

According to the ERC-4626 specification, previewWithdraw should return a value in such a case without reverting due to insufficient shares/assets, as quoted from the specification, previewWithdraw

MUST NOT account for withdrawal limits like those returned from maxWithdraw and should always act as though the withdrawal would be accepted, regardless if the user has enough shares, etc.

An underflow can also happen in previewRedeem, where the totalSupply() is used.

Consider removing the lines with potential underflows and allowing successful previews without reverting.

Inconsistent Interpretation of type(uint256).max

When depositing with assets = type(uint256).max in CometWrapper, this will be interpreted in Comet as a request to transfer the entire balance (i.e., Comet.balanceOf(msg.sender)) instead of the literal max uint256 value. Thus, the transfer will be successful with the user’s entire balance.

However this interpretation is not considered in the wrapper, instead, the parameter asset is used for computing shares. This will make the calculation within the call to previewDeposit always revert due to an overflow.

Consider either using a transferFrom mechanism consistent with Comet or documenting this discrepancy explicitly for all relevant functions. Also consider adding explicit errors for potential under/overflows for better user/developer experience.

Low Severity

Max Deposit Limit Does Not Conform to EIP-4626 Specification

The functions maxDeposit and maxMint return type(uint256).max; however, a user cannot deposit a max value without reverting.

There is an implicit upper limit on principal of type(uint104).max from Comet enforced in the safe casting from the
previewDeposit call. Asset values above it will revert. Thus the returned amount from both maxDeposit and maxMint will always cause reverts due to overflow in casting.

The EIP-4626 specification requires that maxDeposit, like maxMint,

MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

Consider modifying the limits to reflect the maximum values correctly.

Comet Transfer Flag Is Not Considered In Preview Functions

Both the Governor and PauseGuardian roles can pause transfer actions within a Comet market. The corresponding CometWrapper must respect whether transfer actions are paused. According to the EIP-4626 specification, the previewDeposit, previewMint, previewWithdraw and previewRedeem functions should reflect the on-chain conditions at the current block, for instance, previewDeposit:

Allows an on-chain or off-chain user to simulate the effects of their deposit at the current block, given current on-chain conditions.

Consider including a check on the transfer pause flag in the preview functions to reflect the on-chain condition of the underlying Comet market.

Inconsistent Imports From Inconsistent Interfaces

The interfaces CometMainInterface and CometExtInterface defined in this codebase are different from those in Comet. For instance CometExtInterface contains the TotalsBasic struct while the cometWrapper CometExtInterface does not.

This is made more confusing with non-explicit imports. Within CometInterface.sol, global imports are being used. For instance:

But in the main CometWrapper.sol, explicit imports are used.

This may create confusion for developers and readers who are familiar with the comet interfaces. Consider using consistent named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.

Unnecessary Boolean Comparison

Within the isValidSignature method is a check to see if a boolean variable is equal to false. Such comparisons can obfuscate the code’s intention, and use of boolean literals is a common code smell. Consider removing the boolean literal so the check is just on the variable itself (e.g., if(!success) ...).

Solidity Version May Be Incompatible With L2 Chains

The pragma for these contracts is set to Solidity 0.8.21. Version 0.8.20 introduced the new PUSH0 opcode that is not supported by all L2 chains. It is supported by Polygon but not by Arbitrum or Base. When compiling the contracts, be sure to select the correct EVM version for the target network (shanghai being the EVM that supports PUSH0) or consider downgrading all contracts to version 0.8.19.

Potential Locked Tokens

Comet has an
approveThis function which allows it to approve users to transfer tokens away from the contract. Thus any manually transferred tokens to the comet market can be accessed. However CometWrapper has no such functionality and as such, errantly transferred tokens can be locked in the contract.

Consider adding such a function if the protocol would like to have control over locked assets.

Notes & Additional Information

Typographical Error

accruedInterestedIndices on line 373 of CometWrapper should read accruedInterestIndices.

Multiple Instances of Missing Named Parameters in Mappings

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of the mapping’s purpose.

Within CometWrapper.sol, there are multiple mappings without named parameters:

Consider adding named parameters to the mappings to improve the readability and maintainability of the code.

Lack of Security Contact

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice proves beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the maintainers of those libraries to make contact with the appropriate person about the problem and provide mitigation instructions.

Consider adding a NatSpec comment to the CometWrapper contract containing a security contact on top of the contract’s definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Missing Docstrings

Throughout the codebase, there are several parts that do not have docstrings. See CometExtInterface.sol, CometInterface.sol, CometMainInterface.sol, ICometConfigurator.sol, and ICometRewards.sol in the src/vendor directory. Considering these are different from the usual Comet interfaces, it is helpful to spell out the differences for developers and readers.

Potential Unsafe ABI Encoding

It is common practice to use abi.encodeWithSignature or abi.encodeWithSelector to generate calldata for a low-level call. However, the first option is not typo-safe and the second option is not type-safe. The result is that both of these methods are error-prone and should be considered unsafe.

On line 717 of CometWrapper.sol an unsafe ABI encoding is used.

Consider replacing the occurrences of unsafe ABI encodings with abi.encodeCall, which checks whether the supplied values actually match the types expected by the called function and also avoids errors caused by typos.

Floating Pragma

Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.

The file IERC7246.sol has the solidity ^0.8.21 floating pragma directive.

Consider adopting a fixed pragma version.

Inconsistent Use Of Named Returns

To improve the readability of the contract, use the same return style in all of its functions.

CometWrapper’s getSupplyIndices function is the only function to use named return variables.

Consider using a consistent style for function returns.

Conclusion

This audit examined a new feature of Compound Comet markets offering users a non-rebased balance of shares for convenience and compatibility with the wider Defi ecosystem. At the start of this audit, we were very pleased, again, to see a robust testing suite alongside the scope we were given. We expect to see iterations of the contract as we have received it and look forward to the utility it will provide to the community and DeFi generally.

2 Likes