Fixes Issue #66 - Transfer DoS

[laskdaf]

Added mapping from address to balances. At each transfer() function, added transfer amount to the owner's current balance. Created withdraw function that transfers entire balance to the owner's address. This prevents attackers from stopping finalizeExit(). Further described in the issue.

[smartcontracts]

Here's a potential case that might be worth considering. Assume that this is a chain with very few users to make the impact more obvious:

1. Operator withholds blocks.
2. Everyone on the network submits exits at the same time. Every exit will be processed after the 2 week mark in very quick succession.
3. Alice's valid exit is processed before the operator's invalid exit, but the operator withdraws before Alice does.

As a result, the operator has stolen Alice's funds even though her transaction processed first.

A sufficiently motivated attacker could similarly find ways to censor withdrawal transactions on the root chain.

I think that the correct solution to this problem is to send ETH with address.send() instead of address.transfer(), check for failure, and add to withdrawals in the case that the fallback function throws. This inherently punishes contracts that throw in the fallback function but still maintains our security guarantees. Aka:

if (!currentExit.owner.send(currentExit.amount)) {
    balances[currentExit.owner] = balances[currentExit.owner].add(currentExit.amount);
}
...

You could then implement a system that:

1. Records the amount of ETH reserved for withdrawals.
2. Gives each user in the "withdrawal queue" a small window (~ 1 day) to withdraw.

This way we ensure that:

1. The only users who need to deal with this process are users that have implemented bad fallback functions.
2. The process will eventually pay out correctly to attentive users, even if it takes a long time.

We wouldn't want to implement this ordered withdrawal for every exit because that could take a ridiculous amount of time.

[laskdaf]

Would the small window be for the user at the front of the queue? And any after that time period, if the user didn't withdraw, someone could pop them off of the queue and the window would restart for the new withdrawer. We wouldn't want to punish users for not withdrawing just because someone in front of them hasn't acted.

[smartcontracts]

@laskdaf Yes, exactly. The person at the front of the queue would have a small time window to withdraw. If the user completes their exit within the window, then the next person's window begins. If the user fails to complete their exit within the window, then they're popped off the queue and the next person's window begins.

You could also move the person to the back of the queue rather than popping them off entirely, but this is more complex and probably pretty pointless if we're assuming that an attacker is submitting invalid exits.

[laskdaf]

Also, we need to keep track of the total amount allocated for failed withdraws. See the following case: Actors: Alice, Bob, Malicious validator Total Contract Balance: 100 Eth (Alice and Bob both currently hold 50 Eth each on the Plasma Chain)

1. The validator creates a block with an invalid transaction and withholds
2. Everyone on the network submits exits at the same time. Every exit will be processed after the 2 week mark in very quick succession.
3. Alice's valid exit is processed successfully. The Contract Balance is now 50 Eth
4. Bob's valid exit fails, and his balance is placed in the queue
5. The validator's invalid exit of a manufactured UTXO of 50 Eth is not challenged and is now processed. The Contract Balance is now 0 Eth. When Bob fixes his issue, and tries to withdraw, there's no Eth left in the contract.

A way to prevent this is to keep track of the total amount allocated for failed withdraws. Whenever a exit is finalized, we first check that amount <= Contract.balance - failedWithdrawBalance.

[josojo]

Here's a potential case that might be worth considering. Assume that this is a chain with very few users to make the impact more obvious: Operator withholds blocks. Everyone on the network submits exits at the same time. Every exit will be processed after the 2 week mark in very quick succession.

This is a good catch. I propose another solution than using send instead of transfer:

We could simply maintain variabel: virtualBalance in finalizeExits, which will be the current ether balance - the ether from all processed exit items + the already withdrawn items. In your example we would see that the virtualBalance would be <0, if we process the attackers withdraw request and then we not give him any ether to withdraw later.

[smartcontracts]

Closing this for now, I think it's probably fine to limit withdrawals to non-contract accounts for a non-production implementation.