Hacker News Reader

Comment by winteriscoming | Hacker News Reader

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?

View on HN

I think after sleeping on it that I agree with you, this is more likely the outcome of bad refactoring. You can read the code commits on github if you like; I don't have them handy, unfortunately.

transfer is a very poorly named function, of that there is no doubt. And Transfer is badly named as well.

transferAndLockTokens vs LogTransfer would be much, much better.

Test cases...code review... How about type checking??

The idea of writing financial software without taking every available precaution to verify correctness is insane to me.

This is why Fintech groups are some of the biggest users of strongly typed functional languages (ocaml, Haskell, etc). Your insane-o-meter is working.

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.

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/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

(the other repo) 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/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?

> this typo apparently hasn't yet been fixed in the repos

IIUC, once an Ethereum program is started, it cannot be killed or fixed. If so, updating the gitbub repo would be pointless at this time.

If it has a provision for forwarding all assets (with the necessary internal state detailed) to another address/DAO, then you can basically perform a migration to a fixed version.

Or a malicious one. The move has to be voluntary.

There is no such provision. You can transfer the "balance" of a contract to another, but not any of it's internal state. This is left up to the programmer and AFAICT, they didn't implement this feature in the DAO contract(s).

One of the hardest things in computer science: naming things

This is what I wanted to ask about. I don't even know what language this is. Is Transfer even a legit function, and if not, does this language just keep on chugging for non-existant function calls?

The language is Solidity (https://solidity.readthedocs.io/en/latest/) which runs on the Ethereum VM.

Certainly one of the challenges for the project is implementation and security issues in the Turing-complete language they decided to create for the purpose of smart contracts. Postscript and ActionScript have probably averaged a security flaw a month for the last decade; I can't imagine getting Solidity correct from the start.

Reply on news.ycombinator.com