Hacker News Reader

Comment by mmanfrin | Hacker News Reader

Having a little bit of trouble parsing how the attack worked -- so when a transfer happens, the destination account gets to run a command; they then ran a command that called the source's send-ether command, which ran because it was still in 'we owe destination account money' mode because the full initial contract had not concluded?

Then at some point it terminates itself before emptying the source account (and thereby causing a rollback that would cascade back?)

View on HN

You've nailed it. In this case, the call stack depth would halt you before TheDAO runs out of funds, though. So you end up with some multiple, say 30x, where you can put in 1 ether and drain 30 ether. There is about a month lockup period during which your seed cash and your multiplied cash is frozen.

What made this particularly devastating is that there was a second bug which allowed the attacker to transfer their token that represented that first ether back to the DAO. So instead of 30x return, they could eventually drain the whole DAO (although they stopped after the discussion of a fork began).

I'm largely basing my understanding off this article: http://vessenes.com/deconstructing-thedao-attack-a-brief-cod...

My understanding is that it looks essentially like this:

    send(theEther, childDao)
    deduct(theEther, tokenHolder)

But that you can get it to overflow the stack in between the two so that the send completes but the deduction no longer happens.

If you overflow the stack it throws and rolls back (which is another attack in certain circumstances). What killed TheDAO is basically that childDao called back the sender method, which sends again without having deducted yet.

Do the deduct before the send, and the whole problem goes away.

Right, but would the deduction commit even if the send fails? Isn't this the whole concurrency 101 "Atomic deduct and send" problem?

It's not really concurrent, the VM runs one transaction at a time.

The send returns false if it fails. So if you check for that, you can throw an exception, which rolls back the whole transaction just like in SQL.

The reason the send doesn't automatically throw is that if you were doing lots of sends in a loop, you'd actually want to ignore the exception, so if one send fails it doesn't block everybody.

>if you were doing lots of sends in a loop, you'd actually want to ignore the exception, so if one send fails it doesn't block everybody.

This sounds like the wrong default behaviour. If we're talking about sending money, and there's a list of people to send money to, I would say that the default behaviour should be to throw an exception, because if not you could end up having money not be properly redistributed.

For example, a simple smart contract to share costs. If one member not longer participates then shouldn't their cut be given to the remaining participants?

The fact that it doesn't roll back everything by default concerns me quite a bit...

You can accomplish that by checking the return value. But Solidity doesn't have try/catch, it just rolls back everything upon throw. If send() were to actually throw upon failure, then it would roll back the entire method call and nobody gets paid. A troublemaker could use an account that always throws when it receives funds, and permanently block all payouts for everybody (unless a contract admin has a way to kill accounts).

The best pattern is not to put sends in a loop at all. Just update a ledger inside the contract, and have each user call a withdraw() function to get their money. That way if you're careful you can even use call.value, so users can include as much gas as required, and can withdraw the money directly into any sort of contract they like.

Sending before deducting the money seems like an obvious design flaw that should have raised red flags. Is there any explanation for why it was implemented this way, and why it wasn't spotted by the developers?

Here is the analysis:

http://hackingdistributed.com/2016/06/18/analysis-of-the-dao...

Reply on news.ycombinator.com