feat: eip-712 support #222

dohaki · 2023-01-23T13:23:43Z

Closes ACX-590

This allows the usage of EIP-712 structured and typed data signatures for updating relayer fees. The implementation of EIP712CrossChain is based on OZ's implementation of the EIP-712 specification but is less strict to allow the verification of signatures cross-chain.

linear · 2023-01-23T13:23:45Z

ACX-590 Add support for EIP712

Instead of a cryptic bytestring, the user would be presented with a human-readable data before signing the message.

https://eips.ethereum.org/EIPS/eip-712

We should only implement this if there a tested library, for example one from OZ we can use. If we had to roll our own implementation then the risk wouldn't be worth it IMO

contracts/SpokePool.sol

nicholaspai · 2023-01-23T15:46:40Z

contracts/EIP712CrossChain.sol

+    bytes32 private immutable _HASHED_VERSION;
+    bytes32 private immutable _TYPE_HASH;
+    // NOTE: We use a hardcoded address to allow cross-chain verification
+    address public immutable _VERIFYING_CONTRACT = 0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC;


Would we need to change this in production? Isn't this contract the destination SpokePool's address?

This is again the same problem as with the chain id. Because this value is part of the domain separator, we either need to hardcode it across all chains / spoke pool contracts OR make it also a parameter. I decided to hardcode it because:

The spec defines this only with the keyword may, so my interpretation is that this is optional. See https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator

If this would be a parameter, then the function signatures of the methods speedUpDeposit and fillRelayWithUpdatedFee would need to change. Because we would need to somehow pass the address of the respective spoke pool (same value for corresponding chains)?

hmmm this is an interesting point @dohaki . So one way around this is to just hardcode the verifying contract so that its essentially ignored. Does that break wallet clients ability to parse this message? I just don't want them to reject it because the verifyingContract isn't actually the contract that is verifying it

As discussed offline, it seems that some of the fields like verifyingContract are optional in the EIP so we don't need to use that one. I still think we should keep chainId = originChainId. If we remove verifyingContract i don't think we expose ourselves to any replay attacks where someone submits the signature to a different spoke pool on the originChain. The SpokePools now allow fills to be paused including the fillRelayWithUpdatedFee so that will revert on deprecated SpokePools. Additionally, even if the fillRelayWithUpdatedFee succeeds, the relayer will note receive a refund if the SpokePool isn't connected to the HubPool anymore.

nicholaspai

Left some high level comments !

nicholaspai · 2023-01-24T18:42:30Z

test/fixtures/SpokePool.Fixture.ts

+      originChainId,
+    },
+  };
+  const signature = await depositor._signTypedData(typedData.domain, typedData.types, typedData.message);


This is how we'd sign the speed up in the FE after this change goes through? Is there a non underscore function we can use instead?

Do we need any additional unit tests? Like verifying that the typed data has the expected fields and can be parsed by a wallet client?

This is how we'd sign the speed up in the FE after this change goes through? Is there a non underscore function we can use instead?

Yes, we would use this in the FE to speed up. This is just a wrapper method provided by ethers and will eventually be replaced by signTypedData without an underscore. Alternatively, we could just directly use the JSON RPC like:

