makoto / blockparty

NO BLOCK NO PARTY
MIT License
164 stars 41 forks source link

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

Closed plutoegg closed 7 years ago

plutoegg commented 7 years ago
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 commented 7 years ago

Probably need to change to participant.send(payoutAmount)

plutoegg commented 7 years ago

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 commented 7 years ago

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