13 ms·

Deconstructing the DAO Attack: A Brief Code Tour

253 points by bpierre 10y ago

jacquesm 10y ago
Whoever thought it was a good idea to have case sensitive function names where the names are allowed to be identical but not identical in function? Major fuck-up there. That should have never passed the concept stage, nor the review stage. Function names should describe what a function does.
- revelation 10y ago
  What a shitshow. These people don't need "advanced static analysis tools" [1], they need a big old bag of clue. 1: Developers, cryptographers and computer scientists should note that any high-level tools (including IDEs, formal verification, debuggers, symbolic execution) that make it easy to write safe smart contracts on Ethereum are prime candidates for DevGrants, Blockchain Labs grants and String’s autonomous finance grants.
  - ethbro 10y ago
    OpenSSL had some code cruft too, and god knows that didn't stop it from being deployed in production. People use code because it works, not code that's perfect.
    - kartickv 10y ago
      That's not good enough for software that's meant to be money. Besides, OpenSSL has the excuse of being old. Why would you design a new system to be crufty from day one?
      - ethbro 10y ago
        I wouldn't design any system to be crufty or have bugs. I was just pointing out that it happens. And honestly probably gives us a lot cooler stuff than were software production restricted to developers who were certified to produce best practices code. I feel like without weird blood we'd only ever get enterprise-approved spreadsheet software. PS: Caveat. Personally, of course I'd never use stuff like that for mission critical deployments until it's been battle tested.
- _pdp_ 10y ago
  Especially in code that supposedly describes contracts.
- buttershakes 10y ago
  I couldn't agree more with this sentiment. I haven't spent too much time on Ethereum contracts but this is such a ridiculous language/platform design issue.
  - semi-extrinsic 10y ago
    Funny thing is, it reminds me of implicit typing of int/float based on the first letter of the variable name that ye olde FORTRAN77 had as a major footgun.
    - codingmyway 10y ago
      Upvoted for introducing me to the word footgun
- draw_down 10y ago
  The problem is deeper, this idea of contracts enforced by computer code written by humans ( = full of bugs) is a dumb one.
  - krisives 10y ago
    Just because some smart contracts fail doesn't mean they all have to.
    - benjcooley 10y ago
      The problem is one of trust. Contracts that can either be formally verified, or by nature of the language they are written in are mathematically guaranteed to be free of certain bugs and behaviors can be 'TRUSTED'. It's a very bad thing to not be able to trust that your money can't be trivially stolen.
    - draw_down 10y ago
      Yeah. But which are which, and are you willing to put your money on that?
- charonn0 10y ago
  Are we sure that case sensitivity is the culprit? As others have noted, the function being called takes (and is given) 3 arguments but the function that is supposed to be called takes only 2.
  - jacquesm 10y ago
    IDE and autocompletion could be the direct cause, but in the end if the function had been properly named it wouldn't have happened (root cause).
- rbobby 10y ago
  Case sensitivity in any programming language is crazy. I can't for the life of me see a valid engineering principal that accepts IsTheOne() and istheone() being different bits of code. Oh sure at a technical level the computer has no problem with... the problem is restricted to those oh so error prone humans. Can anyone here honestly say that if they were doing a code review they'd agree that solely a difference in case would get their approval? And don't even get me started on filenames that only differ by case. If that's a good idea then I think trailing whitespace should be significant too!!!
  - jsprogrammer 10y ago
    IsTheOne() and istheone() are different bits of code.
    - kartickv 10y ago
      That's a circular argument. They are different only because the language says they are different, while the post you've responded to says (correctly) that that's bad language design.
      - jsprogrammer 10y ago
        The bit representation is different.
        kartickv 10y ago
        Which is irrelevant, since languages are a medium of human expression, not a way to store bits on the disk.
        jsprogrammer 10y ago
        Irrelevant to what? The two words literally are represented with different bits (code).
- bzbarsky 10y ago
  For any language that allows non-ASCII identifiers, trying to define "case insensitive" is a minefield best avoided. For example, with general Unicode characters, "case-insensitive" comparison doesn't give you an equivalence relation on identifiers because transitivity fails. Of course you can define things to be case-insensitive only in the ASCII range, and treat your non-ASCII stuff as second-class citizens...
