Fixes Issue #66 - Transfer DoS 2nd Proposal

[josojo]

This PR solves the issue #66.

It proposes to prevent a possible DoS withdraw attack, where an attacker prevents everybody to withdraw any funds from the root-contract by making this withdraw request throw all the time.

This is prevented by avoiding the transfer function during the finalizeExit call and only crediting balances, which can be withdrawn later. In order to prevent any race on the withdraw requests of the balance, it is ensured that never more balances are credited, as there are funds in the contract available.

For this, I calculate the virtualBalance = sum of all deposits - sum of all withdraw request went through finalizeExit. If virtualBalance < 0, then the chain operator was malicious and in this case stop the finalizeExit method.

While the proposed switch of transfer to send would also work for Ether, I prefer my solution, as it would also work for generic ERC20 Tokens.

[smartcontracts]

I think you're missing the check that virtualBalance > 0.

Another (less important) case: if the attacker ever receives an invalid balance, then they'll effectively be able to immediately steal from anyone who deposits into the contract in the future. Note that we can probably assume that if the attacker has an invalid balance, then the chain is byzantine and people probably shouldn't be using it. The current implementation also has this problem.

[josojo]

Thanks for reviewing my PR.

I think you're missing the check that virtualBalance > 0.

It throws here virtualBalance = virtualBalance.sub(currentExit.amount); if the virtualBalance would turn negative. I know, its not brilliant, since then all executed code is reverted. But since the while loop needs anyway some more optimiazation in order to not run out of gas, I thought I would leave this to another PR. There needs to be some restriction on how many loops are taken in the while loop, in order to not run out of gas. Depending on how this restriction is chosen, we can also check virtualBalance>0.

Another (less important) case: if the attacker ever receives an invalid balance, then they'll effectively be able to immediately steal from anyone who deposits into the contract in the future. Note that we can probably assume that if the attacker has an invalid balance, then the chain is byzantine and people probably shouldn't be using it. The current implementation also has this problem.

I thought exactly about the same problem. Unfortunately, there is no easy solution that crosses my mind. Currently users first have to check that the plasma chain operates well. But the plasma operator could also wait to become malicious, as a high deposit to the plasma chain is on the fly, but not yet confirmed by the root chain.

[smartcontracts]

@josojo my apologies - you're right re: the virtual balance. I didn't catch that. I'm going to get in touch with @DavidKnott and talk about how we want to handle the transfer DoS. This solution seems to be the most complete.

About the second problem, let's not worry about it yet. I think we can currently operate under the assumption that users will exit permanently if something goes wrong. It's an interesting to think about for the future, so I'm going to make a new issue over at https://github.com/omisego/research.

[smartcontracts]

Also closing for now, as per #67. I think this is something we can come back to for production work.