thehubbleproject / hubble-contracts

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

Withdraw roots colliding #664

Open dmaretskyi opened 2 years ago

dmaretskyi commented 2 years ago

It look like WithdrawManager stores pending withdrawals using withdrawRoot which is the root of the merkle tree formed by new user states generated after mass-migration batch.

The states are formed without a nonce:

    Types.UserState memory state =
            Types.UserState({
                pubkeyID: pubkeyID,
                tokenID: tokenID,
                balance: amount,
                nonce: 0
            });

In that case, does this allow for a batch to be forged in a way that will cause a collision in withdrawRoot?

https://github.dev/thehubbleproject/hubble-contracts/blob/7058cba1a4e5a6251571f74a6a7f16de5b77e2a7/contracts/libs/Transition.sol#L217-L217

https://github.dev/thehubbleproject/hubble-contracts/blob/7058cba1a4e5a6251571f74a6a7f16de5b77e2a7/contracts/WithdrawManager.sol#L47-L47

dmaretskyi commented 2 years ago

The simplest case where such a collision might happen is two MM commitments, each with a single transaction. In that case withdraw root for each of them would be calculated with:

withdrawRoot = keccak256(Types.UserState({
    pubkeyID: pubkeyID,
    tokenID: tokenID,
    balance: amount,
    nonce: 0
}).encode());

Now, as long as pubkey id, token id, and amount are the same in both transactions, which is a valid case, then the withdraw roots would be the same for both commitments.