willtim 10y ago
Modelling financial contracts in an imperative event-driven paradigm just seems like an accident waiting to happen. EDIT: To the downvoters, if you disagree, I would very much like to understand why, please reply with a comment. EDIT2: My position is that it is very difficult to reason about correctness and maintain invariants in a highly imperative setting. IMHO, it would be more desirable to use a declarative, or functional language. There are many successful commercial applications of functional programming to financial contracts, e.g. Lexifi, which is based on a subset of OCaml and various in-house solutions developed by banks. Using a functional programming language would also mean the code is far more amenable to formal methods and static verification. One new startup I was very impressed with, aestheticintegration.com, has had success rewriting financial algorithms in a functional programming language in order to prove and verify numerous correctness properties about them. It is a huge shame that folk do not reach straight for the functional programming toolbox when faced with these types of problems.
- rahimnathwani 10y ago
  I didn't downvote you, but your comment left me scratching my head. You made a statement without any explanation. Care to elaborate why imperative programming is so bad here? Real-world contracts also use imperative style.
  - jacquesm 10y ago
    This code is very bad, without going into an exhaustive enumeration, case sensitive function names, variables with unclear scope all over the place and so on. That it's imperative code is something that I don't think is damning by itself but all the side-effects (including side effects based on a single bit in the name of a function) really do warrant the 'accident waiting to happen'.
    - willtim 10y ago
      While personally I would prefer a functional approach, I take your point that a much more restrictive imperative setting would also be considerably better.
fabled_giraffe 10y ago
I know I'll get napalmed for saying this, but why is this all over HN? Why are people obsessed with cryptocurrencies? It's bad enough that those with sufficient computing speed/power can already rip off the stock market; why does the world need another way for people to rip each other off? Since we've determined over history that some people will take advantage of weakness for their own gain, no matter what system you create to transfer goods, resources, services or value representing those, it will be exploited. So- why even spend time on it? If everyone who cares to maintains their own accounting data does so, regardless of what technologies we are using, we really could just trade in goods, services, and coin and completely get rid of all virtual currency. Investments could go back to literally to providing coins, goods, resources, or services to those that we'd like to support possibly in exchange for reciprocity. Since that won't happen right away, if you really want to secure your financial future, major S&P 500 index tracking funds have been one of the smartest things over its history to invest in: http://finance.yahoo.com/echarts?s=%5EGSPC+Interactive#symbol=%5EGSPC;range=my http://finance.yahoo.com/echarts?s=%5EGSPC+Interactive#symbo... While some of you made money on bitcoin, many lost. Now the same game has been played again with people believing that someone can write code to replace our currency system and if they get in early enough, they'll be kings or queens. Well- again, it didn't happen. What really is to gain from this fantasy world of cryptocurrencies? It is harming more than it is helping, from what I see. And if you downvote this, tell me why, otherwise I'll just assume you are with those that want to exploit me and others.
- TazeTSchnitzel 10y ago
  > I know I'll get napalmed for saying this, but why is this all over HN? Because the DAO's failure is interesting.
  - draw_down 10y ago
    And funny!
    - dang 10y ago
      A number of internet forums have branched off from HN to fill niches that HN doesn't fill. I'm beginning to think that somebody needs to fork a forum for Schadenfreude. We don't want comments like this here.
jcfrei 10y ago
So, this review makes me wonder: Will the hard fork of the ethereum blockchain be of any use at all? The DAO appears to be fundamentally flawed, are all the coins there simply lost? What prevents an attacker from exploiting the DAO after the hard fork? Or will they simply invalidate all the transactions that went to the address of the DAO?
- tdaltonc 10y ago
  As I understand it: The goal of the fork is to freeze the DAO account starting just before the 'split' drain (at the blockchain level). And then figure out a long term solution (possibly using another fork to return all ETH to the original contributing accounts dissolving the DAO?). The ability of a core of developers and miner to exercise their will over the system like this is alarming though. https://blog.ethereum.org/2016/06/17/critical-update-re-dao-vulnerability/ https://blog.ethereum.org/2016/06/17/critical-update-re-dao-...
