A Tale of Little Bugs

As most programmers would admit, the most annoying bugs are often the "little" ones. Tiny logic errors caused by a few wrong characters in a single line of code, compiling fine and remaining undetected, patiently waiting to crash our program at the worst possible moment. We've all written such bugs, spent countless hours debugging them, and uttered the most horrific profanities when we finally discovered that we lost our sleep over a couple of wrong characters.

But losing a night's sleep over a little bug isn't the worst of our worries. At least not if one writes software for NASA, whose Mars Climate Orbiter famously burned up in the Martian atmosphere due to a software bug. Well, NASA software is complex; such a catastrophic bug should clearly be complicated, impossible to understand by mere mortals, right? Far from it, the bug that led to the loss of the $125 million Mars Climate Orbiter was a trivial but crucial missing multiplication by 4.45. Europeans aren't immune to little bugs either; the loss of ESA's $370 million Ariane V rocket in just 39 seconds was caused but a simple integer overflow error.

Thankfully, for the longest time, one needed to be employed by a space agency to worry about a little bug having such enormous financial consequences. That is until Smart Contracts arrived! Now, programs consisting of a few hundred lines of relatively "simple" code, developed by small teams over a relatively short period of time, are directly responsible for safeguarding various types of multi-million-dollar assets. All it takes is one undetected little bug and we get, not a spectacular rocket explosion, but an equally spectacular crypto hack that makes the Mars Climate Orbiter seem like pocket change.

So, let's look at an instructive example of such a little bug. Smart Contracts typically use Solidity modifiers to guard their functions, performing crucial security checks.

modifier isOwner() {
    // Make sure we're called by our trusted owner before doing anything.
    require(msg.sender == owner, "Caller is not owner");
    _;
}

Writing such a check is simple, no need to be a NASA engineer to do it. But better double and triple-check it because the consequences of the tiniest of bugs in that line are enormous.

Now, require has been used in solidity modifiers for years, but it has an annoying flaw: using string errors, like "Caller is not owner" above, is not gas efficient. Instead, for some time now, solidity has supported gas-efficient custom errors, which, however, are not allowed in require.

error CallerNotOwner();  // gas efficient and easy to recognize

modifier isOwner() {
    // I wish this were valid code, but it isn't.
    require(msg.sender == owner, CallerNotOwner());
    _;
}

Not a big deal, you'll say, require is just a combination of a check and a revert; we can rewrite it and perform the two steps manually.

modifier isOwner() {
    // This works fine
    if(msg.sender != owner)
        revert CallerNotOwner();
    _;
}

Mission accomplished, but you might have noticed a small detail. In the code above msg.sender == owner was replaced by its negation: msg.sender != owner. This is because require expects a condition that should hold, while its if/revert replacement expects a condition that should not. So, in general, we should replace

require(some_complicated_expression, "my error");

by

if(!some_complicated_expression)
    revert MyError();

This negation of the Boolean expression is exactly the beginning of our "little bugs" story. Well, how hard is it to simply add a "!"? But that's not exactly what we did above, is it? No programmer that appreciates code simplicity and elegance writes

if(!(msg.sender == owner))

Everyone would simplify it to

if(msg.sender != owner)

bringing the negation inside the Boolean expression. And what if the negated expression is more complex? Logic, being the foundation of computer science, provides us with simple rules:

!(A && B)  is equivalent to  (!A || !B)
!(A || B)  is equivalent to  (!A && !B)

Just carefully follow the rules inside the complex Boolean expression, and you'll be fine. Easier said than done; I bet every single programmer with a few years of experience has incorrectly negated a Boolean formula at some point in their career.

So, it shouldn't be surprising that this exact bug appeared in one of our recent audits. The above commit aimed at replacing a string error with a custom one and, in doing so, changed:

modifier onlyOwnerOrUpdater() {
    require(
        owner() == _msgSender() ||
        (updater != address(0) && _msgSender() == address(this)),
        "NetworkRegistry: !owner || !updater"
    );
    _;
}

to

modifier onlyOwnerOrUpdater() {
    if (_msgSender() != owner() &&
        (updater == address(0) && _msgSender() != address(this)))
        revert NetworkRegistry__OnlyOwnerOrUpdater();
    _;
}

Did you spot the negation error? The expression is of the form A || (B && C), so its negation becomes !A && (!B || !C), the && in B && C should change to ||. So, the correct check should be

if (_msgSender() != owner() &&
    (updater == address(0) || _msgSender() != address(this)))

These two wrong characters (&& instead of ||) completely change the logic of the modifier; now an unauthorized call with updater != address(0) and _msgSender() != address(this)) will not trigger the error as it should, which could easily lead to a total loss of funds for this specific contract.

Of course, the point is not that Smart Contracts are impossible to secure: this bug was caught by the audit (the chances of catching it were very high), and even if it weren't, we are confident that it would still have been found before releasing the code, either by manual inspection or automated tests.

But its mere existence shows that Smart Contracts, as with all programs, are not immune to little bugs. Even the simplest of changes require caution and should be properly tested and audited, both internally and by external teams, to minimize the chances of a catastrophic little bug as much as possible.

Share This