improve: SpokePoolPeriphery relative timestamps

[tbwebb22]

SpokePoolPeriphery now accepts quoteTimestamp and fillDeadline in absolute or relative form: any value below MAX_RELATIVE_TIME_SECONDS (30 days) is too small to be a real timestamp and is resolved against block.timestamp at execution time.

The modes run in opposite directions — fillDeadline is added (a deadline is in the future) while quoteTimestamp is subtracted (it names an already-existing HubPool snapshot), so a quoteTimestamp of 0 prices the deposit as of the block it lands in.

This keeps signed payloads in the gasless permit/permit2/ERC-3009 flows submittable without expiring against a clock fixed at signing time. Resolution runs after signature verification, so EIP-712 witnesses commit to the relative values as signed, and absolute values are forwarded unchanged.

[droplet-rl]

Summary

The mechanism is clean and the plumbing is complete — I traced every path into the spoke pool (depositNative directly, and all six gasless entry points plus swapAndBridge* via _deposit/_swapAndBridge) and confirmed both resolvers are applied consistently, after signature verification, with absolute values forwarded untouched. yarn test-evm-foundry --match-contract "SpokePoolPeripheryTest|Tron_SpokePoolPeripheryTest" passes 47/47 on my checkout.

My concern is not the implementation, it's what the feature removes. The absolute quoteTimestamp was a load-bearing expiry for signed payloads, and nothing replaces it.

The core issue

Today a signed DepositData/SwapAndDepositData self-expires: SpokePool._depositV3 rejects the deposit once currentTime - quoteTimestamp > depositQuoteTimeBuffer (~1h), so a payload the relayer never submits goes dead on its own. With a relative quoteTimestamp it resolves to "now" on every block, so the payload passes spoke pool validation forever.

For the permit2 and ERC-3009 flows that's fine — permit2's deadline and ERC-3009's validBefore are enforced by the verifying layer. But the depositWithPermit / swapAndBridgeWithPermit path has no deadline at all once this lands:

• deadline is a loose function argument, not part of the EIP-712 struct, and the permit() call is wrapped in try/catch (SpokePoolPeriphery.sol:497, :328) — so an expired permit is swallowed and execution continues.
• _validateAndIncrementNonce (:765) is strictly sequential (++_permitNonces[user] != providedNonce). A stale signature at nonce N is never invalidated; it blocks N+1 until consumed.

Concretely: a user signs a permit-flow swap, the relayer sits on it, the user re-quotes and signs a fresh payload — which must reuse nonce N, since N was never burned. Both are now valid indefinitely, and whoever submits first wins. A relayer holding the stale signature can redeem the fresh permit against the stale payload (stale outputAmount / minExpectedInputTokenAmount) whenever that's profitable, as long as the stale _pullAmount fits the new allowance. It also works with no fresh permit at all if the signer has a standing ERC-20 allowance to the periphery — which anyone who ever used the non-gasless swapAndBridge path has.

This is bounded to ~1 hour today. After this PR it is unbounded.

Recommendation

CounterfactualDepositSpokePool already solves exactly this in the same directory: signatureDeadline is a field in the signed EIP-712 struct, enforced with if (block.timestamp > submitterData.signatureDeadline) revert SignatureExpired(); (CounterfactualDepositSpokePool.sol:121, :138). Adding the same field to BaseDepositData decouples "how long is this signature good for" from "when does the fill window close", which is the actual goal of the PR. It's a breaking struct change, so worth deciding now rather than in a follow-up.

If you'd rather not add the field, the fallback is to scope relative resolution to the flows that already carry an enforced deadline (permit2, ERC-3009) and keep absolute-only for the bare-permit path — but the explicit deadline is the better answer.

Also worth resolving before merge

• The interface's own security rationale now states something false — see inline on SpokePoolPeripheryInterface.sol.
• quoteTimestamp = 0 resolving to the origin chain's block.timestamp is the risky default to advertise, given the quote is priced against HubPool state on L1. Details inline.
• Test coverage exercises only swapAndBridge and depositNative — the non-gasless paths. The gasless flows are the entire motivation and aren't covered.
• Out of scope but worth a note somewhere: SpokePoolVerifier.deposit and CounterfactualDepositSpokePool keep absolute-only semantics, so the same field name now means different things depending on which periphery contract an integrator calls.

