[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

Hi all - we have re-proposed the cToken upgrade following the completion of the OpenZeppelin audit: Compound

3 Likes

Thanks for all of the effort! This is something I’ve been looking forward to for a while.

I’m thinking, maybe it’s best to only upgrade one or two really small markets - USDP and SUSHI?

Could you please link to OZ’s audit report?

Also, have you written simulations for the governance proposal to ensure the proposal executes correctly and that the upgraded markets all function correctly?

Thanks so much!

1 Like
  • We know that this is a blocking change for a lot of other work so we wanted to get this out a bit quicker - the additional audit by OpenZeppelin gave us more confidence to tackle a few more markets. If the community would rather do fewer markets in the initial proposal we’ll be happy to re-propose

  • We’re requested OZ post the updated audit pointing to our commits, but for now here is the initial report: OpenZeppelin Security Updates for February 2022 - #5 by cylon

  • We do have an internal repo for integration testing our proposals via mainnet forks. It’s not currently public but we can look into making it public Here’s a link to the COMP096 test: governance/CompoundcTokenUpgrades.ts at master · equilibria-xyz/governance · GitHub

3 Likes

I can confirm this new CToken implementation has been audited and all issues have been resolved and/or noted in this report: Compound-TUSD Integration Issue Retrospective - OpenZeppelin blog

Excited to see this move forward after a long wait.

8 Likes

Proposal 101 to upgrade mid-TVL cTokens is up and will start voting tomorrow: Compound

5 Likes

@qlo exciting to see the modern implementation rolling out across additional markets :clap:

1 Like

Hi, what is a sufficient time frame after changing to this new implementation to deem it safe to use in new asset listings? Also, is there an official version of this implementation on test net?

1 Like

Would love for OpenZeppelin to confirm, but the new implementation likely should be the default for all new asset listings; this will reduce discrepancies and future migration work; the protocol is currently on a path to replace all legacy implementations with the new one.

The new implementation has now been audited twice, and is currently live for 5 markets.

+1 to what Robert said - from the Equilibria side we encourage using the new implementation going forward for new markets. For testnet development, we haven’t deployed anything official to testnets yet - is there a specific testnet you’d like a deployment on? We can get that up soon.

2 Likes

Great to see this work and to see RFP’s getting knocked out!

2 Likes

We have deployed implementations to some testnets:

2 Likes

thank you for deploying on testnets!

1 Like

Agreed. The new implementation should be the default for all new asset listings going forward.

3 Likes