6 ms·

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

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?
  - jstolfi 10y ago
    > 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.
    - Natanael_L 10y ago
      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.
      - josu 10y ago
        Or a malicious one. The move has to be voluntary.
      - csarva 10y ago
        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).
legohead 10y ago
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?
- thoth 10y ago
  The language is Solidity (https://solidity.readthedocs.io/en/latest/ 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.
vessenes 10y ago
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.
- deleted 10y ago
  [deleted]
- deleted 10y ago
  [deleted]
rrradical 10y ago
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.
- wyager 10y ago
  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.
vatotemking 10y ago
One of the hardest things in computer science: naming things

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?
- jstolfi 10y ago
  > 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.
  - Natanael_L 10y ago
    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.
    - josu 10y ago
      Or a malicious one. The move has to be voluntary.
    - csarva 10y ago
      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).

legohead 10y ago

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?

thoth 10y ago
The language is Solidity (https://solidity.readthedocs.io/en/latest/ 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.

vessenes 10y ago

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.

deleted 10y ago
[deleted]
deleted 10y ago
[deleted]

rrradical 10y ago

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.

wyager 10y ago
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.

vatotemking 10y ago

One of the hardest things in computer science: naming things