Nice touch adding vm.warp(1_700_000_000) to both setUps — that failure mode (Foundry's default block.timestamp of 1 making every absolute timestamp look relative) would have been a confusing one to debug.

[droplet-rl]

This is where the implicit expiry disappears. Combined with _resolveQuoteTimestamp above, a signed payload using relative values passes SpokePool._depositV3's quoteTimestamp/fillDeadline checks at any future block.

For depositWithPermit/swapAndBridgeWithPermit that leaves the deposit-data signature with no time bound whatsoever: deadline isn't in the EIP-712 struct, the permit() call is try/catch'd, and the sequential nonce doesn't expire — it just blocks the next nonce until consumed.

Suggest adding a signatureDeadline to BaseDepositData and checking it here (or in _deposit), mirroring CounterfactualDepositSpokePool.sol:138. That gives the PR what it actually wants — a fill window that tracks submission time — without making the authorization itself immortal.

[droplet-rl]

Two things on the quoteTimestamp = 0 → "as of the current block" default:

1. It resolves against the wrong chain's clock. quoteTimestamp names a HubPool (L1) snapshot for realizedLpFeePct, but block.timestamp here is the origin chain's. Origin sequencer clocks routinely run ahead of the latest L1 block timestamp (L1 blocks are 12s apart, so "latest L1 block" is already up to 12s stale). When that happens the emitted quoteTimestamp is ahead of any existing L1 block and the dataworker/relayer can't resolve a block for it until L1 catches up — which is exactly why the API deliberately returns a quote timestamp a bit in the past rather than now.

An age of 0 sits precisely on the boundary SpokePool allows (currentTime < quoteTimestamp reverts, SpokePool.sol:1399). Consider either flooring the resolution at a small lag, or at minimum documenting that integrators should pass a nonzero age (~60-300s) and not leaning on 0 as the headline example.

2. Minor: uint32(block.timestamp) - quoteTimestamp reverts on underflow when block.timestamp < 30 days. Only reachable on fresh devnets/test chains — which is what the new vm.warp in setUp papers over — but it'll bite anyone spinning this up locally without knowing to warp. Worth a sentence in the natspec.

[droplet-rl]

The "Design Decision" blocks at lines 173 and 262 (unchanged in this diff) are now inaccurate. Both justify the ERC-2612/ERC-3009 cross-scheme nonce-collision risk as acceptable partly because of:

3. Issuing these signatures within a short amount of time (limited by fillDeadlineBuffer)

With relative timestamps that constraint no longer exists — condition 3 becomes free, leaving only conditions 1 and 2. Please update both blocks, and re-check whether the residual risk is still acceptable with that leg removed. Per CLAUDE.md, docs should move in the same change as the behavior.

[droplet-rl]

Two notes on the constant:

Naming: the comparison is strict <, so this is a threshold, not a max — the largest expressible relative value is MAX_RELATIVE_TIME_SECONDS - 1, and exactly 30 days is treated as an absolute timestamp (Jan 31 1970). RELATIVE_TIME_THRESHOLD_SECONDS would read more accurately, and either way the < / >= boundary deserves a test (see the test-file comment).

Value: 30 days is far wider than anything usable. depositQuoteTimeBuffer (~1h) caps the usable quote age and fillDeadlineBuffer (~6h) caps the usable fill offset, so the effective ranges are ~1-2 orders of magnitude smaller than the threshold. That's a safe direction to err, but a caller who passes e.g. 7 days as a fill offset gets a InvalidFillDeadline revert from the spoke pool rather than anything self-explanatory. Worth naming the real bounds in the natspec.

Also FYI: SpokePool.MAX_EXCLUSIVITY_PERIOD_SECONDS is 365 days, so exclusivityParameter now uses a different relative/absolute cutoff than the two fields next to it. Not wrong, but a documented inconsistency would help integrators.

[droplet-rl]

Coverage gap: all four new tests go through swapAndBridge and depositNative — the two paths that don't involve a signature. The PR's stated motivation is keeping gasless permit/permit2/ERC-3009 payloads submittable, and none of those flows are exercised with relative values.

Worth adding:

1. At least one depositWithPermit2 / depositWithAuthorization test with relative timestamps, asserting the resolved values land in FundsDeposited.
2. A test pinning the PR's central claim — that the EIP-712 witness commits to the relative value as signed. Sign a payload with quoteTimestamp = 0, vm.warp forward, submit, and assert it still verifies and resolves against the new block time. That's the behavior this PR exists to create and nothing currently locks it in.
3. Boundary cases for the strict <: MAX_RELATIVE_TIME_SECONDS - 1 (relative) vs MAX_RELATIVE_TIME_SECONDS (absolute, forwarded untouched).
4. Absolute fillDeadline passthrough — every new test uses a relative fillDeadline. testSwapAndBridgeAbsoluteTimestampsUnmodified only pins the absolute quoteTimestamp; the fillDeadline there comes from the helper default and happens to be absolute, but that's incidental rather than asserted.

[droplet-rl]

🔎 View trace

[fusmanii]

makes sense, just a few comments

[fusmanii]

on SpokePool this is set to 1 year, any reason you are doing 30 days here?

[grasphoper]

Agree, prob. better to be consistent

[tbwebb22]

good call - f38746e

[fusmanii]

Suggested change

	return fillDeadline < MAX_RELATIVE_TIME_SECONDS ? uint32(block.timestamp) + fillDeadline : fillDeadline;
	return fillDeadline <= MAX_RELATIVE_TIME_SECONDS ? uint32(block.timestamp) + fillDeadline : fillDeadline;

on SpokePool its strict equality here

[grasphoper]

Agree

[tbwebb22]

f38746e

[fusmanii]

Suggested change

	return quoteTimestamp < MAX_RELATIVE_TIME_SECONDS ? uint32(block.timestamp) - quoteTimestamp : quoteTimestamp;
	return quoteTimestamp <= MAX_RELATIVE_TIME_SECONDS ? uint32(block.timestamp) - quoteTimestamp : quoteTimestamp;

same here

[grasphoper]

Agree

[tbwebb22]

f38746e

[grasphoper]

Something else that we could address in this PR. Now that we allow these relative timestamps, entrypoints *withPermit, eg depositWithPermit allow the user to create "infinite standing signatures" accidentally.

If they specify both timelines as relative and they have a standing approval to periphery, a party that has their signature can execute this TX at any point in the future, forever.

We might want to add deadline to these entrypoints, eg here:

// add deadline to sig check and check that
_validateSignature(signatureOwner, PeripherySigningLib.hashDepositData(depositData), depositDataSignature);

[grasphoper]

Agree, prob. better to be consistent

[grasphoper]

Agree

[grasphoper]

Agree

[tbwebb22]

Something else that we could address in this PR. Now that we allow these relative timestamps, entrypoints *withPermit, eg depositWithPermit allow the user to create "infinite standing signatures" accidentally.

If they specify both timelines as relative and they have a standing approval to periphery, a party that has their signature can execute this TX at any point in the future, forever.

@grasphoper good call on this - I think having the signatures still be able to expire is pretty much necessary. Added that deadline to permit signature checks here:

3d71731

[droplet-rl]

Re-review of f38746e + 3d71731

The blocking issue is fixed, and fixed well. Approving — everything below is non-blocking.

What I verified

The deadline is correctly bound and enforced. _requireSignatureNotExpired is the first statement in both permit entrypoints (SpokePoolPeriphery.sol:318, :489), so it fires before the nonce is consumed and before either signature is read. The deadline is folded into the payload digest via hashSignedDepositData/hashSignedSwapAndDepositData, so a submitter can't substitute it. That closes the stale-payload hole: a signature at nonce N is no longer redeemable indefinitely against a standing allowance.

Omitting the guard on permit2/ERC-3009 is correct, and I like that each is now pinned by a test rather than just a comment — testPermit2DepositBoundedByItsOwnPermitDeadline demonstrates permit2 enforces its own deadline with no try/catch to swallow it. Same reasoning holds for ERC-3009's validBefore.

The EIP-712 typehashes are correct. This was the highest-risk part of the change and the test suite can't catch it — HashUtils calls the same PeripherySigningLib code that produces the hash, so a malformed type string would pass tests while breaking every real wallet. I checked both new typehashes independently against ethers v6's TypedDataEncoder:

PASS  DepositData               0xa456b32d…  (control)
PASS  SwapAndDepositData        0xf9a445fa…  (control)
PASS  SignedDepositData         0x2b9c1af4…
PASS  SignedSwapAndDepositData  0x44bc9298…

Both match. The alphabetical ordering of referenced types is right in both cases — SignedDepositData needs BaseDepositData, DepositData, Fees and SignedSwapAndDepositData needs BaseDepositData, Fees, SwapAndDepositData, and that's exactly what the abi.encodePacked sequences produce. Easy thing to get wrong; you didn't.

Nice side effect worth knowing about: because the permit entrypoints now hash a different EIP-712 struct type (SignedDepositData) than the permit2/ERC-3009 witnesses (bare DepositData), the digests can no longer collide across schemes. The "theoretical replay attack" in the Design Decision blocks is now structurally impossible for the permit-vs-3009 pair, not merely unlikely. You could state that outright.

MAX_RELATIVE_TIME_SECONDS → 31_536_000 with <= now matches SpokePool.MAX_EXCLUSIVITY_PERIOD_SECONDS and its comparison operator. Functionally a no-op given depositQuoteTimeBuffer/fillDeadlineBuffer cap the usable range far below either value, but one cutoff across all three relative-capable fields is the right call for integrator mental model. No underflow/overflow reachable at the widened bound on a real chain.

66/66 tests pass locally (SpokePoolPeripheryTest, Tron_SpokePoolPeripheryTest, TransferProxyTest).

Non-blocking follow-ups

Detail inline; the short version:

1. deadline does double duty as the ERC-2612 permit deadline, and type(uint256).max is a common integrator default for permits — which would silently restore unbounded payload lifetime. Worth a deliberate decision.
2. Two doc spots still describe the pre-PR world (@param deadline, and item 3 of both Design Decision blocks).
3. Still no success-path test for relative timestamps through a gasless flow — the flow the PR exists to serve.

One thing you consciously didn't change

I raised quoteTimestamp = 0 resolving against the origin chain's clock while the quote prices HubPool (L1) state. The docs still lead with 0 therefore means "as of the current block". That's a defensible product call — the spoke pool explicitly permits currentTime — but it does mean the advertised default is the one most exposed to origin-clock-ahead-of-L1 drift, where the dataworker can't resolve an L1 block for the quote until L1 catches up. Flagging once more only so it's a decision rather than an oversight; not asking for a change.

Coordination note

The permit entrypoints now expect a signature over a new EIP-712 type. Any off-chain signer (API, frontend, SDK) must be updated in lockstep or every depositWithPermit/swapAndBridgeWithPermit call will fail signature validation. New deployment required regardless since the periphery is constructor-initialized, but worth calling out in the release notes.

[droplet-rl]

Correct fix, and the placement (first statement in both entrypoints, before the nonce increment) is right.

One residual gap worth a deliberate decision: deadline is the same value passed to ERC-2612 permit, so it carries two meanings at once. type(uint256).max is an extremely common default for permit deadlines — plenty of SDKs and integrations emit it unthinkingly, since the permit is already single-use and nonce-protected. If that value shows up here, block.timestamp > signatureDeadline is never true and the payload lifetime bound silently evaporates, putting us back at the unbounded case this commit set out to fix.

The user does sign the deadline, so max is technically an explicit authorization — but it's authorization via a field wallets render as a 78-digit number, which isn't meaningful consent.

Two options:

• Cap it here: if (signatureDeadline > block.timestamp + MAX_SIGNATURE_LIFETIME) revert ..., with the max set to something comfortably above any realistic relayer submission delay.
• Keep the fields separate — a distinct signatureDeadline alongside the permit deadline — so a long-lived permit doesn't imply a long-lived payload.

The cap is the smaller change and is enforceable regardless of integrator discipline. Either way, worth a line in the natspec that deadline now bounds the payload and not just the permit.

[droplet-rl]

The new @dev block is a good explanation — but two nearby lines still describe the pre-PR world and are now actively misleading:

Line 268 (and its twin at line 176), item 3 of the Design Decision block:

3. Issuing these signatures within a short amount of time (limited by fillDeadlineBuffer)

fillDeadlineBuffer stopped bounding this the moment relative timestamps landed; the bound is now the signed deadline. Worth rewriting to say so.

And as noted in the summary — this block can now make a stronger claim than it does. The permit entrypoints hash SignedDepositData/SignedSwapAndDepositData while the permit2 and ERC-3009 witnesses hash the bare DepositData/SwapAndDepositData. Different EIP-712 primary types means the digests can't collide, so a signature issued for one scheme can't be replayed into the other at all. That's stronger than "incredibly unlikely" and worth stating.

Line 271 (and line 179):

@PARAM deadline Deadline before which the permit signature is valid.

Now also the deadline for the payload signature, and bound into it. Should say both.

[droplet-rl]

testDepositWithPermitStaleSignatureCannotBeRedeemedLater is a good regression test — it reproduces the exact scenario (relative timestamps + try/catch'd permit + standing allowance) and pins the fix.

But the gasless-flow coverage is still all negative-path. Grepping for relative quoteTimestamp assignments, every gasless test that sets one asserts a revert; the success cases (lines 319, 359, 399) all go through swapAndBridge, which isn't a signed flow.

So nothing yet proves the feature actually works where it was built to work. The missing test is the positive counterpart to this one:

1. Sign a depositWithPermit payload at T with quoteTimestamp = 0, fillDeadline = fillDeadlineBuffer, and a deadline of, say, T + 1 hour.
2. vm.warp(T + 30 minutes) — inside the deadline.
3. Submit, and assert FundsDeposited carries quoteTimestamp == T + 30 minutes and fillDeadline == T + 30 minutes + fillDeadlineBuffer.

That's the whole thesis of the PR in one test: a payload signed against one clock still resolves against the clock it lands in. Right now if _resolveQuoteTimestamp were accidentally dropped from the _deposit path, every test here would still pass.

Also still open from the last round: a boundary test for the <=, i.e. MAX_RELATIVE_TIME_SECONDS exactly (relative) vs MAX_RELATIVE_TIME_SECONDS + 1 (absolute, forwarded untouched). The comparison operator just changed in this commit, which is exactly when that test earns its keep.

[droplet-rl]

🔎 View trace