A good code review isn’t just a read-through of the code, plus some kind of unattainable genius. It’s about finding ways to repeatedly think through each aspect of the system, building a mental model of the code and then using that process to find stupid bugs, classic bugs, and deep bugs.
Good code review works from many different perspectives and switches repeatedly back and forth from high levels to the tiniest details.
I’ve been focused on smart contract security for three years now, and code reviews are a huge part of work. This is my system to get the most out them.
Internal Code reviews are still the most efficient way to find bugs. Ideally your unit tests and fork tests can do the easy work of making sure that the code “works”.
Contract security is a gradient, not a boolean. Fuzz testing, invariant testing, and formal proving are all fantastic layers that increase the odds that the code is correct, but the single biggest move up the security gradient comes from a good code review.
Correctness matters incredibly in smart contracts.
Most other industries don’t have tens or hundreds of millions of dollars instantly transferable to a person who finds and exploits a bug.
If you're flying an airplane: yes, the world is random and yes, there's a lot of airplanes, and yes, hardware failures are a problem. But it's not like there's 10,000 guys in Paris, Eastern Europe, North Korea, San Francisco, and the Canadian wilderness reading your source code, with cables plugged into your flight control system and aggressively changing the weather, the clocks, the airport layout, and beaming radiation at your plane in order to put your system into the specific state where the code makes a mistake. Smart contracts live in inherently adversarial environment.
And it’s not just the evil humans - there are all the automated systems lurking out in the dark forest, constantly poking at code, and ready to pounce at the sign of free money.
It is not enough that the code works. It's not enough that it works in the face of randomness. It has to work in the face of an adversarial enemy who is looking for a way to change the world around it to exploit it. The code has to be perfectly correct under all circumstances. Perfect code is hard for humans to get right on the first try.
The second reason that we do code reviews is that we want to keep our code simple.
In the long term, simplicity is the thing that will keep the bugs out and the development velocity up. Every bit of complexity left in the system is a time tax on every bit of future software development we do, and complexity adds risk to everything we build in the future.
Simplicity is not something that comes for free. It’s not something that is easy. You can't just wave a magic wand and make simplicity happen. It's something that takes time, it takes work, and takes some creativity. Simplicity is a multidimensional problem. Sometimes you have to make trade-offs between different approaches to simplicity.
Because simplicity is so hard, we want opinions, feedback, perfection, from multiple people on the team about the code. Simple is often the result of a collaborative process.
If this were a web2 company, nitpicking the tiniest line of code in ways that don’t change behavior would be an anti-social action. Here, in our solidity code, we want to be chasing as much perfection as each of us can possibly bring.
The third reason why we do code reviews is we want to share knowledge around the team.
We have a team of four smart contract devs. When the person who writes the code does a deep review, and then two more people also do a deep review, then three-quarters of our team now deeply understand this new code. When the team deeply understands the codebase, there’s a major difference in both team dynamics and the ability to write code that works with the rest of the system.
These code reviews also share coding techniques and security concerns across the team. And learning goes both ways. I can learn both from what I read in code reviews, and learn from what people find in code reviews on my code.
For example, last week I was reviewing some code and I saw the way the developer handled the need to get information down from a higher level in an inheritance hierarchy to lower base code. Later that week I used what I learned when looking at handling different kinds of Curve pools from strategies sharing the same base code.
A code review is not reading the code through once, commenting on what you see, and then calling it approved.
Let’s step back for a second and look what we we are trying to do. We are trying to find all the bugs. I like to think of bugs as being in three categories.
The core problem is how can you find the deep, subtle, tricky bugs. This often requires building a better mental model of the code in your head than even the person writing the system had. How do you get to that, particularly from a cold start?
The solution is to make repeated loops through the code. Many, many, many passes over the code each time looking for a different problem, and each time building up a clearer picture, a richer mental model of the code inside your head. It's really in the mental representation that you find the really hard bugs.
So the goal is:
The first thing I do is have a first-pass read-through. This is to get a feel for the overall structure of the code so that I can navigate it well on later passes.
Now I love paper for reviews. So I print code off, often removing code comments for the first read-through.
I then write down on my paper any questions I have, anything that looks bad or looks ugly, and ways I think the code might be broken. I’m often wrong about these things - I’m a bit of an optimist about things being broken.
After I finish my first read-through, I work back through my comments and verify that they actually are issues. I’ll go ahead and write the real ones into PR comments at this point. (Getting down to some detailed work here is another step in building my knowledge of the system.)
The rest of my process works off a checklist document of things to consider / check. The great thing is that this forces you to think about the system from different aspects. And when just reading code, it’s easy to forget to look for the things that aren’t there. Checklists help with seeing these issues of absence.
Our checklist is specific to our own codebase and style of code we want.
As you go through using the checklist, your knowledge of the system builds, stupid and classic bugs get spotted.
Some of the things in our own checklist are not actually things that must be true in order for the code to pass review. They're things that if they're not true then we need to examine very very carefully and beware of danger. For example, we have a checklist item for “does not use raw Ethereum” and almost all of our contracts do not. But if we have to use raw Ethereum, then we know that we need to pay particular attention to the possible dangers, and really, really think about what can go wrong with this.
Checklist are the best way to find the common bugs in your code.
Then next approach is thinking about the invariants in the system.
Invariants are things that must always be true for the system to be good. It’s a tremendously useful way of thinking about the system and then checking that it actually always works the way we're expecting it.
You can break them down into “things that must be true before this code runs”, “things that must be true after this code runs”, and “rules about the relationships of different state variables”.
So once I write these invariants down, then I go back through and check the code and verify that they do actually hold.
State invariants are particularly useful - some piece of state must always have a certain relationship to some other piece of state in order for the system to be good. For example, let's say that if you added up the balances of each account it should be equal to the total balance in the system. One good way of building state invariants is to walk through the list of non-config variables in your contract, and for each one, think how it should be related to each other non-config variable.
Invariant-based thinking is probably the most underrated secret weapon of code review and finding bugs.
The next step is to think about attacking the contracts.
Switching to thinking about your code from the point of view of an attacker is a powerful mental perspective shift - as a developer it’s to thinking about how the code will work, rather than what situations you can put it into to break it.
Rather than just “think about attacking the contracts,” the best way I’ve found to do this is to just start writing out attack ideas first without even seeing if they're valid or not. I just spam out things that might be able to go wrong, and their possible worst-case impact.
Then, after I’ve written down a bunch of attacks, I write out why each can't be done, again, all at once without checking the code. As the final step, I then go through and verify, in the code, that what I thought about what would block these attacks are true. The perspective switching from “attack” to “defense” to “attack” thinking, is useful, and in this step, like all other steps, I’m building up my mental model of the code.
Deployment code, configuration, required monitoring, and governance actions are as much of a part of the final system as the code is. Check that everything here is correct, including manually verifying that all addresses are correct.
Lastly, there’s a lot of value in a few minutes of sanity fork testing. It’s amazing what you find just playing with a system. Code is often written to pass the tests, but it may do wrong things when used with different numbers or order of operations than the tests. This also ensures that the deploy and governance action results in a working system.
I usually record the fork testing I do here, clean it up a bit, and then save it to use again later once the code has actually been deployed.
When the first draft of the code has been written, I like to do an informal first-pass review, that looks nothing like this entire process. This lets us make any design corrections before we build out the test suites, and really add the quality to the system.
So our real internal reviews happen after the code is written, and the owner of this set of code is happy with the code and tests. First, the owner does their own review, then tags-in the other two team members for their reviews.
The code owner themselves should always do the first review. They are currently the person who understands this code better than anyone in the entire universe, so by forcing some new perspectives, they’ve got a pretty good shot at finding a bug. Secondly, this lets the owner get some immediate feedback on how good their reviewing is, since if the owner doesn’t catch the bug, hopefully the other two reviewers will.
You want to do our full internal review process before sending the code to auditors. In this way, the external audit is a check on your internal process. If an auditor finds an important bug, then you need to go back and figure out how to change your internal review process so that this kind of bug does not slip through again.
The key to finding the hardest bugs is to look at the system from many different perspectives, and at many different levels. And use a checklist for the easy bugs!
