Menu

Subscribe

Deconstructing theDAO Attack: A Brief Code Tour

on thedao, security, ethereum, solidity

TheDAO was attacked today, and the attacker seems to have made off with 3.5mm ether (at time of writing in excess of $45mm). The vulnerability was the Race To Empty or Recursive Call attack.

We'll probably be writing about this for some time, but I wanted to get out there what's known now, and trace out the attack so people can see how this attack looks in the wild.

Thanks for this writeup go to a number of helpful people in Ethereum land: Joey Krug wrote up some initial thoughts and is a source of knowledge and information at all times. Dennis Peterson as usual did a bunch of heavy lifting and code review in the early stages this morning, Nick Johnson did theoretical work and batted around some possible solutions.

Tim Goddard identified the most expensive typo this year through prodigious effort, and led the way figuring out why this exploit was able to turn 258 ether into 3.5mm rather than 'just' 7,500.

Some Background And Where To Look

We'll start with the splitDAO function, for two reasons -- the attacker seems have been creating child DAOs, and this is the only active mechanism for getting coins out; a second step would be to look at the proposal mechanism if this is unsuccessful.

To recap the purpose of this function: a subset of TheDAO token holders have decided they'd like to "split" -- either because they do not agree with a proposal, or (at this stage in theDao's brief but eventful life) because they wish to withdraw funds.

The mechanism for doing so is to create a split proposal. Split proposals take 7 days to 'mature' and get participants in. Any participants voting "yes" in the split will be given the right to call splitDAO.

splitDAO will create the DAO child contract if it doesn't exist, send the ether controlled by the splitters into the childDAO, pay out any accrued 'reward' pro-rata to the splitters, and return. At least, that's the plan.

A complication

Some code review happened on the currently live 'tip' of the master branch of theDAO's code on github. Unfortunately, that is NOT the code that has been deployed at theDAO's main contract address. This caused quite a lot of confusion, and highlights a real issue in these analyses -- is the code I'm looking at the same code that is running?

More on this at a later date, but verification is a problem, and it bit some fairly sophisticated reviewers in a hurry this morning.

The Code

I'm cutting out some of the splitDAO function here to focus on the good bits.

function splitDAO(  
        uint _proposalID,
        address _newCurator
    ) noEther onlyTokenholders returns (bool _success) {

Okay, we've got a function. It can't be sent ether. It can only be called by token holders. Let's see what we've got.

[snip]...

A line that says "move ether". Tell me more...

        // Move ether and assign new Tokens
        uint fundsToBeMoved =
            (balances[msg.sender] * p.splitData[0].splitBalance) /
            p.splitData[0].totalSupply;
        if (p.splitData[0].newDAO.createTokenProxy.value(fundsToBeMoved)(msg.sender) == false)
            throw;

Okay, what's happening here is we're calculating out how much to move for this particular caller, and then calling the createTokenProxy function. Let's remember this bit.

[snip]

        // Burn DAO Tokens
        Transfer(msg.sender, 0, balances[msg.sender]);
        withdrawRewardFor(msg.sender); // be nice, and get his rewards
        totalSupply -= balances[msg.sender];
        balances[msg.sender] = 0;
        paidOut[msg.sender] = 0;
        return true;
    }

Okay, this looks dubious from the very beginning. The withdrawRewardFor function is getting called, and then the totalSupply, balances and paidOut variables are getting set after the call.

This is an anti-pattern as we've established in the last few weeks. If withdrawRewardFor can be attacked with Race To Empty, it will be called before the balances or paidOut hash tables are updated.

withdrawRewardFor is Vulnerable

Let's see what's happening with withdrawRewardFor. 

function withdrawRewardFor(address _account) noEther internal returns (bool _success) {  
        if ((balanceOf(_account) * rewardAccount.accumulatedInput()) / totalSupply < paidOut[_account])
            throw;

        uint reward =
            (balanceOf(_account) * rewardAccount.accumulatedInput()) / totalSupply - paidOut[_account];
        if (!rewardAccount.payOut(_account, reward))
            throw;
        paidOut[_account] += reward;
        return true;
    }

The function is short enough I left the whole thing in. But the (bad) magic happens where the reward is set. We can see that the rewardAccount.payOut function is called. let's check out that function.

rewardAccount is a contract of type "ManagedAccount". Here's the relevant code snippet.

function payOut(address _recipient, uint _amount) returns (bool) {  
        if (msg.sender != owner || msg.value > 0 || (payOwnerOnly && _recipient != owner))
            throw;
        if (_recipient.call.value(_amount)()) {
            PayOut(_recipient, _amount);
            return true;
        } else {
            return false;
        }
    }

OK, here we go. _recipient.call.value()() is called without a gas amount. That is easily attackable with an attacking wallet.

