More Rigorous Process On Reviewing Large Code Changes (RE: Comp Bug 9/29/21)

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.

Excited to discuss any of the above.

~~ Geoff

12 Likes