Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

Disallow calling processWithdrawCommitment twice #679

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

duckception
Copy link
Contributor

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.

@github-actions github-actions bot added the contracts This PR changes some contracts label Jan 4, 2022
@msieczko
Copy link

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

@jacque006
Copy link
Collaborator

I agree with @msieczko , resolving #664 would be a better path forward. @duckception Appreciate you taking a stab at trying to resolve this.

@duckception
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

contracts/client/FrontendMassMigration.sol Outdated Show resolved Hide resolved
contracts/libs/Transition.sol Outdated Show resolved Hide resolved
Copy link

@msieczko msieczko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good now! @jacque006, could you take a look? I think this PR is ready to be merged. We should also merge #684 right after this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contracts This PR changes some contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants