Skip to content
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

[Vulnerability] Contract can be drained if event cancelled #81

Closed
plutoegg opened this issue Sep 14, 2017 · 3 comments · Fixed by #83
Closed

[Vulnerability] Contract can be drained if event cancelled #81

plutoegg opened this issue Sep 14, 2017 · 3 comments · Fixed by #83

Comments

@plutoegg
Copy link

plutoegg commented Sep 14, 2017

function withdraw() public onlyPayable notPaid {
	Participant participant = participants[msg.sender];
	participant.paid = true;
	assert(msg.sender.send(payoutAmount));
	WithdrawEvent(msg.sender, payoutAmount);
}

modifier onlyPayable {
	Participant participant = participants[msg.sender];
	if (cancelled || (payoutAmount > 0 && participant.attended)){
		_;
	}
}

modifier notPaid {
	Participant participant = participants[msg.sender];
	if (participant.paid == false){
		_;
	}
}

If the owner cancels the event, anyone can drain the contract via the withdraw function, even if they were not a participant. onlyPayable will allow anyone to withdraw if the event is cancelled, since notPaid will return participant.paid as false by default for a participant who never registered.

This means that when the event is cancelled any address who did not register can withdraw and receive the payoutAmount until there are no funds left.

N.B. have not tested this

@makoto
Copy link
Owner

makoto commented Sep 14, 2017

Probably need to change to participant.send(payoutAmount)

@plutoegg
Copy link
Author

You don't want to risk sending it to 0x00 address (participant.addr will be 0x00) if they never registered. You might need to add a bool registered to Participants and set it to true on register?

Or you can do a check that participant.addr == msg.sender (which is set during registerInternal)

@makoto
Copy link
Owner

makoto commented Sep 14, 2017

True. For the currently deployed contract, do you see any issue progressing to withdraw phase, or should I rather destroy the contract?

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

Successfully merging a pull request may close this issue.

2 participants