Anyone who has written code before knows that writing bug-free code is no small feat. One of my favorite quotes about this challenge comes from renowned computer scientist Tony Hoare:

There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies, and the other way is to make it so complicated that there are no obvious deficiencies. The first method is far more difficult.

–Tony Hoare, 1980

The Withdraw Pattern: a Case Study

When transferring ether from a smart contract, a common pattern is “the withdraw pattern.” Rather than transfer ether immediately, the ether is made available for future withdrawal by the recipient.

The reason for this pattern is that transfers can fail, and it’s a good example of how to simplify your code to avoid bugs. Consider the bid function of an auction contract that doesn’t use the withdraw pattern:

// Don't do this!functionbid()publicpayable{require(msg.value>highBid);// Return bid to previous high bidder.highBidder.transfer(highBid);highBidder=msg.sender;highBid=msg.value;}

This function is simple, but it has a well-known vulnerability. The high bidder can be a smart contract that refuses incoming ether. This means the transfer call will fail, and the bidder can never be beaten no matter how low their bid.

It’s tempting to fix this by using send, which returns a boolean status rather than reverting on failure. Replacing transfer with send in the previous code would mean a failure is ignored:

// Don't do this either!// Return bid to previous high bidder, ignoring failures.highBidder.send(highBid);

It seems at first like this takes care of the problem, because a high bidder causing a failure is only hurting themselves. However, this is heading down the path of “so complicated that there are no obvious deficiencies.” The code itself isn’t terribly complicated, but reasoning about it is. Someone reading the code needs to consider all the ways send can fail and what happens in those cases.

There’s a flaw in the send version of the code. The recipient of the send isn’t the only one who can cause the transfer to fail. The new bidder can, for example, create a deep call stack before calling bid. When the maximum stack depth is exceeded, the transfer will fail and the previous high bidder will not receive their ether.

Okay, things are getting a bit more complicated. This version detects a failed transfer and makes the funds available for explicit withdrawal by the recipient later:

// Maybe this works?mapping(address=>uint256)balances;functionbid()publicpayable{require(msg.value>highBid);// Return bid to previous high bidder.if(!highBidder.send(highBid)){// If ether wasn't transferred, keep track of it.balances[highBidder]+=highBid;}highBidder=msg.sender;highBid=msg.value;}// This is used to withdraw funds when a transfer fails.functionwithdraw()public{// Follow the Checks-Effects-Interactions pattern.uint256amount=balances[msg.sender];balances[msg.sender]=0;msg.sender.transfer(amount);}

At this point, I believe that the code is correct. Ether is either transferred during the bid process or it’s tracked in balances for later withdraw. I’ve corrected the code, but I’ve made the code more complicated. Not all corrections are equal, and it’s important not to settle for a complicated solution like this.

The following code is simpler. Rather than use two different code paths for success and failure, all transfers happen the same way in the same place:

Less Thinking

Making your code “so simple that there are obviously no deficiencies” is not just about making the code shorter or having fewer functions. Note that the final version of the code above is longer and has more functions than the original. The key is that it’s easier to reason about. I get to think less, which gives me a better chance of getting things right.

A place where we recently got this wrong was in our post on Flipping a Coin in Ethereum. I thought it was interesting that claimTimeout could be called by anyone after the expiration time passed, but an astute reader pointed out that because expiration (at the time) was initially 0, claimTimeout could be called as soon as the contract was created. This would result in the first player losing their ether. The code is now fixed, but it would have been better to just restrict who could call claimTimeout. Then I wouldn’t have to think about the various cases at all.

We did better in our recent post on Vickrey auctions, where we forbade bidding by the seller to avoid unnecessary complications.