search

thehubbleproject / hubble-contracts

Hubble optimistic rollup
https://thehubbleproject.github.io/docs/
MIT License
133 stars 28 forks source link

Disallow calling processWithdrawCommitment twice #679

Open duckception opened 2 years ago

duckception commented 2 years ago

Currently, anyone can call processWithdrawCommitment multiple times which may lead to permanent lock up of tokens in the WithdrawManager. This PR fixes it and removes redundant code from the Vault smart contract.

msieczko commented 2 years ago

I feel that this issue should be resolved together with #664 and we would come up with a better solution then.

jacque006 commented 2 years ago

I agree with @msieczko , resolving https://github.com/thehubbleproject/hubble-contracts/issues/664 would be a better path forward. @duckception Appreciate you taking a stab at trying to resolve this.

duckception commented 2 years ago

Issue described in #664 should be now resolved 🚀

Additionally, I have a question @jacque006. What is the purpose of validateAndApply functions in FrontendCreate2Transfer.sol and FrontendMassMigration.sol contracts?

duckception commented 2 years ago

We've discovered that changing just the nonce is not enough to prevent withdraw roots collision. Hubble allows registration of multiple user states for the same pubkey ID, therefore there still can be a case where withdraw root collision could happen (see modified test in rollup.massMigration.test.ts). To overcome this, I've added a new user state structure, just for mass migrations, that additionally takes into account state ID of the sender and is now used to generate collision-free withdraw root.