const signature = await depositor.provider.send("eth_signTypedData_v4", [fromAddress, encodedMsgParams]

where we need to do some additional encoding. This approach is a bit verbose though.

Do we need any additional unit tests? Like verifying that the typed data has the expected fields and can be parsed by a wallet client?

Not sure if I understand correctly. Verifying whether the typed data has the expected fields is already covered in our tests, no? Basically, the test case is where if the signed message is not equal to the expected message then the verification fails. I am also not sure how to write a unit test on whether or not a wallet client can parse the typed data. Doesn't this depend on the implementation of the wallet? And can't we assume that if a wallet supports the method eth_signTypedData_v4 then it can parse any message supported by the spec?

eventually be replaced by signTypedData without an underscore

Any idea when this is going to happen? Not that it blocks us i'm just curious what's holding this up

Verifying whether the typed data has the expected fields is already covered in our tests, no? Basically, the test case is where if the signed message is not equal to the expected message then the verification fails

Do we need to perform additional checks that the signed message is in the correct EIP712 format?

eventually be replaced by signTypedData without an underscore

Any idea when this is going to happen? Not that it blocks us i'm just curious what's holding this up

No idea tbh... Couldn't find anything on this in the repo.

Verifying whether the typed data has the expected fields is already covered in our tests, no? Basically, the test case is where if the signed message is not equal to the expected message then the verification fails

Do we need to perform additional checks that the signed message is in the correct EIP712 format?

Is this possible? The signature is in the end just the same bytes string as a message signed with signer.signMessage.

mrice32

LGTM!

contracts/EIP712CrossChain.sol

Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>

james-a-morris

Left some comments.

contracts/EIP712CrossChain.sol

contracts/SpokePool.sol

james-a-morris · 2023-01-26T14:40:55Z

contracts/SpokePool.sol

+            keccak256(
+                abi.encode(
+                    keccak256(
+                        "UpdateRelayerFeeMessage(uint64 newRelayerFeePct,uint32 depositId,uint256 originChainId)"
+                    ),
+                    newRelayerFeePct,
+                    depositId,
+                    originChainId
+                )
+            ),


Would it make sense to store some of these values in local variables with descriptive names? From a readability standpoint, there are several nested function calls that may not be entirely clear for a newcomer to the repo.

Yea, I agree that this is a bit nested. But tbh I don't know what to put in local variables because this hash structure is defined like that in the EIP. I could do

bytes32 hashStruct = keccak256(...); bytes32 expectedTypedDataV4Hash = _hashTypedDataV4(hashStruct);

but not sure if this makes it really more readable?

Is there a gas difference if you change the syntax? Can run the test like so REPORT_GAS=true hardhat test

@nicholaspai Thanks for the suggestion. Yes, with the additional local variable the methods speedUpDeposit and fillRelayWithUpdatedFee cost 10 gas more.

this is an interesting case where smart contract engineering is different from off-chain engineering. Temp variables increase readability but incur a real cost! In this case I'd opt for the reduction in readability to save gas

amateima

Nice work 👍

james-a-morris

Great work 🔥 Thanks for addressing my comments

dohaki · 2023-01-31T15:22:16Z

@nicholaspai I e2e tested a deployed version of the contract with the frontend and everything seems fine 👍 When you are happy, then I can merge

nicholaspai · 2023-02-01T15:48:00Z

@nicholaspai I e2e tested a deployed version of the contract with the frontend and everything seems fine 👍 When you are happy, then I can merge

#222 (comment)

Just one comment then LGTM

nicholaspai

#222 (comment)

dohaki · 2023-02-01T16:32:25Z

#222 (comment)

@nicholaspai Thanks! I haven't actually put the nested stuff in a local variable. So this PR has the version with lower gas costs

dohaki requested review from amateima, james-a-morris, mrice32 and nicholaspai January 23, 2023 13:23

dohaki force-pushed the eip-712-support branch from adfcd42 to 0a407b2 Compare January 23, 2023 13:24

nicholaspai reviewed Jan 23, 2023

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Jan 23, 2023

View reviewed changes

dohaki force-pushed the eip-712-support branch from 8be9cc1 to e8a9c8d Compare January 24, 2023 10:15

dohaki requested a review from nicholaspai January 24, 2023 10:15

nicholaspai reviewed Jan 24, 2023

View reviewed changes

mrice32 approved these changes Jan 25, 2023

View reviewed changes

contracts/EIP712CrossChain.sol Show resolved Hide resolved

dohaki added 5 commits January 26, 2023 11:07

feat: eip-712 support

3315015

Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>

refactor: pass domain name and version to abstract SpokePool

230b6d3

Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>

fixup

1f07b8e

Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>

refactor: remove optional verifyingContract type

9ee37be

Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>

refactor: combine domain separator build methods

21aab9c

Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>

dohaki force-pushed the eip-712-support branch from e87158f to 21aab9c Compare January 26, 2023 10:07

dohaki requested a review from nicholaspai January 26, 2023 10:07

james-a-morris reviewed Jan 26, 2023

View reviewed changes

amateima approved these changes Jan 26, 2023

View reviewed changes

docs: add natspec params and return to EIP712CrossChain

566b4dd

dohaki requested a review from james-a-morris January 27, 2023 08:42

james-a-morris approved these changes Jan 30, 2023

View reviewed changes

nicholaspai reviewed Feb 1, 2023

View reviewed changes

nicholaspai approved these changes Feb 3, 2023

View reviewed changes

dohaki merged commit 74a239f into master Feb 3, 2023

dohaki deleted the eip-712-support branch February 3, 2023 15:49

feat: eip-712 support #222

feat: eip-712 support #222

Uh oh!

Conversation

dohaki commented Jan 23, 2023

Uh oh!

linear bot commented Jan 23, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

james-a-morris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amateima left a comment

Choose a reason for hiding this comment

Uh oh!

james-a-morris left a comment

Choose a reason for hiding this comment

Uh oh!

dohaki commented Jan 31, 2023

Uh oh!

nicholaspai commented Feb 1, 2023

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

dohaki commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants