fix: enforce single-token outputs and report cosigner-adjusted amounts to fillers - #693
Open
codyborn wants to merge 2 commits into
Open
fix: enforce single-token outputs and report cosigner-adjusted amounts to fillers#693codyborn wants to merge 2 commits into
codyborn wants to merge 2 commits into
Conversation
…s to fillers Two gaps found while reviewing a bug bounty report against the V2 hard-RFQ path. Output validation never ran for Dutch V2. `validate()` classifies a CosignedV2DutchOrder as `OrderType.Dutch_V2`, but the branch that calls `validateDutchOutputs` was gated on `orderType == OrderType.Dutch`, so V2 orders reached the repository with no output checks at all: not the token address check, not the uint256 range checks, not `endAmount <= startAmount`. Fixed the gate, and added the V2 equivalent of the V3 cosigner-override checks (array length, and a non-zero override must be >= the signed startAmount) so we stop storing orders that `V2DutchOrderReactor._updateWithCosignerAmounts` will revert on. Every output of an order must now pay the same token, across Dutch V1/V2/V3 and Priority. RFQ quotes an order as one (tokenIn, tokenOut, amount) tuple with tokenOut taken from `outputs[0].token`, and the cosigner sums output start amounts into a single scalar to compare against the quote, so an order whose outputs span multiple tokens gets priced against a total that mixes assets. Multi-output orders are unaffected: fee outputs use the swapper output's token. `GET /orders` also reported the signed base amounts while the reactor substitutes `cosignerData.inputOverride` / `outputOverrides` for `startAmount` whenever they are non-zero, so neither `input`/`outputs` nor `cosignerData` alone describes what a fill moves. Dutch V2 and V3 responses now carry `effectiveInput` and `effectiveOutputs` with the overrides applied. Both are additive and optional: `input`, `outputs` and `cosignerData` are unchanged, so existing filler integrations keep working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Run the same-token check after each output's own token address is validated.
Checking first made an invalid outputs[0].token the "expected" token, so
`[{token: '0xfoo'}, {token: WETH}]` reported the valid output as the offender
and echoed the garbage string back as the expected value.
- Return `inputEndAmountValidation` instead of `inputStartAmountValidation` when
the input endAmount is invalid. The wrong variable is known-valid at that point,
so an invalid endAmount returned success and skipped output validation, the
cosigner override checks and the hash check. Latent today because
`validateInputAmount` only rejects non-uint256 values, which ABI decoding cannot
produce, but it is the same class of bypass this PR exists to close.
- Cover the hybrid same-token path, which the previous commit enforced without a
test. The hybrid factory hardcodes a placeholder reactor, so the test points it
at the real Unichain Sepolia address to get past the reactor check.
- Count and log off-chain validation failures. The rejection becomes a 400 with the
reason in `detail`, but left no trace on our side, so a rule that starts rejecting
real traffic was invisible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while reviewing a bug bounty report (#847) against the V2 hard-RFQ path. Two separate problems, both in this repo.
1. Output validation never ran for Dutch V2
OffChainUniswapXOrderValidator.validate()classifies aCosignedV2DutchOrderasOrderType.Dutch_V2, but the branch that callsvalidateDutchOutputswas gated onorderType == OrderType.Dutch. That condition is never true for a V2 order, so V2 orders reached the repository with no output validation at all: no token address check, no uint256 range checks, noendAmount <= startAmount. Confirmed against the SDK before the fix: a V2 order withstartAmount: 1, endAmount: 999returned{valid: true}.Fixed the gate, and added the V2 equivalent of the checks V3 already has on cosigner overrides (array length matches outputs, and a non-zero override must be
>= startAmount) so we stop storing orders thatV2DutchOrderReactor._updateWithCosignerAmountswill revert on.2. Outputs may no longer span multiple tokens
Every output of an order must now pay
outputs[0].token. RFQ prices an order as one(tokenIn, tokenOut, amount)tuple wheretokenOutisoutputs[0].token, and the cosigner sums output start amounts into a single scalar to compare against the quote, so an order whose outputs span multiple tokens gets priced against a total that mixes assets. Multi-output orders are unaffected, since fee outputs use the same token as the swapper output.This is wired into all four output validators, so it covers every order type we accept: Dutch V1, Limit (a
DutchOrderclassified asOrderType.Dutch), Dutch V2, Dutch V3, Priority and Hybrid. There is no on-chain equivalent of this rule, so it is a new hard 400 atPOST /orderfor a shape the reactors would fill. To make that observable, off-chain validation failures now emit anOffchainValidationFailuremetric and a log line with the reason, order hash and swapper. Previously the rejection returned a 400 and left no trace on our side. Worth watching that metric after deploy; I'm confident no live integrator sends heterogeneous outputs, but the metric is what would prove it.3.
GET /ordersreported the signed amounts, not the fill amountsinput/outputsare what the swapper signed. The reactor substitutescosignerData.inputOverrideandcosignerData.outputOverrides[i]forstartAmountwhenever they are non-zero, so neither the base fields norcosignerDataon its own tells a filler what a fill actually moves. Dutch V2 and V3 responses now carryeffectiveInputandeffectiveOutputswith the overrides already applied.No breaking change.
input,outputsandcosignerDataare untouched and byte-identical to before;encodedOrderwas already complete. The two new fields are additive and optional in both the TS types and the joi response schemas, so existing filler integrations are unaffected.swagger.jsondocuments them (the sync test intest/unit/swagger.test.tsrequires it).Testing
yarn testgreen, 579 tests. New coverage: V2 same-token rejection, V2endAmount > startAmount, V2 invalid output token, V2 override< startAmount, V2 override length mismatch, V2 zero-override still valid, V3, Priority and Hybrid same-token rejection, V3 override length mismatch, an inputendAmountregression test, andeffectiveInput/effectiveOutputsfor V2 and V3 including the zero-override fallback and the base fields staying untouched. The invalid V2 fixtures mutate a built order becauseV2DutchOrderBuilderenforces invariants the reactor does not, which is also what parsing a caller-suppliedencodedOrderproduces.Follow-up commit after review
outputs[0].tokenthe "expected" token, so[{token: '0xfoo'}, {token: WETH}]named the valid output as the offender and echoed the garbage back as the expected value.endAmountreturnedinputStartAmountValidation, which is known-valid at that point, so the order validated successfully and skipped output validation, the override checks and the hash check. Latent (only non-uint256 values trip it, which ABI decoding cannot produce) but it is the same class of bug this PR closes.Out of scope
HardQuoteRequest.toCleanJSONinuniswapx-parameterization-apisends onetokenOutplusnumOutputs, andtotalOutputAmountStartraw-sums everystartAmount). This PR makes the service reject such an order atPOST /order, which stops it reaching fillers, but the parameterization API should reject it before KMS signing too. Separate PR.GET /ordersresponse (noorderTypeparam) returns raw DynamoDB entities and does not gain the new fields.encodedOrder, which already contains every output and the full override array.🤖 Generated with Claude Code