[RFP12 Implementation] cToken Cleanup

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

Hi all! The final upgrade proposal is up and will start voting soon :grin:

Our integration test to verify the proposal can be viewed here

1 Like

From what I tell by clicking on the link for RFP12, RFP12 was $250,000, but the current proposal is requesting $350,000. “In addition to updating the implementation, it grants $350,000[1] worth of COMP to Equilibria for the upgrade work and 79,764.36 USDC to Compound Labs for the ChainSecrity audit of the protocol[2].”

Did the work completed also cover RFP13 or 14? Or is there perhaps another explanation (or perhaps, other discussion/thread) that explains the difference in the amounts? Thanks for any clarification.

Yes! Sorry for the confusion - we should have mentioned in earlier posts that the upgrade also has some gas savings, about 8k total across the main mint/redeem/borrow flows. So after discussing with the Compound Labs team we thought it was fair to also claim RFP14. You can see the gas savings from the gas profiler here

4 Likes

Hi all - we’re super happy to announce that this upgrade has been executed and live for the past two days! This concludes Equilibria’s work for RFP12 and 14 - we look forward to more productive community building :slight_smile:

4 Likes