-
Notifications
You must be signed in to change notification settings - Fork 80
feat: move to single signature ERC-3009 and use deterministic depositIds for gasless deposits #1275
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Matt Rice <[email protected]>
fusmanii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just one Q
contracts/SpokePoolPeriphery.sol
Outdated
| bytes32 witness = keccak256( | ||
| abi.encodePacked(BRIDGE_AND_SWAP_WITNESS_IDENTIFIER, abi.encode(swapAndDepositData)) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we ensure witness uniqueness given that its now obtained from deposit data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the token implementing ERC-3009 will revert on the receiveWithAuthorization call if the witness has already been used
Also there's a nonce in SwapAndDepositData thats part of what gets hashed to generate the witness - so this can create uniqueness even if all the other intent data is identical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my undertsanding was the same as Taylor's, witness here is essentially the nonce for the ERC-3009 token's permit with auth
From https://eips.ethereum.org/EIPS/eip-3009 reference implementation:
require(
!_authorizationStates[from][nonce],
"EIP3009: authorization is used"
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with the above. There is nonce reuse prevention that is per-user. The nonce within the hashed data means that a user should never have an identical witness hash unless they are accidentally reusing the same API response twice.
Signed-off-by: Ihor Farion <[email protected]>
|
@mrice32 one other Q: do we need to explicitly use domainSeparator in our witness? |
I don't think so. I think the EIP-712 related info is only needed in the outermost layer: the 3009 data that's being signed (which contains the witness). |
Signed-off-by: Matt Rice <[email protected]>
Signed-off-by: Matt Rice <[email protected]>
contracts/SpokePoolPeriphery.sol
Outdated
| depositData.baseDepositData.outputAmount, | ||
| depositData.baseDepositData.destinationChainId, | ||
| depositData.baseDepositData.exclusiveRelayer, | ||
| depositData.nonce, // TODO: should we hash this nonce (if nonzero) with the permit2 nonce to protect the user against nonce reuse? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grasphoper @fusmanii @tbwebb22 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, hashing it together sounds right to me. Then the relayer would know to pre-fill with the hash that they can derive easily right
contracts/SpokePoolPeriphery.sol
Outdated
| depositData.baseDepositData.outputAmount, | ||
| depositData.baseDepositData.destinationChainId, | ||
| depositData.baseDepositData.exclusiveRelayer, | ||
| depositData.nonce + 1, // +1 to avoid 0 (which triggers regular deposit) and ensure uniqueness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is needed. If the relayer is doing a pre-fill and a deposit after, wouldn't they just submit a non-zero nonce? Feels like it should be their responsibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I take it back, the user signed over depositData. Wouldn't we, then, trigger a lot of deposits to have nonce = 1?
I like the solution proposed for the Permit2: hashing this with the permit nonce seems like it'd ensure uniqueness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why are we forcing the user to have a non-zero nonce here, but not in Permit2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point -- let me see if we can allow 0 nonces on all of them.
Signed-off-by: Matt Rice <[email protected]>
Signed-off-by: Matt Rice <[email protected]>
Signed-off-by: Matt Rice <[email protected]>
| function _validateAndIncrementNonce(address user, uint256 providedNonce) private { | ||
| if (permitNonces[user] != providedNonce) { | ||
| // Pre-increment nonce so it can never be 0. | ||
| if (++permitNonces[user] != providedNonce) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in future if we re-deploy the periphery contract these nonces will reset and there could be collisions right? might make sense to version the identifiers or have a global version number that gets hashed in _deposit()
| _validateAndIncrementNonce(signatureOwner, depositData.nonce); | ||
| // Verify that the signatureOwner signed the input depositData. | ||
| _validateSignature(signatureOwner, PeripherySigningLib.hashDepositData(depositData), depositDataSignature); | ||
| // Use nonce + 1 to avoid 0 (which triggers regular deposit) and ensure uniqueness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is no longer accurate
This PR makes two primary changes that have some ripple effects on the
SpokePoolPeripherycontract:In addition to this, we now do not ignore the nonce passed in the structs. For user-submitted deposits, we require a 0 nonce (which signals an incrementing depositId using the normal deposit method). For Permit2, we now require that the permit2 nonce match the nonce in the witness data, so they can be interchangeably used to compute depositId (reduce confusion).
Relatedly, we also add some convenience methods for computing depositId (and the witness value) to enable easy prefills.