Date: December 27, 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.
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.
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.
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.
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.
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.
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.
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.
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.
No critical or high severity issues were found.