3 ms·

My guess at how the attackers are doing it: They are calling splitDAO: https://github.com/slockit/DAO/blob/develop/DAO.sol#L618 https://github.com/slockit/DAO

My guess at how the attackers are doing it: They are calling splitDAO: https://github.com/slockit/DAO/blob/develop/DAO.sol#L618 https://github.com/slockit/DAO/blob/develop/DAO.sol#L618 splitDAO calls withdrawRewardFor which ends up calling back into the users contract. https://github.com/slockit/DAO/blob/develop/DAO.sol#L686 https://github.com/slockit/DAO/blob/develop/DAO.sol#L686 withdrawRewardFor(msg.sender); // be nice, and get his rewards totalSupply -= balances[msg.sender]; balances[msg.sender] = 0; paidOut[msg.sender] = 0; the state is modified after the callback in particular the balances variable. however, earlier in the function it moved funds to a new dao based on the balances variable. // 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) so presumably an attacker can call splitDAO and then recursively call splitDAO and the funds will be transferred twice. there is also some complications around rewardToken because this state is modified before the callback but apparently it is all zero at the moment. if this is the bug the attackers are exploiting then maybe if they generated rewards it would stop the drain of funds. however, the fact the draining is still going on and the DAO people are likely to know how they are doing it and it hasn't been stopped reduces my confidence that this is how the attackers are doing it. EDIT: to add i don't think you can cash out the new DAO for 28 days so this is probably not how the attackers are doing it. EDIT: update again. https://blog.slock.it/dao-security-advisory-live-updates-2a0a42a2d07b#.ajsb7cpgv https://blog.slock.it/dao-security-advisory-live-updates-2a0... 'It would appear the attacker has moved the stolen ether to a child DAO, which means that the funds be moved for at least 27 days.' -> i'm now fairly confident this is how the attack worked :)

amluto 10y ago
This makes sense to me, but where's the actual code that calls into the attacker's contract and lets the attacker call back into splitDAO? And how on Earth is it a good idea to allow one contract to call another? There are any number of more sensible ways to communicate that don't have this problem: allow contracts to pass messages to other contracts, allow contracts to subscribe to each other's events, etc.
- benmmurphy 10y ago
  withdrawRewardFor ends up calling ManagedAccount#payOut which does _recipient.call.value(amount): 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; } } so the 'vulnerable' code is calling a function in another contract and it is this function that is doing the external callback. this is why security is so hard. everytime you add an external callback you need trace all of your callers to check that they are correctly re-entrant or everytime you use another function you need to trace forward to make sure it doesn't have any external callbacks.

amluto 10y ago

This makes sense to me, but where's the actual code that calls into the attacker's contract and lets the attacker call back into splitDAO? And how on Earth is it a good idea to allow one contract to call another? There are any number of more sensible ways to communicate that don't have this problem: allow contracts to pass messages to other contracts, allow contracts to subscribe to each other's events, etc.

benmmurphy 10y ago
withdrawRewardFor ends up calling ManagedAccount#payOut which does _recipient.call.value(amount): 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; } } so the 'vulnerable' code is calling a function in another contract and it is this function that is doing the external callback. this is why security is so hard. everytime you add an external callback you need trace all of your callers to check that they are correctly re-entrant or everytime you use another function you need to trace forward to make sure it doesn't have any external callbacks.