thehubbleproject / hubble-contracts

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

Strengthen the domain separator #587

Closed ChihChengLiang closed 3 years ago

ChihChengLiang commented 3 years ago

We thank @Marik-D for finding this issue

What's wrong

We currently have the domain separator to be the hash of the address of "Rollup.sol". It is possible the operator deploys rollup on other networks, so the new rollup has the same address and the same domain. Then a user's fund is at risk because the old txs and the old signatures can be replayed at the other networks.

https://github.com/thehubbleproject/hubble-contracts/blob/f23e37c2580ea1f40c08eafcecbba826ca041db3/contracts/rollup/Rollup.sol#L91

How can we fix it

List some possible solutions

string name               the user readable name of signing domain, i.e. the name of the DApp or the protocol.
string version            the current major version of the signing domain. Signatures from different versions are not compatible.
uint256 chainId           the EIP-155 chain id. The user-agent should refuse signing if it does not match the currently active chain.
address verifyingContract the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.
bytes32 salt              an disambiguating salt for the protocol. This can be used as a domain separator of last resort.

Here's an example of Uniswap's domain separator

https://github.com/Uniswap/uniswap-v2-periphery/blob/dda62473e2da448bc9cb8f4514dadda4aeede5f4/contracts/test/DeflatingERC20.sol#L23-L36

and its JS implementation

https://github.com/Uniswap/uniswap-v2-periphery/blob/dda62473e2da448bc9cb8f4514dadda4aeede5f4/test/shared/utilities.ts#L15-L28