deleted 10y ago
[deleted]
altern8tif 10y ago
So all this hoo-ha is really just because of a capitalised "T"? If the code were fixed, is the DAO model still valid in the real world? (I'm a cryptocurrency noob... Some of technical stuff just went over my head)
- kolinko 10y ago
  The real question is whether smart contracts can ever be safe enough so that they have real world applications.
seibelj 10y ago
Bored computer scientists: analyze contracts that are less popular but still have money attached to them and see if you can locate a bug. Apparently you can get the money legally and the only way to stop you is to hard fork the entire currency.
homero 10y ago
Glad I never touched alts. My bitcoin are safe and increasing
tinco 10y ago
In addition to this the language is not explicit enough. Even standard Haskell wouldn't be safe enough for a program that manages $250MM directly without safe guards. In addition to typesafety, it would have to be both explicitly typed at the lowest possible granularity and annotated with pre and post conditions. Not only did this code allow a bunch of tokens be transferred without compensation, it left the whole accounting system in a corrupted state! One that could have easily been detected and verified at any point. This line: ``` Transfer(msg.sender, 0, balances[msg.sender]); ``` Should have looked like this: ``` // postcondition: balances[msg.sender] == 0 Transfer(msg.sender, 0, balances[msg.sender]) :: Action<Transfer>; // Compiler result: // # Error 1: line XX expected type Action<Transfer> but instead got type Transfer // # Error 2: line XX violates postcondition, expected balances[msg.sender] == 0, got balances[msg.sender] != 0 ``` And the whole function 'splitDao' should have a type annotation that completely captures the possible actions it might encompass, and pre- and postconditions that at the very least constrain the result of the function to not corrupt the accounting of the DAO (i.e. it should still have as many tokens as it believes it has). It's nice that Vitalik Buterin is a genius, but it shows that this guy is only 22 and dropped out of university because anyone with a degree in computer science knows about this stuff and its importance in high reliability systems. He should have known the contract language should have had typechecked side effect and he should have known proper pre- postconditions would have been the least he should have done.
- tmalsburg2 10y ago
  One solution might be to design a safe language that compiles to the the language used in Ethereum. Not sure if possible within the given constraints.
  - eudox 10y ago
    The problem then becomes ensuring that only code compiled from that language can run.
    - tmalsburg2 10y ago
winteriscoming 10y ago
The apparent typo is odd, IMO. I don't know the language in which that program is written, but looking at that snippet, "Transfer" takes 3 arguments whereas the "transfer" function takes 2 arguments. Isn't there any code review involved when this makes into the codebase. Assuming there was some code review and the reviewer just missed it (which is very much possible), a basic unit test case would have easily caught this bug, isn't it? After all, the test case would have checked for the balance etc... after execution of these functions. Aren't there any test cases around this?
- niftich 10y ago
  I couldn't follow that Transfer vs transfer call either; the number of arguments don't match, and I don't know where 'amount_' comes from. The code is exceedingly painful to read, and makes me wish I had been involved in this effort before it made the news. I can't help but think, lack of unit tests aside, is this the first time they're running this code? What sorts of decision-making led to this outcome? I want Hanlon's razor to apply but given the amount of money involved, I am not entirely convinced.
  - winteriscoming 10y ago
    If I understood the explanation on that article correctly, what's even more interesting is that this typo apparently hasn't yet been fixed in the repos (assuming I'm looking at the right ones). There are 2 repos which I looked at: https://github.com/slockit/DAO https://github.com/slockit/DAO https://github.com/TheDAO/DAO-1.0 https://github.com/TheDAO/DAO-1.0 I don't know which one is the authoritative one. But the interesting bit is both these repos still have that same typo in the master branch: https://github.com/TheDAO/DAO-1.0/blob/master/DAO.sol#L666 https://github.com/TheDAO/DAO-1.0/blob/master/DAO.sol#L666 (the other repo) https://github.com/slockit/DAO/blob/develop/DAO.sol#L685 https://github.com/slockit/DAO/blob/develop/DAO.sol#L685 The slockit account repo has a commit which was made 6 days back, with the commit message "Protect against recursive withdrawRewardFor attack" https://github.com/slockit/DAO/commit/f01f3bd8df5e1e222dde625118b7e0f2bfe5b680 https://github.com/slockit/DAO/commit/f01f3bd8df5e1e222dde62... but it doesn't fix the typo and there aren't any more commit in there after that one. Does this mean, the upstream repos containing the typo haven't yet been fixed? Or am I just looking at the wrong repos/branches? Or did this typo get known only a few hours back?
harmegido 10y ago
I'm curious, are there no tests for the code running this? It seems like some of these bugs would be caught with them (especially the typo). And it seems like insanity to not have them.
htns 10y ago
I don't get how the transfer vs Transfer typo is supposed to have enabled a larger attack. The token balance is zeroed in the splitDAO function anyway, and presumably the sanity checks in transfer are also redundant, or else there would have been no need for recursion.
RobertoG 10y ago
I'm going to be a little polemic here. It's this an attack? It's not the problem that those technologies try to solve, to get rid of subjectivity?. In a way, this could be interpreted as trying to be free of politics. If my understanding of what they are trying to accomplish here is correct, if the system allow it, then, by definition, it's legal. If you require a framework where something allowed by the code but with unexpected consequences is illegal, you are again where you started.
- wyager 10y ago
  You're being thoroughly downvoted, but I don't see anything wrong with your argument. The person who pulled off this "hack" was following all the rules. In fact, I could even see the argument that by undoing this transaction, the people who started the DAO are in fact going back on their word. They promised that they would build a system which would obey certain rules. By undoing this transaction, they are no longer following those rules. Now, obviously there's a strong argument for rolling back the transactions, but at this point ethereum has lost any claim to objectivity or decentralization.
  - developer2 10y ago
    >> obviously there's a strong argument for rolling back the transactions Absolutely not. Such an act would entirely destroy the credibility of the entire system and its future. If not rolling this back is detrimental to the system, then it has already failed and should be dismantled. Who would continue to use a blockchain that permits such a thing? It completely defeats the purpose.
- deleted 10y ago
  [deleted]
beachstartup 10y ago
if you could 100% accurately and reliably model a contractual understanding in any kind of language, it would have been done eons ago. human language (i'm fond of english in particular), is extremely expressive. but there's a reason we still have courts and judges. it's because language is the tool through which humans express their desires and fears (computer language included, because it is written by humans, so far), it's not a machine.
- tdaltonc 10y ago
  I don't think that there was any pressure to deeply formalize contrasts before this because even if you did, there would still be recourse to a human judge (not just a compiler). If one party didn't like the strict reading they could sue. The question that's playing out right now is "is there human judge behind the code." If a set of humans (miners and developers) successfully overrule the code, this will seriously undermine the whole premise of this project.
tdaltonc 10y ago
It looks like this is a typo. The question is, "does that matter?" It seems to me that either the code means what it says, or it doesn't. If a cabal of humans are able to overrule the machines, then what is the point of Etherium? Wouldn't we just be trading one group of authorities for another? I want to see Etherium work, and it is extremely unfortunate that basically THE first high profile smart contract has gone sideways. But I'm not sure if I want there to be anything we can do about it. The whole point of this project is that the blockchain is supposed to be like an indifferent force of nature.
lordnacho 10y ago
It reads like the debugging that you've done a million times if you are working with ordinary, mutable, C-family code. To start off with, you think you're developing a valuable skill in being able to follow a call stack along with a bunch of variables. It certainly has its uses, but after a while you come to wonder why it always ends up this way. Sure, you can understand every bug eventually, but surely there's some reason why you're constantly reasoning about the state of the variables and the order of the function calls. You can write unit tests, but of course you can't think of all the undesired behaviours beforehand. The author does mention that something functional with strong types is necessary. Probably a good idea. As for the split function, there's a question of whether it is really necessary. I wouldn't have thought an investment fund would need a split function. An ordinary fund doesn't; if you're in, you're in for whatever proportion that you've put in. Why not just make a second fund with the same (reduced) code if you have some other group of investors? More functions = bigger attack surface.
amaks 10y ago
The biggest problem I see with the code at https://github.com/slockit/DAO/ https://github.com/slockit/DAO/ is absense of the unit tests. The code looks complex, how the hell can you make sure that it's right? The Solidity wiki doesn't have a single entry on testing either: https://github.com/ethereum/wiki/wiki/The-Solidity-Programming-Language https://github.com/ethereum/wiki/wiki/The-Solidity-Programmi...
- graup 10y ago
  The repo you linked does have a "tests" directory. It contains "scenarios", so not really unit tests. You can't really unit-test contracts at the moment.
winteriscoming 10y ago
This function will reduce user balances, before the vulnerable withdraw function is called. So, instead of the logging function, we should have: if (!transfer(0 , balances[msg.sender])) { throw; } This would <snip>.... also reduce the tokens available to the user later on The more I think of the typo and the explanation about it in that article, the more unclear I am about that whole code. Keeping aside this hack, for a moment, if that is indeed a typo and instead it should have called the lower case transfer method to really reduce the user tokens, then does this mean that all these DAO contracts (or whatever the right term is) that have been executed till date have been affected by this and as such the entire eco-system state was already messed up before this hack? Nothing in that article suggests that this code flow, where this apparent typo is present, is applicable only for this specific hack.
- caf 10y ago
  No, because after the call to Transfer() and withdrawRewardFor(), it sets their balance to zero: totalSupply -= balances[msg.sender]; balances[msg.sender] = 0; paidOut[msg.sender] = 0; So the bug is only exposed if the recipient of the reward transfers out the balance during the execution of withdrawRewardFor(). In fact, to me it doesn't look like there's a typo at all there - the call to Transfer() was fully intended just to log the burning of the tokens that will be accomplished by the later lines setting the balance to zero. The bug is simply that the balance isn't adjusted before the attacker gets a chance to execute code.
  - htns 10y ago
    The top comment on /r/ethereum agrees with you on it not being a typo: https://www.reddit.com/r/ethereum/comments/4onbkj/deconstructing_thedao_attack_a_brief_code_tour/ https://www.reddit.com/r/ethereum/comments/4onbkj/deconstruc... . Hopefully in a few days we will have an authoritative explanation. Otherwise this might become a HN urban legend.
waxmiguel 10y ago
Can welcome revocover?

253 points by bpierre 10y ago

Whoever thought it was a good idea to have case sensitive function names where the names are allowed to be identical but not identical in function? Major fuck-up there. That should have never passed the concept stage, nor the review stage. Function names should describe what a function does.

revelation 10y ago
What a shitshow. These people don't need "advanced static analysis tools" [1], they need a big old bag of clue. 1: Developers, cryptographers and computer scientists should note that any high-level tools (including IDEs, formal verification, debuggers, symbolic execution) that make it easy to write safe smart contracts on Ethereum are prime candidates for DevGrants, Blockchain Labs grants and String’s autonomous finance grants.
- ethbro 10y ago
  OpenSSL had some code cruft too, and god knows that didn't stop it from being deployed in production. People use code because it works, not code that's perfect.
  - kartickv 10y ago
    That's not good enough for software that's meant to be money. Besides, OpenSSL has the excuse of being old. Why would you design a new system to be crufty from day one?
    - ethbro 10y ago
      I wouldn't design any system to be crufty or have bugs. I was just pointing out that it happens. And honestly probably gives us a lot cooler stuff than were software production restricted to developers who were certified to produce best practices code. I feel like without weird blood we'd only ever get enterprise-approved spreadsheet software. PS: Caveat. Personally, of course I'd never use stuff like that for mission critical deployments until it's been battle tested.
_pdp_ 10y ago
Especially in code that supposedly describes contracts.
buttershakes 10y ago
I couldn't agree more with this sentiment. I haven't spent too much time on Ethereum contracts but this is such a ridiculous language/platform design issue.
- semi-extrinsic 10y ago
  Funny thing is, it reminds me of implicit typing of int/float based on the first letter of the variable name that ye olde FORTRAN77 had as a major footgun.
  - codingmyway 10y ago
    Upvoted for introducing me to the word footgun
draw_down 10y ago
The problem is deeper, this idea of contracts enforced by computer code written by humans ( = full of bugs) is a dumb one.
- krisives 10y ago
  Just because some smart contracts fail doesn't mean they all have to.
  - benjcooley 10y ago
    The problem is one of trust. Contracts that can either be formally verified, or by nature of the language they are written in are mathematically guaranteed to be free of certain bugs and behaviors can be 'TRUSTED'. It's a very bad thing to not be able to trust that your money can't be trivially stolen.
  - draw_down 10y ago
    Yeah. But which are which, and are you willing to put your money on that?
charonn0 10y ago
Are we sure that case sensitivity is the culprit? As others have noted, the function being called takes (and is given) 3 arguments but the function that is supposed to be called takes only 2.
- jacquesm 10y ago
  IDE and autocompletion could be the direct cause, but in the end if the function had been properly named it wouldn't have happened (root cause).
rbobby 10y ago
Case sensitivity in any programming language is crazy. I can't for the life of me see a valid engineering principal that accepts IsTheOne() and istheone() being different bits of code. Oh sure at a technical level the computer has no problem with... the problem is restricted to those oh so error prone humans. Can anyone here honestly say that if they were doing a code review they'd agree that solely a difference in case would get their approval? And don't even get me started on filenames that only differ by case. If that's a good idea then I think trailing whitespace should be significant too!!!
- jsprogrammer 10y ago
  IsTheOne() and istheone() are different bits of code.
  - kartickv 10y ago
    That's a circular argument. They are different only because the language says they are different, while the post you've responded to says (correctly) that that's bad language design.
    - jsprogrammer 10y ago
      The bit representation is different.
      - kartickv 10y ago
        Which is irrelevant, since languages are a medium of human expression, not a way to store bits on the disk.
        jsprogrammer 10y ago
        Irrelevant to what? The two words literally are represented with different bits (code).
bzbarsky 10y ago
For any language that allows non-ASCII identifiers, trying to define "case insensitive" is a minefield best avoided. For example, with general Unicode characters, "case-insensitive" comparison doesn't give you an equivalence relation on identifiers because transitivity fails. Of course you can define things to be case-insensitive only in the ASCII range, and treat your non-ASCII stuff as second-class citizens...

willtim 10y ago

Modelling financial contracts in an imperative event-driven paradigm just seems like an accident waiting to happen. EDIT: To the downvoters, if you disagree, I would very much like to understand why, please reply with a comment. EDIT2: My position is that it is very difficult to reason about correctness and maintain invariants in a highly imperative setting. IMHO, it would be more desirable to use a declarative, or functional language. There are many successful commercial applications of functional programming to financial contracts, e.g. Lexifi, which is based on a subset of OCaml and various in-house solutions developed by banks. Using a functional programming language would also mean the code is far more amenable to formal methods and static verification. One new startup I was very impressed with, aestheticintegration.com, has had success rewriting financial algorithms in a functional programming language in order to prove and verify numerous correctness properties about them. It is a huge shame that folk do not reach straight for the functional programming toolbox when faced with these types of problems.

rahimnathwani 10y ago
I didn't downvote you, but your comment left me scratching my head. You made a statement without any explanation. Care to elaborate why imperative programming is so bad here? Real-world contracts also use imperative style.
- jacquesm 10y ago
  This code is very bad, without going into an exhaustive enumeration, case sensitive function names, variables with unclear scope all over the place and so on. That it's imperative code is something that I don't think is damning by itself but all the side-effects (including side effects based on a single bit in the name of a function) really do warrant the 'accident waiting to happen'.
  - willtim 10y ago
    While personally I would prefer a functional approach, I take your point that a much more restrictive imperative setting would also be considerably better.

fabled_giraffe 10y ago

I know I'll get napalmed for saying this, but why is this all over HN? Why are people obsessed with cryptocurrencies? It's bad enough that those with sufficient computing speed/power can already rip off the stock market; why does the world need another way for people to rip each other off? Since we've determined over history that some people will take advantage of weakness for their own gain, no matter what system you create to transfer goods, resources, services or value representing those, it will be exploited. So- why even spend time on it? If everyone who cares to maintains their own accounting data does so, regardless of what technologies we are using, we really could just trade in goods, services, and coin and completely get rid of all virtual currency. Investments could go back to literally to providing coins, goods, resources, or services to those that we'd like to support possibly in exchange for reciprocity. Since that won't happen right away, if you really want to secure your financial future, major S&P 500 index tracking funds have been one of the smartest things over its history to invest in: http://finance.yahoo.com/echarts?s=%5EGSPC+Interactive#symbol=%5EGSPC;range=my http://finance.yahoo.com/echarts?s=%5EGSPC+Interactive#symbo... While some of you made money on bitcoin, many lost. Now the same game has been played again with people believing that someone can write code to replace our currency system and if they get in early enough, they'll be kings or queens. Well- again, it didn't happen. What really is to gain from this fantasy world of cryptocurrencies? It is harming more than it is helping, from what I see. And if you downvote this, tell me why, otherwise I'll just assume you are with those that want to exploit me and others.

TazeTSchnitzel 10y ago
> I know I'll get napalmed for saying this, but why is this all over HN? Because the DAO's failure is interesting.
- draw_down 10y ago
  And funny!
  - dang 10y ago
    A number of internet forums have branched off from HN to fill niches that HN doesn't fill. I'm beginning to think that somebody needs to fork a forum for Schadenfreude. We don't want comments like this here.

jcfrei 10y ago

So, this review makes me wonder: Will the hard fork of the ethereum blockchain be of any use at all? The DAO appears to be fundamentally flawed, are all the coins there simply lost? What prevents an attacker from exploiting the DAO after the hard fork? Or will they simply invalidate all the transactions that went to the address of the DAO?

tdaltonc 10y ago
As I understand it: The goal of the fork is to freeze the DAO account starting just before the 'split' drain (at the blockchain level). And then figure out a long term solution (possibly using another fork to return all ETH to the original contributing accounts dissolving the DAO?). The ability of a core of developers and miner to exercise their will over the system like this is alarming though. https://blog.ethereum.org/2016/06/17/critical-update-re-dao-vulnerability/ https://blog.ethereum.org/2016/06/17/critical-update-re-dao-...

deleted 10y ago

[deleted]

altern8tif 10y ago

So all this hoo-ha is really just because of a capitalised "T"? If the code were fixed, is the DAO model still valid in the real world? (I'm a cryptocurrency noob... Some of technical stuff just went over my head)

kolinko 10y ago
The real question is whether smart contracts can ever be safe enough so that they have real world applications.

seibelj 10y ago

Bored computer scientists: analyze contracts that are less popular but still have money attached to them and see if you can locate a bug. Apparently you can get the money legally and the only way to stop you is to hard fork the entire currency.

homero 10y ago

Glad I never touched alts. My bitcoin are safe and increasing

tinco 10y ago

In addition to this the language is not explicit enough. Even standard Haskell wouldn't be safe enough for a program that manages $250MM directly without safe guards. In addition to typesafety, it would have to be both explicitly typed at the lowest possible granularity and annotated with pre and post conditions. Not only did this code allow a bunch of tokens be transferred without compensation, it left the whole accounting system in a corrupted state! One that could have easily been detected and verified at any point. This line: ``` Transfer(msg.sender, 0, balances[msg.sender]); ``` Should have looked like this: ``` // postcondition: balances[msg.sender] == 0 Transfer(msg.sender, 0, balances[msg.sender]) :: Action<Transfer>; // Compiler result: // # Error 1: line XX expected type Action<Transfer> but instead got type Transfer // # Error 2: line XX violates postcondition, expected balances[msg.sender] == 0, got balances[msg.sender] != 0 ``` And the whole function 'splitDao' should have a type annotation that completely captures the possible actions it might encompass, and pre- and postconditions that at the very least constrain the result of the function to not corrupt the accounting of the DAO (i.e. it should still have as many tokens as it believes it has). It's nice that Vitalik Buterin is a genius, but it shows that this guy is only 22 and dropped out of university because anyone with a degree in computer science knows about this stuff and its importance in high reliability systems. He should have known the contract language should have had typechecked side effect and he should have known proper pre- postconditions would have been the least he should have done.

tmalsburg2 10y ago
One solution might be to design a safe language that compiles to the the language used in Ethereum. Not sure if possible within the given constraints.
- eudox 10y ago
  The problem then becomes ensuring that only code compiled from that language can run.
  - tmalsburg2 10y ago

winteriscoming 10y ago

The apparent typo is odd, IMO. I don't know the language in which that program is written, but looking at that snippet, "Transfer" takes 3 arguments whereas the "transfer" function takes 2 arguments. Isn't there any code review involved when this makes into the codebase. Assuming there was some code review and the reviewer just missed it (which is very much possible), a basic unit test case would have easily caught this bug, isn't it? After all, the test case would have checked for the balance etc... after execution of these functions. Aren't there any test cases around this?

niftich 10y ago
I couldn't follow that Transfer vs transfer call either; the number of arguments don't match, and I don't know where 'amount_' comes from. The code is exceedingly painful to read, and makes me wish I had been involved in this effort before it made the news. I can't help but think, lack of unit tests aside, is this the first time they're running this code? What sorts of decision-making led to this outcome? I want Hanlon's razor to apply but given the amount of money involved, I am not entirely convinced.
- winteriscoming 10y ago
  If I understood the explanation on that article correctly, what's even more interesting is that this typo apparently hasn't yet been fixed in the repos (assuming I'm looking at the right ones). There are 2 repos which I looked at: https://github.com/slockit/DAO https://github.com/slockit/DAO https://github.com/TheDAO/DAO-1.0 https://github.com/TheDAO/DAO-1.0 I don't know which one is the authoritative one. But the interesting bit is both these repos still have that same typo in the master branch: https://github.com/TheDAO/DAO-1.0/blob/master/DAO.sol#L666 https://github.com/TheDAO/DAO-1.0/blob/master/DAO.sol#L666 (the other repo) https://github.com/slockit/DAO/blob/develop/DAO.sol#L685 https://github.com/slockit/DAO/blob/develop/DAO.sol#L685 The slockit account repo has a commit which was made 6 days back, with the commit message "Protect against recursive withdrawRewardFor attack" https://github.com/slockit/DAO/commit/f01f3bd8df5e1e222dde625118b7e0f2bfe5b680 https://github.com/slockit/DAO/commit/f01f3bd8df5e1e222dde62... but it doesn't fix the typo and there aren't any more commit in there after that one. Does this mean, the upstream repos containing the typo haven't yet been fixed? Or am I just looking at the wrong repos/branches? Or did this typo get known only a few hours back?

harmegido 10y ago

I'm curious, are there no tests for the code running this? It seems like some of these bugs would be caught with them (especially the typo). And it seems like insanity to not have them.

htns 10y ago

I don't get how the transfer vs Transfer typo is supposed to have enabled a larger attack. The token balance is zeroed in the splitDAO function anyway, and presumably the sanity checks in transfer are also redundant, or else there would have been no need for recursion.

RobertoG 10y ago

I'm going to be a little polemic here. It's this an attack? It's not the problem that those technologies try to solve, to get rid of subjectivity?. In a way, this could be interpreted as trying to be free of politics. If my understanding of what they are trying to accomplish here is correct, if the system allow it, then, by definition, it's legal. If you require a framework where something allowed by the code but with unexpected consequences is illegal, you are again where you started.

wyager 10y ago
You're being thoroughly downvoted, but I don't see anything wrong with your argument. The person who pulled off this "hack" was following all the rules. In fact, I could even see the argument that by undoing this transaction, the people who started the DAO are in fact going back on their word. They promised that they would build a system which would obey certain rules. By undoing this transaction, they are no longer following those rules. Now, obviously there's a strong argument for rolling back the transactions, but at this point ethereum has lost any claim to objectivity or decentralization.
- developer2 10y ago
  >> obviously there's a strong argument for rolling back the transactions Absolutely not. Such an act would entirely destroy the credibility of the entire system and its future. If not rolling this back is detrimental to the system, then it has already failed and should be dismantled. Who would continue to use a blockchain that permits such a thing? It completely defeats the purpose.
deleted 10y ago
[deleted]

beachstartup 10y ago

if you could 100% accurately and reliably model a contractual understanding in any kind of language, it would have been done eons ago. human language (i'm fond of english in particular), is extremely expressive. but there's a reason we still have courts and judges. it's because language is the tool through which humans express their desires and fears (computer language included, because it is written by humans, so far), it's not a machine.

tdaltonc 10y ago
I don't think that there was any pressure to deeply formalize contrasts before this because even if you did, there would still be recourse to a human judge (not just a compiler). If one party didn't like the strict reading they could sue. The question that's playing out right now is "is there human judge behind the code." If a set of humans (miners and developers) successfully overrule the code, this will seriously undermine the whole premise of this project.

tdaltonc 10y ago

It looks like this is a typo. The question is, "does that matter?" It seems to me that either the code means what it says, or it doesn't. If a cabal of humans are able to overrule the machines, then what is the point of Etherium? Wouldn't we just be trading one group of authorities for another? I want to see Etherium work, and it is extremely unfortunate that basically THE first high profile smart contract has gone sideways. But I'm not sure if I want there to be anything we can do about it. The whole point of this project is that the blockchain is supposed to be like an indifferent force of nature.

lordnacho 10y ago

It reads like the debugging that you've done a million times if you are working with ordinary, mutable, C-family code. To start off with, you think you're developing a valuable skill in being able to follow a call stack along with a bunch of variables. It certainly has its uses, but after a while you come to wonder why it always ends up this way. Sure, you can understand every bug eventually, but surely there's some reason why you're constantly reasoning about the state of the variables and the order of the function calls. You can write unit tests, but of course you can't think of all the undesired behaviours beforehand. The author does mention that something functional with strong types is necessary. Probably a good idea. As for the split function, there's a question of whether it is really necessary. I wouldn't have thought an investment fund would need a split function. An ordinary fund doesn't; if you're in, you're in for whatever proportion that you've put in. Why not just make a second fund with the same (reduced) code if you have some other group of investors? More functions = bigger attack surface.

amaks 10y ago

The biggest problem I see with the code at https://github.com/slockit/DAO/ https://github.com/slockit/DAO/ is absense of the unit tests. The code looks complex, how the hell can you make sure that it's right? The Solidity wiki doesn't have a single entry on testing either: https://github.com/ethereum/wiki/wiki/The-Solidity-Programming-Language https://github.com/ethereum/wiki/wiki/The-Solidity-Programmi...

graup 10y ago
The repo you linked does have a "tests" directory. It contains "scenarios", so not really unit tests. You can't really unit-test contracts at the moment.

winteriscoming 10y ago

This function will reduce user balances, before the vulnerable withdraw function is called. So, instead of the logging function, we should have: if (!transfer(0 , balances[msg.sender])) { throw; } This would <snip>.... also reduce the tokens available to the user later on The more I think of the typo and the explanation about it in that article, the more unclear I am about that whole code. Keeping aside this hack, for a moment, if that is indeed a typo and instead it should have called the lower case transfer method to really reduce the user tokens, then does this mean that all these DAO contracts (or whatever the right term is) that have been executed till date have been affected by this and as such the entire eco-system state was already messed up before this hack? Nothing in that article suggests that this code flow, where this apparent typo is present, is applicable only for this specific hack.

caf 10y ago
No, because after the call to Transfer() and withdrawRewardFor(), it sets their balance to zero: totalSupply -= balances[msg.sender]; balances[msg.sender] = 0; paidOut[msg.sender] = 0; So the bug is only exposed if the recipient of the reward transfers out the balance during the execution of withdrawRewardFor(). In fact, to me it doesn't look like there's a typo at all there - the call to Transfer() was fully intended just to log the burning of the tokens that will be accomplished by the later lines setting the balance to zero. The bug is simply that the balance isn't adjusted before the attacker gets a chance to execute code.
- htns 10y ago
  The top comment on /r/ethereum agrees with you on it not being a typo: https://www.reddit.com/r/ethereum/comments/4onbkj/deconstructing_thedao_attack_a_brief_code_tour/ https://www.reddit.com/r/ethereum/comments/4onbkj/deconstruc... . Hopefully in a few days we will have an authoritative explanation. Otherwise this might become a HN urban legend.

waxmiguel 10y ago

Can welcome revocover?