In response to the comp distribution bug that has been recently exploited, I think it’s important we respond to this with a better process in reviewing large code changes before implementation. We are very lucky that this bug was only a bug that distributed more comp to other people and didn’t threaten the protocol.
The bug was exploited after proposal 62 was passed. Looking at the discussion thread for this proposal, there was not much testing outside of what TylerEther tested on his own as well as a deployment on the Ropsten testnet to check that it worked correctly, which only a few people tested.
One idea we should absolutely be doing is implementing large code changes on a copy of the current state of the Ethereum blockchain and observing the changes made to peoples addresses. We also need to encourage more people to help test these changes before it gets deployed. We shouldn’t be voting yes on proposals that make big changes unless its been through rigorous testing.
Another idea to avoid something like this from happening again is that we could give the multi sig the power to revert a proposal. Giving the multi sig this ability will be for emergencies that require quick responses, like the current comp distribution bug.
I want to invite others to continue this thread with ideas to improve our methods of testing in order to prevent a bug like this to occur again.
Completely agree with threading ideas here! I believe that we should be creating some form of a Compound Audit DAO (a group of 5-7 smart contract auditors) that is tasked and compensated for executing all audits. In this way, we can add more structure to the proposal review process and within the voting delay period the audit team can take a look at all protocol changes alongside @phazejeff’s aforementioned simulation to significantly reduce the likelihood that a similar scenario doesn’t occur in the future.
+1 to the idea of emergency revert, regardless of the complexity of the changes.
Quick thoughts on testing, more thorough ones to follow. The question that should be asked is what could have been reasonably done to catch the issue.
Perhaps a testnet + QA process for a period of time before deploying sufficiently complex code changes would make a difference in this case? Some kind of simulation of a deployment on real data may have also made this issue apparent.
Over at idle.finance we have developed a tool for simulating proposals built on a compound governance like system, and we have used it to simulate test and debug complicated proposals before going on-chain. It was is built as a plugin for hardhat, but currently it only works for GovernorAlpha proposals, but it could easily be extended to also support GovernorBravo proposals as well.
I also noticed that prop 62 was many different changes rolled into one. It seems like this may have increased the complexity unnecessarily so it may be worthwhile to question the necessity of bundling change up like that, especially if a quick revert is implemented, you’ll appreciate the granularity.
What would help catch bugs like this in the future is using test driven development methodology. Doesn’t have to be purest; but, what you want is 100% code coverage ideally. According to DeFi Safety we only have 52%. How TDD works is before writing your code you write tests for what you want the code to do, and then you test it during development. You start off with extremely simple code that solves the first test, and then add additional tests for each behavior the function should have. It would catch bugs like this almost immediately.
Also has the added benefit of producing simpler code, and speeds up later development. The initial development just takes longer. But, you end up with a much more robust code base.
Test driven development would do this too. If not using TDD, at least require code to have 100% branch coverage. I think 70% to 90% is the normal benchmark sought. But, since this involves potentially billions of dollars 100% might be a good idea.
I want to echo ElPro’s sentiments here. He (like myself) is a voting delegate over at Maker.
We have a strong interest in a healthy DeFi ecosystem, and if there is a way that we can be helpful as you build out and ruggedize your processes, please do not hesitate to reach out.
Compound was built on being a stable, safe, error-free protocol. Over the last dozen proposals, bugs (of increasing consequence) have made their way into the protocol, culminating in Proposal 62, which distributed COMP incorrectly.
First off, I completely understand the mindset from the community that we need a rigorous process for evaluating proposals. Historically, all changes to the protocol were audited by Open Zeppelin and / or Trail of Bits, included complete code coverage and invariant testing (formal proofs), and were manually QA’ed on test-nets before any patches were pushed to production / governance. As more community members contribute patches, the process has become more ad hoc. This will be the case in any truly decentralized community, but we should ask ourselves what process we think makes sense to keep the protocol safe, and use Proposal 62 to immediately re-calibrate. We can use this framework to collectively evaluate if patches and changes have met that bar, a priori of discussing the deeper implications of those changes.
First, we should become a “default no” community instead of “default yes.” Patches are not considered safe until we (the community) collectively decide so. If a patch is put up too early, it’s fine to vote against it and ask for more checks and assurances from the developers or other community members.
Less is more. Patches should cover the smallest ground possible. Don’t refactor module X and change its behavior at the same time. This often leads to subtleties that are hard to evaluate and discover. Refactor a module with no changed behavior and then make your patch as a follow-up. Smaller changes are easier to evaluate and will help get a patch through significantly faster than grouping several changes together.
Testing should be crucial to any patch. This should include standard tests, “scenario” tests, and, if possible, invariant testing. Patches should be evaluated on test-net before being proposed to main-net. Every piece of evidence that the patch is safe should be evaluated and understood, with a special emphasis on looking for blind spots where there is no coverage (not just in line counts, but in actual real-life scenarios).
Communicate! Talk on the forums about your patch; ask other members in Discord; post it in draft form to a GitHub pull request. Get the juices flowing early so people can give directional signals and help make sure the best decisions are being made.
All patches should be audited. I understand that it can be difficult to coordinate an audit in a decentralized environment. The protocol could specifically reserve the time of an auditor in advance, or proposers could look to repay auditors as part of the proposal. Ideally, the protocol could retain enough time with an auditor which could be used flexibly, as proposals come up.
The protocol should offer a standing bounty to anyone who finds a vulnerability with a pending proposal. Implementors should have some amount of their reward escrowed for a period to evaluate that the patch works as intended before being released to the contributor.
The community should consider establishing a technical committee to establish development standards; review, and approve changes before they are proposed to governance.
Proposal 62 was a catalyst for us to improve, and build a world-class decentralized development process. The community will have to find the long-term balance of security and risk tolerance, but I know we can use this to get on the right track.