-
Notifications
You must be signed in to change notification settings - Fork 75
feat: eip-712 support #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3315015
230b6d3
1f07b8e
9ee37be
21aab9c
566b4dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-only | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; | ||
|
|
||
| /** | ||
| * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. | ||
| * | ||
| * This contract is based on OpenZeppelin's implementation: | ||
| * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.0/contracts/utils/cryptography/EIP712.sol | ||
| * | ||
| * NOTE: Modified version that allows to build a domain separator that relies on a different chain id than the chain this | ||
| * contract is deployed to. An example use case we want to support is: | ||
| * - User A signs a message on chain with id = 1 | ||
| * - User B executes a method by verifying user A's EIP-712 compliant signature on a chain with id != 1 | ||
| */ | ||
| abstract contract EIP712CrossChain { | ||
| /* solhint-disable var-name-mixedcase */ | ||
| bytes32 private immutable _HASHED_NAME; | ||
| bytes32 private immutable _HASHED_VERSION; | ||
| bytes32 private immutable _TYPE_HASH; | ||
|
|
||
| /* solhint-enable var-name-mixedcase */ | ||
|
|
||
| /** | ||
| * @dev Initializes the domain separator and parameter caches. | ||
| * | ||
| * The meaning of `name` and `version` is specified in | ||
| * https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator[EIP 712]: | ||
| * | ||
| * - `name`: the user readable name of the signing domain, i.e. the name of the DApp or the protocol. | ||
| * - `version`: the current major version of the signing domain. | ||
| * | ||
| * NOTE: These parameters cannot be changed except through a xref:learn::upgrading-smart-contracts.adoc[smart | ||
| * contract upgrade]. | ||
| */ | ||
| constructor(string memory name, string memory version) { | ||
| bytes32 hashedName = keccak256(bytes(name)); | ||
| bytes32 hashedVersion = keccak256(bytes(version)); | ||
| bytes32 typeHash = keccak256("EIP712Domain(string name,string version,uint256 chainId)"); | ||
| _HASHED_NAME = hashedName; | ||
| _HASHED_VERSION = hashedVersion; | ||
| _TYPE_HASH = typeHash; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Returns the domain separator depending on the `originChainId`. | ||
| * @param originChainId Chain id of network where message originates from. | ||
| * @return bytes32 EIP-712-compliant domain separator. | ||
| */ | ||
| function _domainSeparatorV4(uint256 originChainId) internal view returns (bytes32) { | ||
| return keccak256(abi.encode(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION, originChainId)); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Given an already https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct[hashed struct], this | ||
| * function returns the hash of the fully encoded EIP712 message for this domain. | ||
| * | ||
| * This hash can be used together with {ECDSA-recover} to obtain the signer of a message. For example: | ||
| * | ||
| * ```solidity | ||
| * bytes32 structHash = keccak256(abi.encode( | ||
| * keccak256("Mail(address to,string contents)"), | ||
| * mailTo, | ||
| * keccak256(bytes(mailContents)) | ||
| * )); | ||
| * bytes32 digest = _hashTypedDataV4(structHash, originChainId); | ||
| * address signer = ECDSA.recover(digest, signature); | ||
| * ``` | ||
| * @param structHash Hashed struct as defined in https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct. | ||
| * @param originChainId Chain id of network where message originates from. | ||
| * @return bytes32 Hash digest that is recoverable via `EDCSA.recover`. | ||
| */ | ||
| function _hashTypedDataV4(bytes32 structHash, uint256 originChainId) internal view virtual returns (bytes32) { | ||
| return ECDSA.toTypedDataHash(_domainSeparatorV4(originChainId), structHash); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,11 +5,11 @@ import "./MerkleLib.sol"; | |
| import "./interfaces/WETH9.sol"; | ||
| import "./Lockable.sol"; | ||
| import "./SpokePoolInterface.sol"; | ||
| import "./EIP712CrossChain.sol"; | ||
|
|
||
| import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||
| import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
| import "@openzeppelin/contracts/utils/Address.sol"; | ||
|
|
||
| import "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; | ||
| import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; | ||
| import "@uma/core/contracts/common/implementation/Testable.sol"; | ||
|
|
@@ -24,7 +24,7 @@ import "@uma/core/contracts/common/implementation/MultiCaller.sol"; | |
| * Relayers are refunded with destination tokens out of this contract after another off-chain actor, a "data worker", | ||
| * submits a proof that the relayer correctly submitted a relay on this SpokePool. | ||
| */ | ||
| abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCaller { | ||
| abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCaller, EIP712CrossChain { | ||
| using SafeERC20 for IERC20; | ||
| using Address for address; | ||
|
|
||
|
|
@@ -144,7 +144,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall | |
| address _hubPool, | ||
| address _wrappedNativeTokenAddress, | ||
| address timerAddress | ||
| ) Testable(timerAddress) { | ||
| ) Testable(timerAddress) EIP712CrossChain("ACROSS-V2", "1.0.0") { | ||
| numberOfDeposits = _initialDepositId; | ||
| _setCrossDomainAdmin(_crossDomainAdmin); | ||
| _setHubPool(_hubPool); | ||
|
|
@@ -348,7 +348,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall | |
| * @param depositId Deposit to update fee for that originated in this contract. | ||
| * @param depositorSignature Signed message containing the depositor address, this contract chain ID, the updated | ||
| * relayer fee %, and the deposit ID. This signature is produced by signing a hash of data according to the | ||
| * EIP-1271 standard. See more in the _verifyUpdateRelayerFeeMessage() comments. | ||
| * EIP-712 standard. See more in the _verifyUpdateRelayerFeeMessage() comments. | ||
| */ | ||
| function speedUpDeposit( | ||
| address depositor, | ||
|
|
@@ -446,7 +446,9 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall | |
| * @param relayerFeePct Original fee % to keep as relayer set by depositor. | ||
| * @param newRelayerFeePct New fee % to keep as relayer also specified by depositor. | ||
| * @param depositId Unique deposit ID on origin spoke pool. | ||
| * @param depositorSignature Depositor-signed message containing updated fee %. | ||
| * @param depositorSignature Signed message containing the depositor address, this contract chain ID, the updated | ||
| * relayer fee %, and the deposit ID. This signature is produced by signing a hash of data according to the | ||
| * EIP-712 standard. See more in the _verifyUpdateRelayerFeeMessage() comments. | ||
| */ | ||
| function fillRelayWithUpdatedFee( | ||
| address depositor, | ||
|
|
@@ -698,20 +700,26 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall | |
| ) internal view { | ||
| // A depositor can request to speed up an un-relayed deposit by signing a hash containing the relayer | ||
| // fee % to update to and information uniquely identifying the deposit to relay. This information ensures | ||
| // that this signature cannot be re-used for other deposits. The version string is included as a precaution | ||
| // in case this contract is upgraded. | ||
| // Note: we use encode instead of encodePacked because it is more secure, more in the "warning" section | ||
| // here: https://docs.soliditylang.org/en/v0.8.11/abi-spec.html#non-standard-packed-mode | ||
| bytes32 expectedDepositorMessageHash = keccak256( | ||
| abi.encode("ACROSS-V2-FEE-2.0", newRelayerFeePct, depositId, originChainId) | ||
| // that this signature cannot be re-used for other deposits. | ||
| // Note: We use the EIP-712 (https://eips.ethereum.org/EIPS/eip-712) standard for hashing and signing typed data. | ||
| // Specifically, we use the version of the encoding known as "v4", as implemented by the JSON RPC method | ||
| // `eth_signedTypedDataV4` in MetaMask (https://docs.metamask.io/guide/signing-data.html). | ||
| bytes32 expectedTypedDataV4Hash = _hashTypedDataV4( | ||
| // EIP-712 compliant hash struct: https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct | ||
| keccak256( | ||
| abi.encode( | ||
| keccak256( | ||
| "UpdateRelayerFeeMessage(uint64 newRelayerFeePct,uint32 depositId,uint256 originChainId)" | ||
| ), | ||
| newRelayerFeePct, | ||
| depositId, | ||
| originChainId | ||
| ) | ||
| ), | ||
|
Comment on lines
+709
to
+718
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a gas difference if you change the syntax? Can run the test like so
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nicholaspai Thanks for the suggestion. Yes, with the additional local variable the methods
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn 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 |
||
| // By passing in the origin chain id, we enable the verification of the signature on a different chain | ||
| originChainId | ||
| ); | ||
|
|
||
| // Check the hash corresponding to the https://eth.wiki/json-rpc/API#eth_sign[eth_sign] | ||
| // If the depositor signed a message with a different updated fee (or any other param included in the | ||
| // above keccak156 hash), then this will revert. | ||
| bytes32 ethSignedMessageHash = ECDSA.toEthSignedMessageHash(expectedDepositorMessageHash); | ||
|
|
||
| _verifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); | ||
| _verifyDepositorUpdateFeeMessage(depositor, expectedTypedDataV4Hash, depositorSignature); | ||
| } | ||
|
|
||
| // This function is isolated and made virtual to allow different L2's to implement chain specific recovery of | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,15 @@ export async function deploySpokePool(ethers: any): Promise<{ | |
| // ERC1271 | ||
| const erc1271 = await (await getContractFactory("MockERC1271", deployerWallet)).deploy(deployerWallet.address); | ||
|
|
||
| return { timer, weth, erc20, spokePool, unwhitelistedErc20, destErc20, erc1271 }; | ||
| return { | ||
| timer, | ||
| weth, | ||
| erc20, | ||
| spokePool, | ||
| unwhitelistedErc20, | ||
| destErc20, | ||
| erc1271, | ||
| }; | ||
| } | ||
|
|
||
| export interface DepositRoute { | ||
|
|
@@ -297,17 +305,28 @@ export async function modifyRelayHelper( | |
| depositId: string, | ||
| originChainId: string, | ||
| depositor: SignerWithAddress | ||
| ): Promise<{ messageHash: string; signature: string }> { | ||
| const messageHash = ethers.utils.keccak256( | ||
| defaultAbiCoder.encode( | ||
| ["string", "uint64", "uint32", "uint32"], | ||
| ["ACROSS-V2-FEE-2.0", modifiedRelayerFeePct, depositId, originChainId] | ||
| ) | ||
| ); | ||
| const signature = await depositor.signMessage(ethers.utils.arrayify(messageHash)); | ||
|
|
||
| ): Promise<{ signature: string }> { | ||
| const typedData = { | ||
| types: { | ||
| UpdateRelayerFeeMessage: [ | ||
| { name: "newRelayerFeePct", type: "uint64" }, | ||
| { name: "depositId", type: "uint32" }, | ||
| { name: "originChainId", type: "uint256" }, | ||
| ], | ||
| }, | ||
| domain: { | ||
| name: "ACROSS-V2", | ||
| version: "1.0.0", | ||
| chainId: Number(originChainId), | ||
| }, | ||
| message: { | ||
| newRelayerFeePct: modifiedRelayerFeePct, | ||
| depositId, | ||
| originChainId, | ||
| }, | ||
| }; | ||
| const signature = await depositor._signTypedData(typedData.domain, typedData.types, typedData.message); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 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.
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Any idea when this is going to happen? Not that it blocks us i'm just curious what's holding this up
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do we need to perform additional checks that the signed message is in the correct EIP712 format?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No idea tbh... Couldn't find anything on this in the repo.
Is this possible? The signature is in the end just the same bytes string as a message signed with |
||
| return { | ||
| messageHash, | ||
| signature, | ||
| }; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.