Let's Construct Our Attack

  1. Create a wallet contract that has a default function which calls splitDAO a certain number of times, but not too many -- we don't want to drain the entire DAO, hit the per contract gas limit, or the callstack limit.
  2. Create a split proposal with the recipient address of our new wallet contract
  3. Wait 7 days for the split to finalize
  4. call splitDAO.

At that point the call stack will look like this (assuming the wallet only calls twice)

    splitDao
      withdrawRewardFor
         payOut
            recipient.call.value()()
               splitDao
                 withdrawRewardFor
                    payOut
                       recipient.call.value()()

And only then does the call stack resolve. So, boom. There's the nested call stack. Let's talk implications.

Edit As Martin Köppelmann noted, the attacker actually just joined an existing split with a new wallet contract two days ago. There was no need to wait 7 days, any available split contract would do.

Implication 1: The DAO is Now Confused

Let's recall the last few lines of the split function: 

 withdrawRewardFor(msg.sender); // be nice, and get his rewards
        totalSupply -= balances[msg.sender];
        balances[msg.sender] = 0;
        paidOut[msg.sender] = 0;

Here's what happens at the end of the second call to splitDAO:

  1. The totalSupply goes down by the sender balance. (In this case about 258 ether).
  2. Sender's balance is set to zero
  3. Sender's paidOut balance is set to zero

Here's what happens at the end of the top call to splitDAO:

  1. The totalSupply goes down by the sender balance, which is now 0.
  2. Sender balance re-set to zero.
  3. paidOut balance is re-set to zero.

So, theDao thinks it has 258 more coins than it has.

Right now, as of writing, theDAO thinks it has 3.5mm more coins than it has. There's some discussion as to what this means, but almost certainly it means other splits and reward calculations are going to be incorrect.

Implication 2: A Child DAO Has $45mm More Than It Should

So, what else happened during that nested call? Mostly the Child DAO was funded.

Here's the salient bit.

// Move ether and assign new Tokens
        uint fundsToBeMoved =
            (balances[msg.sender] * p.splitData[0].splitBalance) /
            p.splitData[0].totalSupply;
        if (p.splitData[0].newDAO.createTokenProxy.value(fundsToBeMoved)(msg.sender) == false)
            throw;

Tokens are moved into the newDAO, whatever the user's pro-rata portion of theDAO's total ether / token ratio. I'm going to skip digging in, but in brief, tokens are created in the subDAO, and a ManagedAccount tied to the msg.sender is sent fundsToBeMoved ether.

The attacker was able to do this 30x per call with their attacking wallet.

30 * 258 != 3,500,000, Also the Worst Single Letter Typo of The Year

a 30x attack would have pulled out about 7,500 ether. How did this turn into a much larger attack? Tim Goddard found this terrible, terrible bug in the code, either a typo or a malicious attack.

Let's look back at the withdrawRewardFor lines. Note that I have left all code indented as it is published at etherscan's verification section. 

// Burn DAO Tokens
        Transfer(msg.sender, 0, balances[msg.sender]);
        withdrawRewardFor(msg.sender); // be nice, and get his reward

It's typical in solidity contracts to capitalize event logging functions. I assumed on my first read-through that this is the purpose of Transfer. And, in fact, if you read Transfer's code, you see the following: 

 event Transfer(address indexed _from, address indexed _to, uint256 _amount);

Looks legit to me. I moved on. Tim is a more persistent person, and asked the salient question: how do the DAO tokens get burnt here? Is this just a bad comment?

It would be a true comment IF the function called was transfer -- note the lower case t. Here's that lower-case-t transfer function.

function transfer(address _to, uint256 _amount) noEther returns (bool success) {  
        if (balances[msg.sender] >= _amount && _amount > 0) {
            balances[msg.sender] -= _amount;
            balances[_to] += _amount;
            Transfer(msg.sender, _to, _amount);
            return true;
        } else {
           return false;
        }
    }

This function will reduce user balances, before the vulnerable withdraw function is called. So, instead of the logging function, we should have: 

if (!transfer(msg.sender, balances[msg.sender])) { throw; }

Root Causes And Lessons Learned

This bug is a confluence of bad programming habits, a (probable) typo, and a complex call stack.

Things that should be done next time:

  1. All calls that send to untrusted address should have a gas limit
  2. Balances should be reduced before a send, not after one
  3. Events should probably have a Log prepended to their name.

Further, we're still waiting to see what happens with respect to any possible hard fork that might roll these back, but I think there is a risk that theDAO is somewhat borked right now because it's sense of its own tokens is wrong. I will update on this and many other tidbits over the next week.

As always, contact me for audit and other solidity-related work - others who worked on this may be available as well; the best place to contact them is on gitter. Also, I'm told Tim works for Aura Security.