Chriseth at github casually pointed out a terrible, terrible attack on wallet contracts that I had not considered. If there were a responsible disclosure avenue for ethereum contract developers, I would use it, but there doesn't seem to be. Not only that, this code has been out and published on github for long enough that I wanted to get the news out there quickly.

In Brief: Your smart contract is probably vulnerable to being emptied if you keep track of any sort of user balances and were not very, very careful.

As always, I'm available for smart contract review and audit, email me. You can read about other security considerations on my blog here.

UPDATE: Chriseth notified me that warnings went out to key devs four days ago. He says as written, the attack is less worrisome than I say because by default send() only offers 21k gas, this will not be enough to pay for the nested calls. However, if you use call.value(amt).(), this is still a vulnerability. I am testing a few permutations and will update.

UPDATE 2: In general, this reentrancy bug is widespread, but mitigated in the case of using vanilla send . I still strongly recommend a code review and update for all functions that might not have considered if they will be called repeatedly during execution. Thanks to Nick Johnson and Dennis Peterson for engaging on this as well.

Update 3: I have been contacted by multiple DAO's with real concerns over this bug in the last week, and it appears the TheDAO is not only vulnerable, but that an attacker is draining TheDAO of millions of ether as we speak: discussion on reddit.

The Vulnerability

Here is some code; see if you can find the problem.

function getBalance(address user) constant returns(uint) { return userBalances[user]; } function addToBalance() { userBalances[msg.sender] += msg.amount; } function withdrawBalance() { amountToWithdraw = userBalances[msg.sender]; if (!(msg.sender.call.value(amountToWithdraw)())) { throw; } userBalances[msg.sender] = 0; }

Here's the problem: msg.sender might have a default function that looks like this.

function () { // To be called by a vulnerable contract with a withdraw function. // This will double withdraw. vulnerableContract v; uint times; if (times == 0 && attackModeIsOn) { times = 1; v.withdraw(); } else { times = 0; } }

What happens? The call stack looks like this:

vulnerableContract.withdraw run 1 attacker default function run 1 vulnerableContract.withdraw run 2 attacker default function run 2

Each time, the contract checks that the user's withdrawable balance and sends it out. So, the user will get twice their balance out of the contract.

When the code resolves, the user's balance will be set to 0 however many times the contract was called.

Now, as I've written the attacking wallet code, this (purposely) won't work exactly. There are a few caveats and things to do. But, hopefully this strikes some fear in your heart.

Caveat: send is Accidentally Not Vulnerable To Race-To-Empty

Thanks to the solidity devs for engaging on this immediately, it appears we got lucky with the most common way to send funds. The send function is the typical way to send a contract money. If you've been reading my blog posts, you know that send has some problems. In this case, though, it provides some helpful mitigation because send by default only offers 21,000 gas, not enough to support nested withdraw calls.

So, if you are using only send for all calls, carry on. But, this bug is both nasty and pervasive, I urge you to think through the two approaches below.

Remediation Approach 1: Get Your Ordering Correct

The recommended approach in the soon to be published upgraded solidity examples is to use code like this:

function withdrawBalance() { amountToWithdraw = userBalances[msg.sender]; userBalances[msg.sender] = 0; if (amountToWithdraw > 0) { if (!(msg.sender.send(amountToWithdraw))) { throw; } } }

Note that userBalances is now reset before the send is called. There is nothing wrong with this solution; it works fine. When the race-to-empty wallet calls this function a second time, the code will correctly notice the user has 0 balance.

However, this race-to-empty attach is not the only problem that could arise from a race condition. Without the benefit of a long time to think about, I would propose a mutex is a better solution: this will protect from known and unknown race conditions.

Remediation Approach 2: Mutexes

Consider this code instead.

function withdrawBalance() { if ( withdrawMutex[msg.sender] == true) { throw; } withdrawMutex[msg.sender] = true; amountToWithdraw = userBalances[msg.sender]; if (amountToWithdraw > 0) { if (!(msg.sender.send(amountToWithdraw))) { throw; } } userBalances[msg.sender] = 0; withdrawMutex[msg.sender] = false; }

Note that here we can keep the userBalances zeroing in the intuitive spot in the code. This costs an additional amount of gas, maybe in the realm of 10k gas, because we are changing a variable.

That said, I haven't had time to think through all the implications of this, and I'm happy for any feedback: e-mail me and tell me your thoughts.