Skip to content

fix(uniswapx-sdk): unsigned v3 dutch order serializes each output's own relativeBlocks - #711

Open
gomesalexandre wants to merge 1 commit into
Uniswap:mainfrom
gomesalexandre:fix_unsigned_v3dutchorder_output_blocks
Open

fix(uniswapx-sdk): unsigned v3 dutch order serializes each output's own relativeBlocks#711
gomesalexandre wants to merge 1 commit into
Uniswap:mainfrom
gomesalexandre:fix_unsigned_v3dutchorder_output_blocks

Conversation

@gomesalexandre

Copy link
Copy Markdown

What

UnsignedV3DutchOrder.serialize() writes the input curve's encoded relativeBlocks into every output, instead of each output's own. CosignedV3DutchOrder.serialize() (same file, line ~557) already does per-output encoding correctly — this fix brings UnsignedV3DutchOrder.serialize() in line with it.

// before — one shared encoding, reused for every output
const encodedRelativeBlocks = encodeRelativeBlocks(this.info.input.curve.relativeBlocks);
...
this.info.outputs.map((output) => [
    output.token, output.startAmount,
    [encodedRelativeBlocks, output.curve.relativeAmounts],   // input's blocks + this output's amounts
    ...

Repro (real code, not described)

Input curve relativeBlocks: [1,2,3,4], output curve relativeBlocks: [5,10,15,20]:

UNSIGNED input blocks written : [ 1, 2, 3, 4 ]
UNSIGNED output blocks written: [ 5, 10, 15, 20 ]
UNSIGNED output blocks READ BACK: [ 1, 2, 3, 4 ]        <- wrong, this is the input's
COSIGNED (control) output blocks READ BACK: [ 5, 10, 15, 20 ]   <- correct sibling

Why the existing tests never caught it

Both round-trip tests in V3DutchOrder.test.ts construct a CosignedV3DutchOrder — the sibling that already encodes correctly — so UnsignedV3DutchOrder.serialize() itself was never exercised by any prior test. The one test touching UnsignedV3DutchOrder only calls fromJSON and asserts two startAmount strings, never relativeBlocks.

Added a new test constructing UnsignedV3DutchOrder directly with differing input/output curves (a shared curve would make the bug invisible), asserting each side round-trips its own blocks.

Severity — read this before assuming a signing bug

I checked hash()/witnessInfo() (the function _TypedDataEncoder hashes for signing) and confirmed it already encodes each output's own relativeBlocks correctly:

baseOutputs: this.info.outputs.map((output) => ({
    ...
    curve: { relativeBlocks: encodeRelativeBlocks(output.curve.relativeBlocks), ... },

So this is not a signing-forgery path — the signed digest was always correct. The corruption is confined to the byte string serialize() produces (storage/transport/pre-cosign quoting), not to what gets signed.

I did not identify a production caller of UnsignedV3DutchOrder.serialize() in this repo — flagging as an open dependency rather than a demonstrated live-money bug.

One caveat worth noting: an order already serialized under the buggy code will still decode with the wrong output block schedule after this fix lands — it corrects newly-serialized orders only, it can't retroactively repair previously-persisted bytes.

Collision check

Open PR #705 touches the same file (CosignedV3DutchOrder.resolve(), encodeRelativeBlocks/decodeRelativeBlocks range checks) but never touches UnsignedV3DutchOrder.serialize() (lines 197-233) — confirmed via gh pr diff 705 before starting, file-level overlap only, no hunk conflict expected.

Testing

  • New test: genuine red-before/green-after via git stash — fails with expected [ 1, 2, 3, 4 ] to deeply equal [ 5, 10, 15, 20 ] on unfixed code, passes after.
  • Full uniswapx-sdk unit suite: 351/351 pass (bun test src/).
  • tsc --noEmit -p tsconfig.cjs.json: clean.
  • bun run lint: currently broken on unmodified maineslint src --ext .ts fails with Invalid option '--ext' because the installed eslint (v8.57.1) auto-detects flat-config mode from... actually no eslint.config.*/.eslintrc* file exists anywhere in the repo at all. Confirmed this is pre-existing and unrelated to this diff by stashing my changes and re-running on a clean tree — identical failure. Not fixed here, out of scope for this PR; flagging for a maintainer.

Codex reviewed the diff adversarially: verdict "no findings," confirmed the fix is complete (no sibling class has the same pattern), the new test is non-vacuous, and independently re-ran the full order test suite (62/62 pass) as part of its own verification.

Risk

Low. Change is additive-correct (each output now encodes its own already-present data instead of borrowed data) with no change to types, no change to hash(), no change to any other class.

@gomesalexandre
gomesalexandre marked this pull request as ready for review September 1, 2026 18:07
@gomesalexandre
gomesalexandre requested a review from a team as a code owner September 1, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant