Compound-PR599-review.md

Review of Pull Request 599

Performed by OpenZeppelin

Date: October 19th, 2022

We were asked to review the changes made to Comet.sol and Liquidator.sol in commit 3bd3e3b9eb032fc84fd53504ea88e3d98edd0f8c from pull request 599.

Overview

This pull request introduced changes in how the absorbed collateral is represented in the internal accounting system. Before the patch, all absorbed collateral was tracked in the same userCollateral mapping under the address of the Comet contract. However, this new implementation would treat the absorbed collateral as part of the protocol's reserve instead of being tracked in both userCollateral and totalsCollateral mappings. Necessary changes were done to be backward-compatible such as implementing the getCollateralReserves function and replace the withdrawCollateral call with a direct doTransferOut execution in the buyCollateral function.

Findings

Low

L01 - Possible reentrancy when buying collateral

If a user calls the buyCollateral function and passes the address of the Comet contract as the asset parameter, the doTransferOut function would end up executing the transfer function with the Comet address as the msg.sender. While this scenario is highly unlikely, as it would require an error on the collaterals' configuration, it could have severe consequences.

Consider verifying that the asset parameter is not the Comet contract address to prevent this scenario from ever happening.

Update: Acknowledged, not fixed. The Compound Labs team stated:

Since buyCollateral depends on quoteCollateral, which uses getAssetInfoByAddress, this should revert with BadAsset(). If we want to protect against the market listing itself as a collateral asset, this probably isn’t the appropriate place, but should be added to the list of assumptions about collateral assets. Further, even if the market were listed as its own collateral asset, it would not normally have a balance of itself, and so is not obviously problematic in itself. There is another theoretical re-entrancy issue in buyCollateral, which we have also added a comment about to the code, which further restricts the list of supported collateral assets. The protocol should not list any collateral asset with an open pre-transfer hook which could re-enter buyCollateral. ERC777 tokens are not in this category, as the pre-transfer hook is only available to the sender.

L02 - Reserve funds could get soft-stuck during upgrate

The previous version of the Comet contract tracks the protocol absorbed collateral by storing it in the userCollateral[address(this)] mapping. After the upgrade to the new version, this will be replaced by the new function getCollateralReserves. While the current value of it is zero, in the case that there was some absorbed collateral during the deployment of the new version, a non-zero value would be stuck and would require another upgrade to rescue it.

Consider adding a validation mechanism to ensure that the userCollateral[address(this)] is set to zero before the upgrade.

Update: Acknowledged, not fixed. The Compound Labs team stated:

This issue is noted in the PR, with a potential mitigation to add a function which could convert the old-style reserves into new-style reserves, instead of reverting. However, the likelihood of a meaningful absorb call without a corresponding buyCollateral call, right before execution of the proposal, is extremely low*, and such a function could always be added in the event that it did occur. In the worst case, the protocol will hold some collateral as a supplier (with the assets not counted in reserves, nor available for sale), until such a function, or other mitigation is applied.

In the first place, a meaningful account would have to be liquidatable. This can only happen under certain market conditions. Further, the caller of absorb, which currently does not have an incentive beyond its ability to call buyCollateral in the same transaction, would have to forego the profits available by liquidation. The more meaningful the amount of collateral is, the more profit the griefer would lose in opportunity cost, thus we consider this extremely unlikely.

Notes

N01 - Mismatch between function names and their functionality

The function getCollateralReserves returns the amount of collateral that the comet contract holds after it has been absorbed by the protocol. However, the name implies that it is returning the collateral reserves, when it is actually returning the amount of collateral that has been absorbed by the protocol and is no longer being used as a collateral asset.

Consider renaming the function to getAbsorbedReserves or getProtocolReserves to describe more accurately what it is actually returning. In the same direction, consider renaming any other instances such as the function buyCollateral to buyAbsorbedReserves to reflect their current behavior.

Update: Acknowledged, not fixed. The Compound Labs team stated:

The name is meant to signify that it is the amount of reserves of the given (supported) collateral asset, not that the reserves are collateral. We believe the name is in fact exactly corresponding to the functionality.

The name buyCollateral can also be interpreted as applying to the collateral asset.

N02 - Implementation style inconsistency

The operations in the buyCollateral function could be rewritten in a separate function, as the former function used, withdrawCollateral, to improve the consistency of the codebase.

Update: Acknowledged, not fixed. The Compound Labs team stated:

We feel this would add additional complexity, and that the change to doTransferOut actually restores a symmetry and consistency to the buyCollateral function that did not exist in the previous implementation.

Conclusion

No critical or high severity issues were found.