fix(underwriter): bind the source-deposit hash to the caller's target_amount - #556
Merged
Merged
Conversation
…_amount Both outposts fold the user's accepted `target_amount` into the `SwapDeposit` correlation hash — `ReserveManagerLib.hashSwapDeposit` on EVM, `swap_correlation_hash` on SVM. The underwriter's source-deposit verifier recomputed that preimage from `uwreq.dst_amount` instead. Those were the same value until #550 split them: `dst_amount` is now the depot's AMM quote, re-priced at ingestion, and `sysio.uwrit` documents it as "never the caller's `target_amount`". So the recomputed hash matches only when the caller's target happens to equal the depot's quote exactly — and a gap between the two is the expected case, bounded by `variance_tolerance_bps`. Every other swap stalls: verification fails, the UWREQ never leaves PENDING, and no underwriter commits. Observed on a live cluster (ETH→SOL, target 98039214 vs quote 97747972): 148 consecutive `SwapDeposit hash mismatch` rejections against one stable hash pair, until the flow timed out waiting for a CONFIRMED commitment. The packing was duplicated across the EVM and SVM verifiers, which is how one wrong field became two. Both now call a single `source_deposit_hash` helper whose input struct names the field `target_amount`, so the depot's mutable quote is not reachable from the preimage by accident. The EVM leg gains the explicit depositor-size check the SVM leg already had — previously it was implied by a post-hoc buffer-length assertion the helper makes unnecessary. Regression coverage pins the preimage to the production vector above: the recomputed hash equals the `SwapDeposit` hash `ReserveManager` actually emitted, and the settlement quote demonstrably does not reproduce it. Change-Id: Ib2365213ae59d495182360477b26dd5e16ff0062
huangminghuang
approved these changes
Aug 10, 2026
huangminghuang
left a comment
Contributor
There was a problem hiding this comment.
Approved. As the PR description notes, these tests pin the helper's preimage rather than the two production call sites. The call-site regression test—using distinct target_amount and dst_amount values and asserting that the EVM/SVM verifier input selects target_amount—should be added in #543, which owns the broader verifier refactor.
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.
Split out of #543, which fixes this as part of a broader UIC-prevalidation refactor. It is isolated here because it is a live defect on
masterwith a small, independently-reviewable fix, and #543 still has open design discussion on unrelatedsysio.uwritreject semantics.The defect
Both outposts fold the user's accepted
target_amountinto theSwapDepositcorrelation hash:ReserveManagerLib.hashSwapDeposit(sender, sourceAmountDepot, args)packsargs.targetAmountswap_correlation_hash(..., target_amount, target_tolerance_bps)The underwriter's source-deposit verifier recomputed that preimage from
uwreq.dst_amount.Those were the same value until #550 split them.
dst_amountis now the depot's AMM quote, re-priced at ingestion;sysio.uwrit.hppstates it is "never the caller'sSwapRequest.target_amount". The recomputed hash therefore matches only when the caller's target happens to equal the depot's quote exactly — and a gap between the two is the expected case, which is whatvariance_tolerance_bpsexists to bound. Any swap with a non-zero gap fails verification, the UWREQ never leavesPENDING, and no underwriter commits.Observed
A live ETH→SOL underwritten swap (
target_amount98039214,dst_amount97747972) produced 148 consecutive rejections against a single stable hash pair, until the flow timed out waiting for a CONFIRMED commitment:Recomputing that preimage both ways confirms the split precisely:
target_amount= 98039214fd8f16aa…ReserveManageremitteddst_amount= 97747972758ed0f4…The fix
Carry
target_amounton the plugin'suw_request, read it from theuwreqsrow, and use it in the preimage.The packing was duplicated across the EVM and SVM verifiers, which is how one wrong field became two. Both now call a single
source_deposit_hashhelper (lifted verbatim from #543, so that PR's rebase is a no-op) whose input struct names the fieldtarget_amount— the depot's mutable quote is no longer reachable from the preimage by accident. The EVM leg also gains the explicit depositor-size check the SVM leg already had; previously it was only implied by a post-hoc buffer-length assertion the helper makes unnecessary.Coverage
Two cases pin the preimage to the production vector above — the recomputed hash equals the
SwapDeposithashReserveManageractually emitted, and the settlement quote demonstrably does not reproduce it.Note the tests pin the helper's preimage layout and field semantics, not the two call sites' argument selection; the naming (
target_amountvsdst_amounton the input struct) is what guards those.Verification
test_underwriter_plugin— 50 cases,*** No errors detectedplugin_test—*** No errors detectednodeoplinks cleanNo
contracts/**changes, socontracts_unit_testis not implicated.