Date: January 3, 2022
We were asked to review the Comet repository on branch mainnet-eth
at commit 4db8aca43bc1f48c3666df0fe29ca5e9f012fe43. As we conducted a previous code review in pull request 587 at commit cc7e500e0e69e3d3ef5bb9c1b16dd062cd292427, this engagement focused on the changes from commits cc7e500e0e69e3d3ef5bb9c1b16dd062cd292427 and 4db8aca43bc1f48c3666df0fe29ca5e9f012fe43 for the following files in scope:
BaseBulker.solMainnetBulker.solLiquidator.solComet.solCometCore.solConstantPriceFeed.solIERC20NonStandard.solIPriceFeed.solScalingPriceFeed.solWstETHPriceFeed.sol
Between both commits several changes have been made in the codebase. The most relevants are the addition of more documentation, the refactor of certain parts of the code, fixes from a previous engagement, the possibility to choose a different exchange when liquidating assets, the possibility to use non-standard ERC20 tokens in the protocol, and the construction of a constant and a scaled price feed.
Update: The Compound team applied several fixes based on our recommendations. We have reviewed and addressed each fix submitted by the team and updated the report accordingly. Moreover, as the Liquidator contract is still work-in-progress, any recommendations will be taken into account for a future version of such contract. Fixes for such contract were not included in this update. We suggest a second review of the Liquidator contract once the new changes and fixes have been applied.
In the BaseBulker contract, the transferAdmin function does not validate if the new admin address is non-zero address. It is unclear whether this is a conscious decision so that the ownership can be renounced or not.
Consider documenting if this is intended to be used for renouncing the ownership. Otherwise, consider adding validation to ensure that the new admin address is non-zero address.
Update: Resolved in commit 00cf59d89a2d834c332928cd96f73e05754f761a from pull request 641.
In the ConstantPriceFeed contract, the latestRoundData function returns 0 for the roundId and answeredInRound returned values.
However, the Chainlink documentation states that the roundId should start at 1. This could lead to unnecessary confusion or unexpected behavior in the future for users or other protocols that make use of this contract.
Consider setting as 1 the return value for roundId and answeredInRound to comply with the Chainlink specification.
Update: Resolved in commit d7ab1dd12be37efe5988a3fa8214cfd748f9b2c7 from pull request 641.
In the Liquidator contract, the admin and the recipient addresses are set on deployment. However, once those are set, they cannot be updated. The operator would not be able to update either address and would be forced to deploy a new contract.
Consider using the OpenZeppelin's Ownable library for management of the admin role and adding an admin-allowed function to update the recipient address.
Update: Acknowledged, will resolve. The Compound team stated:
In the latest version (
OnChainLiquidator.sol), the admin role is eliminated and the profits of liquidation are returned to msg.sender.
Throughout the codebase there are instances of confusing, incomplete, or inconsistent inline documentation that should be fixed. The following are instances of confusing inline documentation:
- In line 6 from
IERC20NonStandard.sol, the documentation states that it is a "Version of ERC20 with no return values fortransferandtransferFrom", but theapprovefunction is also modified from theEIP20 - Missing documentation explaining the overall details of the
ConstantPriceFeedandScalingPriceFeedcontracts - In line 69 from
WstETHPriceFeed.sol, the documentation states "Assumes the stETH price feed has an equal or larger amount of decimals than this price feed", but this is enforced by the validation done in line 42, which prevents having something bigger than that - Missing documentation for all the variables inside the
PoolConfigstruct
Clear inline documentation is fundamental for outlining the intended behavior of the code. Mismatches between the inline documentation and the implementation can lead to serious misconceptions about how the system is expected to behave.
Consider fixing these errors to avoid confusion for developers, users, and auditors alike.
Update: Resolved in commit 89f6b166fe3e3a2095da666996d1f6f522700a0c from pull request 641.
When a position in Comet becomes absorbable by the protocol, the Comet contract assigns "liquidator points" to the absorber address, which is passed as input.
However, because the Liquidator contract passes its address as the absorber, this could cause a few problems:
- The
Liquidatordoes not have any method to handle "liquidation points" once Compound implements the feature - If the
Liquidatorcontract becomes deprecated, the points held by theCometcontract for it cannot be transferred and will be lost - If collateral purchases are paused while absorptions are still allowed, the
Liquidatorcontract will not be able to absorb collateral to collect those points (even though it may not be desired to purchase collateral) as the encapsulation of both operations would revert the transaction. The admin must manually absorb the assets, but these points will not be included with theLiquidator's points
To ensure that the liquidation points are used correctly in the future, consider passing the Liquidator admin's address or the msg.sender when calling absorb in the initFlash function. Alternatively, you might consider using a proxy contract behind the Liquidator contract to extend the contract's features.
Update: Acknowledged, will resolve. The Compound team stated:
Great point. We will update
OnChainLiquidator.solto accrue liquidator points to msg.sender when absorbing, so that the caller can redeem the liquidator points in the future.
The Liquidator.sol file contains the IUniswapV2Router interface and the Liquidator contract.
Consider moving the interface to another file and then importing it to the Liquidator contract, to maintain the best practice of using a single interface or contract per file.
Update: Acknowledged, will resolve. The Compound team stated:
The same issue exists in
OnChainLiquidator.sol. We will move interfaces into separate files.
Throughout the codebase, there are cases of inconsistent coding style or possible improvements to it. In particular:
- Even though it was implemented the possibility to scale up/down in the
ScalingPriceFeedcontract, for theWstETHPriceFeedcontract it was only allowed to down-scale the output. - Internal functions in the
Liquidatorcontract are written before external functions - Constructor parameters in the
ScalingPriceFeedcontract have underscore post-fixes, while parameters from theLiquidatorcontract have underscore pre-fixes
Consider fixing and keeping a consistent coding style throughout the codebase to improve the code's readability.
Update: Resolved for the WstETHPriceFeed contract. Acknowledged, will resolve for the Liquidator contract. A detailed reasons of the lack of dual scaling features in the WstETHPriceFeed contract was given. We suggest documenting it along with the contract's documentation. The Compound team stated:
ScalingPriceFeedis a generic contract that allows for scaling in both directions.WstETHPriceFeedis a more specific contract that converts the price from a stETH Chainlink price feed (18 decimals) to a price that Comet supports (<= 18 decimals), meaning only down-scaling needs to be supported. TheLiquidatorcontracts are still being worked on and are out of scope for this audit, but we will definitely take this suggestion into consideration when making updates toOnChainLiquidator. The underscore postfixes inScalingPriceFeedare consistent with the style of the other core Comet contracts (e.g.Configurator). Same comment regardingLiquidatorcontracts being out of scope.
There are multiple "TODO" comments in the codebase (written as "XXX"). In particular, lines 64, 272, and 320 from Liquidator.sol.
Consider removing those commets and instead track those issues in the project backlog.
Update: Acknowledged, will resolve. The Compound team stated:
We will remove all the TODOs once the liquidator contract is in its final state.
No critical or high severity issues were found.