New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect against recursive withdrawRewardFor attack #242

Merged
merged 1 commit into from Jun 13, 2016

Conversation

Projects
None yet
7 participants
@LefterisJP
Contributor

LefterisJP commented Jun 12, 2016

The DAO code is vulnerable to the recursive call attack in the withdrawRewardFor() function. This fix should handle it for the next deployment of the DAO code.

A very big thank you to eththrowa from the daohub forums for spotting this.

@LefterisJP LefterisJP added this to the DAO v1.1 milestone Jun 12, 2016

@CJentzsch CJentzsch merged commit a188e9e into slockit:master Jun 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@glesaint

This comment has been minimized.

Show comment
Hide comment
@glesaint

glesaint Jun 17, 2016

So the issue was identified, the solution developed, but no one could implement it. How to setup DAOs to allow security hot-fixes?

glesaint commented Jun 17, 2016

So the issue was identified, the solution developed, but no one could implement it. How to setup DAOs to allow security hot-fixes?

@craigcalef

This comment has been minimized.

Show comment
Hide comment
@craigcalef

craigcalef Jun 18, 2016

Too bad this didn't get in before the Eth walked away. :(

craigcalef commented on f01f3bd Jun 18, 2016

Too bad this didn't get in before the Eth walked away. :(

This comment has been minimized.

Show comment
Hide comment
@nukec

nukec Apr 5, 2018

well sadly, i think this might've given an idea to the attacker in the first place. security fixes should've been reviewed and committed immediately.

nukec replied Apr 5, 2018

well sadly, i think this might've given an idea to the attacker in the first place. security fixes should've been reviewed and committed immediately.

@ktorn

This comment has been minimized.

Show comment
Hide comment
@ktorn

ktorn Jun 21, 2016

Any comments on why this didn't get deployed urgently?
Especially taking into account that the attacker had already initiated his attack preparation 7 days in advance with the split proposal.

ktorn commented Jun 21, 2016

Any comments on why this didn't get deployed urgently?
Especially taking into account that the attacker had already initiated his attack preparation 7 days in advance with the split proposal.

@LefterisJP

This comment has been minimized.

Show comment
Hide comment
@LefterisJP

LefterisJP Jun 21, 2016

Contributor

@ktorn The fix for the actual attack that happened is not yet in the v1.1 repo.
The reason that the fix in this PR or ... any of the v1.1 fixes did not make it into the DAO is simple. The DAO is a completely decentralized entity out of anyone's control.

The upgrade mechanism is slow and involves a vote of the token holders which would take at least 2 weeks.

Contributor

LefterisJP commented Jun 21, 2016

@ktorn The fix for the actual attack that happened is not yet in the v1.1 repo.
The reason that the fix in this PR or ... any of the v1.1 fixes did not make it into the DAO is simple. The DAO is a completely decentralized entity out of anyone's control.

The upgrade mechanism is slow and involves a vote of the token holders which would take at least 2 weeks.

@@ -744,9 +744,10 @@ contract DAO is DAOInterface, Token, TokenCreation {
reward = rewardAccount.balance < reward ? rewardAccount.balance : reward;
paidOut[_account] += reward;
if (!rewardAccount.payOut(_account, reward))
throw;

This comment has been minimized.

@ithinuel

ithinuel Jun 23, 2016

If the payOut failed, should'nt it be removed from paidOut[_account] ?

@ithinuel

ithinuel Jun 23, 2016

If the payOut failed, should'nt it be removed from paidOut[_account] ?

This comment has been minimized.

@LefterisJP

LefterisJP Jun 23, 2016

Contributor

If it failed, the transaction would throw, so this would be as if it never happened.

@LefterisJP

LefterisJP Jun 23, 2016

Contributor

If it failed, the transaction would throw, so this would be as if it never happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment