feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

nicholaspai · 2024-08-20T15:08:41Z

Motivation

We should allow a depositor to pre-compute their relay data hash so that they can work with a filler to more quickly fill their deposit while avoiding re-org risk (which could change the ordering of V3FundsDeposited events and change a depositor's expected depositId) and race conditions with another innocent or malicious depositor who front-runs their depositId.

The advantage of using the global depositId as a nonce today is gas cost savings since we don't need to store a new value in a new slot on each deposit. We should not add gas cost to deposits.

Secondly, incrementing the global depositId after each deposit guarantees that each depositV3() produces a unique deposit. This means that a depositor does not have to worry about accidentally producing a relay hash collision that means their deposit would be unfillable. Fortunately, unfillable deposits are refunded on the origin chain to the depositor but this possibility is undesirable for many depositors.

Therefore, any implementation of deterministic relay hashes should ideally be opt-in.

Implementation

Introduces a new function: unsafeDepositV3() which gives the msg.sender 12 bytes with which to set a deposit nonce. It is marked unsafe because there is no protection given by the contract that the resultant relay data hash produced will be unique. The 12 byte deposit nonce is combined with the msg.sender's address, allowing each msg.sender to only track their own deposit nonces to avoid collisions. This also protects depositors from being griefed by malicious actors who might try to front-run their unsafe relay data hashes.

unsafeDepositV3() is slightly more expensive than depositV3 from a gas-cost perspective because it adds a keccak256 + abi.encodePacked operation but it removes a SSTORE

The existing depositV3() function's implementation is untouched.

Changes to V3RelayData struct

Changes the depositId parameter in the V3RelayData struct from uint32 to uint256. In storage, this parameter is stored as a uint256 so at the low-level, this changes nothing. Moreover, in practice until now, depositId's were always set as uint32 meaning that the first 24 bytes of the uint256 depositId slot are currently set to 0.

This change is made because this PR introduces a new function: unsafeDepositV3() which allows someone to set a depositId with non-zero bytes in the first 24 bytes.

Implications for clients

Any logic that assumes that depositId's will always be monotonically increasing now needs to be changed. For example, the queryHistoricalDepositForFill function performs a binary search on V3FundsDeposited events' depositId values to locate historical fills. This clearly will no longer work following this PR, so this corresponding change in the relayer client skips this function in all cases unless the deposit was sent via a legacy function where all deposit ID's are produced "safely": across-protocol/relayer#1769

## Motivation We should allow a depositor to pre-compute their relay data hash so that they can work with a filler to more quickly fill their deposit while avoiding re-org risk (which could change the ordering of V3FundsDeposited events and change a depositor's expected `depositId`) and race conditions with another innocent or malicious depositor who front-runs their depositId. The advantage of using the global `depositId` as a nonce today is gas cost savings since we don't need to store a new value in a new slot on each deposit. We should not add gas cost to deposits. Secondly, incrementing the global `depositId` after each deposit guarantees that each `depositV3()` produces a unique deposit. This means that a depositor does not have to worry about accidentally producing a relay hash collision that means their deposit would be unfillable. Fortunately, unfillable deposits are refunded on the origin chain to the depositor but this possibility is undesirable for many depositors. Therefore, any implementation of deterministic relay hashes should ideally be opt-in. ## Implementation Introduces a new function: `unsafeDepositV3()` which gives the msg.sender 12 bytes with which to set a deposit nonce. It is marked unsafe because there is no protection given by the contract that the resultant relay data hash produced will be unique. The 12 byte deposit nonce is combined with the msg.sender's address, allowing each msg.sender to only track their own deposit nonces to avoid collisions. This also protects depositors from being griefed by malicious actors who might try to front-run their unsafe relay data hashes. `unsafeDepositV3()` is no more expensive than `depositV3` from a gas-cost perspective. The existing `depositV3()` function's implementation is untouched. ## Changes to V3RelayData struct Changes the `depositId` parameter in the `V3RelayData` struct from uint32 to uint256. In storage, this parameter is stored as a uint256 so at the low-level, this changes nothing. Moreover, in practice until now, depositId's were always set as uint32 meaning that the first 24 bytes of the uint256 `depositId` slot are currently set to 0. This change is made because this PR introduces a new function: `unsafeDepositV3()` which allows someone to set a `depositId` with non-zero bytes in the first 24 bytes.

nicholaspai · 2024-08-26T14:11:31Z

Some off chain work related to this pr:

improve(BundleDataClient): Skip queryHistoricalDepositForFill function for non-infinite expiry deposits relayer#1769

It will be necessary to change all offchain code that tries to query deposits using deposit Id since we can no longer assume that deposit id is monotonically increasing for each deposit

ankitchiplunkar · 2024-09-13T12:19:04Z

contracts/SpokePool.sol

+     * all parameters to this function along with this chain's chainId(). Relayers are only refunded for filling
+     * deposits with deposit hashes that map exactly to the one emitted by this contract.
+     * @param depositNonce The nonce that uniquely identifies this deposit. This function will combine this parameter
+     * with the msg.sender address to create a unique uint256 depositNonce and ensure that the msg.sender cannot


if the deposit is being made via a 4337 bundler, then the msg.sender will be the bundler address?

I'm not super familiar with 4337 but that sounds right. I think that might be a feature that allows the bundler to ensure that its own deposit hashes are all unique. The alternative instead of msg.sender is using the depositor address but that allows the msg.sender to manipulate the hashes and try to front-run honest msg.senders.

Qumeric · 2024-09-13T12:24:39Z

contracts/SpokePool.sol

+        // this is unlikely and we also should consider not checking it and incurring the extra gas cost:
+        // if (msg.sender == address()) revert InvalidUnsafeDepositor();
+
+        uint256 depositHash = uint256((uint160(msg.sender) << 12) | depositNonce);


nit: not sure if "hash" is the correct word here. Probably "id" would be better, like in "speedUpV3Deposit"

Qumeric · 2024-09-13T12:25:50Z

contracts/SpokePool.sol

+        uint256 outputAmount,
+        uint256 destinationChainId,
+        address exclusiveRelayer,
+        uint256 depositNonce,


nit: "nonce" is overloaded as "uint96" provided by the user and also as "uint256" (msg.sender + value provided by the user). Perhaps "id" would be fitting here as well

nicholaspai · 2024-09-24T15:54:34Z

I've updated the algorithm to derive the depositId in unsafeDepositV3 by removing the left shift and OR operations in favor of more simply packing together the msg.sender's address and the input depositNonce, keccak256 hashing the packed bytes, and converting that bytes32 into a uint256. I believe this preserves the properties we want (every depositor can generate their own unique deposit hashes and not get griefed by other EOA's)

nicholaspai · 2024-09-25T14:37:58Z

contracts/SpokePool.sol

            // Increment count of deposits so that deposit ID for this spoke pool is unique.
+            // @dev Implicitly casts from uint32 to uint256 by padding the left-most bytes with zeros. Guarantees


Something to verify for the reviewer: We want to guarantee that any caller of depositV3 will never produce a uint256 depositId that could collide with one produced by a caller of unsafeDepositV3. I think this is true because numberOfDeposits is a uint32 while the way unsafeDepositV3 produces a depositId is by packing the 20 byte address of the msg.sender with the uint96 depositId which guarantees that the resultant 32 byte value when casted into a uint256 will always be greater than the MAX_UINT32 if the address is non-0

UPDATED: packing the address with a depositId input by the caller and then hashing the resultant bytes. The chance that a resultant hash collides with a uint32 is less than the chance that the first 28 bytes of the hash are equal to 0 which is extremely small. I don't think this is a realistic possibility we need to guard against.

The keccak256 hashing of the packed 32 bytes removes the guarantee that unsafe and safe deposits can't collide. This is because the resultanthash might produce a number less than uint32.max

mrice32

A few comments! Looks awesome

mrice32 · 2024-09-25T16:24:51Z

contracts/SpokePool.sol

+        // this is unlikely and we choose to not check it and incur the extra gas cost:
+        // e.g. if (msg.sender == address(0)) revert InvalidUnsafeDepositor();
+
+        uint256 depositId = uint256(bytes32(abi.encodePacked(msg.sender, depositNonce)));


We could flip the ordering and require that depositNonce > 0, making every custom nonce a very large number regardless of address. That would mean that any address would work, which would remove any concerns around precompiles that are at address 0x1, 0x2, 0x3 (not sure if anybody does this, but would prefer to avoid building our noncing around it).

Thoughts?

require that depositNonce > 0,

Ideally I was trying to avoid an additional require statement here but I agree this would be a good algorithm. This is why I wanted to avoid require(msg.sender != address(0))

I think this comment is irrelevant now that we hash the packed data: 19a3380

mrice32 · 2024-09-25T16:32:35Z

contracts/SpokePool.sol

+        // we might want to consider explicitly checking that the msg.sender is not the zero address. However,
+        // this is unlikely and we choose to not check it and incur the extra gas cost:
+        // e.g. if (msg.sender == address(0)) revert InvalidUnsafeDepositor();
+


OOC, what's the reasoning behind not wrapping all this in a hash? Simplicity for the integrator to predict the deposit hash?

Hash advantages:

We can use full 256 bit depositNonces (might improve cross compatibility with things like permit2 or other noncing systems that may be used concurrently).

No matter how we concat the data, there is no concern about creating collisions with normal depositIds.

We could add more parameters that get baked into the depositId. For instance, this could be called by a contract that passes its msg.sender as the depositor. Adding the depositor to the depositId hash would mean that different depositors wouldn't collide.

there is no concern about creating collisions with normal depositIds.

Is this true? What if the hash produces some value where the first 28 bytes are 0 (miraculously)? Wouldn't that then translate into a valid uint32 value?

Otherwise I totally agree with all the reasons FOR hashing this

Did the math on this.

The probability of finding 28 bytes of leading zeros is 1 - (1 - (1/2^224))^num_tries.

You get to about 0.3% after 1e65 tries (screenshot below). For reference, Bitcoin produces about 2e28 hashes per year, meaning it would take about 5e36 years for the entire bitcoin network to have a 0.3% chance of cracking this.

contracts/SpokePool.sol

nicholaspai · 2024-09-26T14:50:44Z

@mrice32 just one question: do you think we should add more params to the hashed struct?

We could add more parameters that get baked into the depositId. For instance, this could be called by a contract that passes its msg.sender as the depositor. Adding the depositor to the depositId hash would mean that different depositors wouldn't collide.

nicholaspai · 2024-09-26T14:52:52Z

contracts/SpokePool.sol

    /**************************************
     *         INTERNAL FUNCTIONS         *
     **************************************/

+    function _depositV3(


This should be copied from the existing depositV3 logic

mrice32 · 2024-10-01T10:28:42Z

@mrice32 just one question: do you think we should add more params to the hashed struct?

We could add more parameters that get baked into the depositId. For instance, this could be called by a contract that passes its msg.sender as the depositor. Adding the depositor to the depositId hash would mean that different depositors wouldn't collide.

Yeah, I think msg.sender and depositor should both be in there to handle interactions from both regular users and passthrough contracts.

pxrl · 2024-10-01T19:42:33Z

contracts/SpokePool.sol

+     * @dev msg.sender and depositor are both used as inputs to allow passthrough depositors to create unique
+     * deposit hash spaces for unique depositors.
+     * @param sender The msg.sender address used as input to produce the deposit ID.


This wording is a bit counter-intuitive for me because it's relying on the implementation details of another function (unsafeDepositV3()). i.e. it's confusing that msg.sender is referenced in the comment, but sender is an input argument and can be any address.

Is relayer a candidate variable name? It implies that the relayer's address is needed to produce the deterministic deposit ID. In context of a relayer filling via a contract it would be the contract address, not the EOA, so idk whether that introduces a possible additional source of confusion.

I think msgSender maybe is a better variable name

Because maybe an aggregator also wants to use the unsafeDeposit method but there would be no natural "relayer" to fill in, right? Maybe i'm over thinking it

mrice32

LGTM!

…shes (#583)" This reverts commit 9d21d1b.

…ashes (#583)" This reverts commit d363bf0.

…shes (#583)" (#638)

…n offset, or a timestamp (#670) * Revert "feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)" This reverts commit 9d21d1b. * Reapply "feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)" This reverts commit d363bf0. * add deposit nonces to 7683 Signed-off-by: Matt Rice <[email protected]> * fix Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583) * fix(SpokePool): Apply exclusivity consistently The new relative exclusivity check has not been propagated to fillV3RelayWithUpdatedDeposit(). Identified via test case failures in the relayer. Signed-off-by: Paul <[email protected]> * Also check on slow fill requests * Update contracts/SpokePool.sol * lint * Update * Add pure * Fix * Add tests * improve(SpokePool): _depositV3 interprets `exclusivityParameter` as 0, an offset, or a timestamp There should be a way for the deposit transaction to remove chain re-org risk affecting the block.timestamp by allowing the caller to set a fixed `exclusivityDeadline` value. This supports the existing behavior where the `exclusivityDeadline` is always emitted as its passed in. The new behavior is that if the `exclusivityParameter`, which replaces the `exclusivityDeadlineOffset` parameter, is 0 or greater than 1 year in seconds, then the `exclusivityDeadline` is equal to this parameter. Otherwise, its interpreted by `_depositV3()` as an offset. The offset would be useful in cases where the origin chain will not re-org, for example. * Update SpokePool.sol * Update SpokePool.Relay.ts * Update SpokePool.SlowRelay.ts * Update contracts/SpokePool.sol Co-authored-by: Paul <[email protected]> * Update SpokePool.sol * Update contracts/SpokePool.sol * rebase * Update SpokePool.sol --------- Signed-off-by: Matt Rice <[email protected]> Signed-off-by: Paul <[email protected]> Co-authored-by: Matt Rice <[email protected]> Co-authored-by: Paul <[email protected]>

* Revert "feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)" This reverts commit 9d21d1b. * Reapply "feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)" This reverts commit d363bf0. * add deposit nonces to 7683 Signed-off-by: Matt Rice <[email protected]> * fix Signed-off-by: Matt Rice <[email protected]> * WIP Signed-off-by: Matt Rice <[email protected]> * feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583) * fix(SpokePool): Apply exclusivity consistently The new relative exclusivity check has not been propagated to fillV3RelayWithUpdatedDeposit(). Identified via test case failures in the relayer. Signed-off-by: Paul <[email protected]> * Also check on slow fill requests * Update contracts/SpokePool.sol * lint * Update * Add pure * Fix * Add tests * improve(SpokePool): _depositV3 interprets `exclusivityParameter` as 0, an offset, or a timestamp There should be a way for the deposit transaction to remove chain re-org risk affecting the block.timestamp by allowing the caller to set a fixed `exclusivityDeadline` value. This supports the existing behavior where the `exclusivityDeadline` is always emitted as its passed in. The new behavior is that if the `exclusivityParameter`, which replaces the `exclusivityDeadlineOffset` parameter, is 0 or greater than 1 year in seconds, then the `exclusivityDeadline` is equal to this parameter. Otherwise, its interpreted by `_depositV3()` as an offset. The offset would be useful in cases where the origin chain will not re-org, for example. * Update SpokePool.sol * Update SpokePool.Relay.ts * Update SpokePool.SlowRelay.ts * Update contracts/SpokePool.sol Co-authored-by: Paul <[email protected]> * Update SpokePool.sol * Update contracts/SpokePool.sol * rebase * Update SpokePool.sol * Revert "Merge branch 'npai/exclusivity-switch' into mrice32/deterministic-new" This reverts commit 2432944, reversing changes made to 6fe3534. * Revert "Merge branch 'npai/exclusivity-switch' into mrice32/deterministic-new" This reverts commit 2432944, reversing changes made to 6fe3534. * revert * Update SpokePool.sol * Fix * Update SpokePool.sol Co-authored-by: Chris Maree <[email protected]> * WIP * WIP * wip * Update SpokePool.Relay.ts * Fix * Update SpokePool.sol * Update SpokePool.sol --------- Signed-off-by: Matt Rice <[email protected]> Signed-off-by: Paul <[email protected]> Co-authored-by: nicholaspai <[email protected]> Co-authored-by: nicholaspai <[email protected]> Co-authored-by: Paul <[email protected]> Co-authored-by: Chris Maree <[email protected]>

…h on "unsafe" deposit ID's I'm combing through the SDK for any functions that will need to change once this contracts [PR](across-protocol/contracts#583) is merged and here are some changes: - Added comments to BundleDataClient - Export useful constant `MAX_SAFE_DEPOSIT_ID` - Export useful utility `isUnsafeDepositId()` - Delete unused functions that wouldn't work post spoke pool upgrade because the `V3RelayData.depositId` type will change to uint256

nicholaspai mentioned this pull request Aug 20, 2024

[WIP] feat: Define in-place alternative to depositId that preserves deposit hash uniqueness while allowing deterministic fill hash computations for honest depositors #538

Closed

change types

28fcb30

nicholaspai requested a review from mrice32 August 20, 2024 16:57

nicholaspai mentioned this pull request Aug 24, 2024

improve(BundleDataClient): Skip queryHistoricalDepositForFill function for non-infinite expiry deposits across-protocol/relayer#1769

Merged

ankitchiplunkar reviewed Sep 13, 2024

View reviewed changes

Qumeric reviewed Sep 13, 2024

View reviewed changes

nicholaspai added 3 commits September 24, 2024 10:10

merge master

cbdb3b5

Update SpokePool.sol

229f5ea

Use different hashing algorithm, add tests

9e732fb

nicholaspai marked this pull request as ready for review September 24, 2024 15:55

nicholaspai requested a review from pxrl September 24, 2024 15:56

nicholaspai commented Sep 25, 2024

View reviewed changes

Remove the keccak256

60589f1

The keccak256 hashing of the packed 32 bytes removes the guarantee that unsafe and safe deposits can't collide. This is because the resultanthash might produce a number less than uint32.max

mrice32 reviewed Sep 25, 2024

View reviewed changes

nicholaspai requested a review from mrice32 September 25, 2024 17:04

nicholaspai added 3 commits September 25, 2024 13:09

Add view function

751ddf8

Hash packed deterministic deposit id

19a3380

Update SpokePool.Deposit.ts

635c541

nicholaspai commented Sep 26, 2024

View reviewed changes

Include both depositor and msg.sender in the hash

497a493

pxrl reviewed Oct 1, 2024

View reviewed changes

Update SpokePool.sol

78eb4c0

mrice32 mentioned this pull request Oct 7, 2024

feat: add updated implementation of ERC-7683 #632

Merged

mrice32 approved these changes Oct 7, 2024

View reviewed changes

nicholaspai merged commit 9d21d1b into master Oct 8, 2024
9 checks passed

nicholaspai deleted the npai/deterministic-relay-hash branch October 8, 2024 13:55

mrice32 added a commit that referenced this pull request Oct 9, 2024

Revert "feat(SpokePool): Introduce opt-in deterministic relay data ha…

d363bf0

…shes (#583)" This reverts commit 9d21d1b.

mrice32 added a commit that referenced this pull request Oct 9, 2024

Reapply "feat(SpokePool): Introduce opt-in deterministic relay data h…

c40c197

…ashes (#583)" This reverts commit d363bf0.

mrice32 mentioned this pull request Oct 9, 2024

feat: Introduce opt-in deterministic relay data hashes (again) #639

Merged

nicholaspai pushed a commit that referenced this pull request Oct 9, 2024

Revert "feat(SpokePool): Introduce opt-in deterministic relay data ha…

5a0c67c

…shes (#583)" (#638)

nicholaspai added a commit that referenced this pull request Oct 9, 2024

feat(SpokePool): Introduce opt-in deterministic relay data hashes (#583)

7641fbf

nicholaspai mentioned this pull request Dec 17, 2024

improve(BundleDataClient): Add comment about not running binary search on "unsafe" deposit ID's across-protocol/sdk#796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

nicholaspai commented Aug 20, 2024 •

edited

Loading

nicholaspai commented Aug 26, 2024

ankitchiplunkar Sep 13, 2024

nicholaspai Sep 24, 2024

Qumeric Sep 13, 2024

nicholaspai Sep 24, 2024

Qumeric Sep 13, 2024

nicholaspai commented Sep 24, 2024

nicholaspai Sep 25, 2024 •

edited

Loading

mrice32 left a comment

mrice32 Sep 25, 2024

nicholaspai Sep 25, 2024

nicholaspai Sep 26, 2024

mrice32 Sep 25, 2024

nicholaspai Sep 25, 2024

nicholaspai Sep 25, 2024

mrice32 Sep 26, 2024 •

edited

Loading

nicholaspai commented Sep 26, 2024

nicholaspai Sep 26, 2024

mrice32 commented Oct 1, 2024

pxrl Oct 1, 2024

nicholaspai Oct 2, 2024

nicholaspai Oct 2, 2024

nicholaspai Oct 2, 2024

mrice32 left a comment

		// Increment count of deposits so that deposit ID for this spoke pool is unique.
		// @dev Implicitly casts from uint32 to uint256 by padding the left-most bytes with zeros. Guarantees

feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

feat(SpokePool): Introduce opt-in deterministic relay data hashes #583

Conversation

nicholaspai commented Aug 20, 2024 • edited Loading

Motivation

Implementation

Changes to V3RelayData struct

Implications for clients

nicholaspai commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai commented Sep 24, 2024

nicholaspai Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

nicholaspai commented Sep 26, 2024

Choose a reason for hiding this comment

mrice32 commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

nicholaspai commented Aug 20, 2024 •

edited

Loading

nicholaspai Sep 25, 2024 •

edited

Loading

mrice32 Sep 26, 2024 •

edited

Loading