[RFP12 Implementation] cToken Cleanup

Tldr;

The Equilibria Team has implemented various changes to the core cToken contracts as part of RFP12: cToken Cleanup.

Background

The existing cToken contracts are written in Solidity 0.5.x and 0.6.x. While not extremely old, these versions lack some of the newer features present in later versions of Solidity like custom errors, checked math, and gas optimizations. By upgrading these contracts, we can unlock a number of quality of life improvements for developers, decrease gas costs, and position the codebase to be upgraded to newer versions at a more regular cadence.

Proposal

These changes implement the following:

  1. Upgrade the Solidity version of the cToken and related contracts to 0.8.6 - all contracts in the repo were changed to 0.8.6 but only the cTokens will be upgraded as part of upcoming governance proposals. This is due to the complexity of having multiple Solidity versions in the same repo.
  2. Remove the usage of SafeMath and CarefulMath in favor of Solidity 0.8’s checked math - Solidity will now automatically revert when math errors occur (overflows, division by zero, etc)
  3. Remove the custom errorCode return values in favor of reverts and custom errors - this allows for a more structured way to deal with errors rather than enum or string comparisons.

For more detailed code changes, please take a look at the PR which can be viewed here: [RFP12] CToken Cleanup by arjun-io · Pull Request #152 · compound-finance/compound-protocol · GitHub

It is important to note that the goal is to have no behavior changes in the happy path case, and to only move away from errorCodes and to revert in the failure case (both math errors and checks). All existing unit and scenario tests should pass with only changes to the error code cases.

Status

  1. [x] Code written and tested
  2. [x] PR made for review - PR #152
  3. [x] Changes audited - link will be posted once audit is published
  4. [x] Audit remediations implemented - the PR will be updated with audit remediations once the audit is published
  5. [ ] Create upgrade proposal(s)
    • We plan on creating proposals to upgrade low TVL cTokens first
11 Likes

The audit report has been published and can be viewed here: Compound – cToken – Chainsecurity. We have also pushed all audit remediations to the GitHub Pull Request

5 Likes

I am very excited to get this into production.

After reading the (very well written) Chain Security audit, I wanted to highlight point 7.1 in particular since the community has been talking more about asset listings recently.

In the future, new tokens might be added. When markets for those are created, issues can appear. In this non-exhaustive list, we highlight some of those issues:
• On-demand Balance Modification + Callback:
Different token types (inflationary, deflationary, or rebasing) can have balances which change without a Transfer occurring. For some of these tokens there is a permissionless trigger to update everyone’s balances. Tokens with such a permissionless trigger and a callback on transfer should not be added for the following reason. While receiving the callback of mint() the depositor could trigger the balance adjustment and thereby increase the ERC20 balance of the market without making a deposit.
• Blacklist, Freezable, Seizable:
Tokens where some addresses can be blacklisted, certain funds can be frozen or some funds can be seized/burnt, need to be added with great consideration. A blacklisted market would stop working properly. A (partially) frozen market would not function correctly (as the underlying fungibility assumption is violated). Finally, seizing could lead to sudden drops in the exchange rate.
• Transfer Fees:
In principle the protocol supports tokens with transfer fees. However, if a user borrows a certain amount of tokens with transfer fees, it will be almost impossible to completely repay that borrow. This is because the existing feature of providing -1 as the amount wouldn’t work due to the transfer fees. Hence, a small borrow residue will most likely remain.
When borrowing tokens with transfer fees, the requested amount will not be received. Similarly, when reducing the reserve of a token with transfer fees, there will be unexpected losses.
Tokens with potential for sudden increase in value:
If a token whose value can suddenly increase by a significant amount, can be borrowed, then attacks due to extremely bad positions are possible. Such tokens include UniswapV2 and Curve pool tokens, but also DPI tokens. Extreme care has to be taken, when adding such tokens to the protocol as they will most likely lead to an attack.

Inflationary, deflationary, and rebasing tokens pose the most risk to Compound (even with a 0 cf). The protocol should continue to consider adding new assets carefully. One of the first things I check when reviewing new assets is the mint functionality. Either minting needs to be impossible or if possible, it needs to be clearly documented what is possible and two what extent.

5 Likes

Proposal 80 to upgrade cSUSHI has been created!

2 Likes

Just so everyone is aware, Proposal 80 has not been audited by the OpenZeppelin team. I discussed this with @qlo. Since the scope of this proposal is limited to cSUSHI and Chainsecurity has already performed an audit, the risks appear low.

Community voters should proceed at their own discretion knowing OpenZeppelin cannot yet vouch for this proposal’s security although we can say that Chainsecurity auditors have done excellent work in the past. We are planning to perform our own audit of this refactor in the near future before it gets deployed more widely.

2 Likes

Threw in a No vote on this for Polychain. We may as well delay slightly this to let OZ take a look.

Nonetheless, great improvement, and thanks to Equilibria for taking lead on this. Hopefully you guys are getting paid for this work.

1 Like

Shoot, just saw @cylon from OZs comments in the gov chat.

"Yeah, it would take us some time to get it full audited. It’s already received an audit from Chainsecurity and since the rollout is limited to cSUSHI the impact isn’t massive. According to qlo , this is meant to be a limited rollout anyway.

I wanted to make sure people knew OZ hasn’t looked at it yet just in case that was assumed. We do plan to give it a solid look before it rolls out further."

What’s the benefit of rolling it out and then having OZ take a look? The extent to which this improves comp as a product feels so minor that the very slim chance a bug that hurts COMPs rep of some sort probably overweighs the benefit of getting it into prod sooner rather than later?

(Worst case, PC can always void our vote)

1 Like