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:
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.
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)
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.
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
[x] Code written and tested
[x] PR made for review - PR #152
[x] Changes audited - link will be posted once audit is published
[x] Audit remediations implemented - the PR will be updated with audit remediations once the audit is published
[ ] Create upgrade proposal(s)
We plan on creating proposals to upgrade low TVL cTokens first
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
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.
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.
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?
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?
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
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?
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.
