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
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.
Proposal 80 to upgrade cSUSHI has been created!
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.
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.
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)
Hi all - we have re-proposed the cToken upgrade following the completion of the OpenZeppelin audit: Compound
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!
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 publicHere’s a link to the COMP096 test: governance/CompoundcTokenUpgrades.ts at master · equilibria-xyz/governance · GitHub
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.
Proposal 101 to upgrade mid-TVL cTokens is up and will start voting tomorrow: Compound
@qlo exciting to see the modern implementation rolling out across additional markets
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.
Great to see this work and to see RFP’s getting knocked out!
thank you for deploying on testnets!
Agreed. The new implementation should be the default for all new asset listings going forward.