search

clearmatics / mobius

Trustless Tumbling for Transaction Privacy
GNU Lesser General Public License v3.0
86 stars 23 forks source link

Add ERC20 token support #11

Closed zoenolan closed 6 years ago

zoenolan commented 6 years ago

Still need a load more tests to check all the codepaths

zoenolan commented 6 years ago

The last two comments should be addressed now

HarryR commented 6 years ago

So, I'm looking at the difference between the two versions:

https://github.com/clearmatics/mobius/blob/28c390aff56a4d3e219d85abd871c485957ac6d2/contracts/Ring.sol (before refactor) https://github.com/clearmatics/mobius/blob/67ff3b0f4a3b2a8ead8daf21ff4ddde86a3238a8/contracts/Ring.sol (after refactor)

I think we should put the new one on hold for a bit and revert back to 28c390aff56a4d3e219d85abd871c485957ac6d2 because splitting withdraw into two parts has changed a handful of things and introduced some really weird bugs. The split into authoriseWithdraw and withdraw needs some more thought about if it's worth doing and what the best approach is and what the downsides are.

The first and most important thing I noticed is tagList.push(tagx); is missing from withdraw, this introduces a double-spend bug where I can run withdrawList[msg.sender] += PaymentAmount; multiple times etc. - I can use this to withdraw everybody else's ETH/tokens without their signatures as long as I have one valid signature in the ring.

Second thing is PaymentAmount = payments * 1 ether; is back

Third thing https://github.com/clearmatics/mobius/blob/67ff3b0f4a3b2a8ead8daf21ff4ddde86a3238a8/contracts/Ring.sol#L216 is it better to revert() the whole transaction, the token/eth failing to transfer is a catastrophic error and it doesn't feel right to add the balance back on then let the function call get committed forever.

And the last thing that's making me wonder is what happens if they do authoriseWithdraw but then don't do a withdraw, I guess it's the same as not doing a withdraw.

I think it's been worth looking into what splitting it into authorise and withdraw would be like, but I don't think it prevents denial of service (only that one persons funds would get stuck, and the ring would remain open), and I'm unsure about if transferFrom or send are still a real risk of re-entrancy bugs.

zoenolan commented 6 years ago

Yep agree with all that. The withdraw changes are also out of scope for the ERC20 token work anyway. I'll revert the code on this branch in the morning. Create a new branch just for the withdraw changes. We can continue to work on them there

zoenolan commented 6 years ago

Removed the two stage withdraw