fix(ts-sdk): refuse refund on ambiguous OP_RETURN funded tx#1698
fix(ts-sdk): refuse refund on ambiguous OP_RETURN funded tx#1698jrwbabylonlab wants to merge 2 commits into
Conversation
🔐 Commit Signature Verification✅ All 2 commit(s) passed verification
Summary
Required key type: Last verified: 2026-05-21 02:01 UTC |
jrwbabylonlab
requested review from
gbarkhatov,
jeremy-babylonlabs and
jonybur
as code owners
Greptile SummaryThis PR makes refund anchor discovery fail closed on ambiguous funded transactions. The main changes are:
Confidence Score: 4/5This is close, but the validation mismatch should be fixed before merging.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(ts-sdk): refuse refund on ambiguous ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jrwbabylonlab
force-pushed
the
fix/refund-ambiguous-op-return
branch
from
24ede47 to
e6efe5b
Compare
jrwbabylonlab
force-pushed
the
fix/refund-ambiguous-op-return
branch
from
e6efe5b to
6870a7c
Compare
gbarkhatov
left a comment
gbarkhatov
left a comment
There was a problem hiding this comment.
Review
Solid, protocol-aligned cleanup of the auth-anchor read. Replacing the scan-based finder with positional countHtlcOutputs + readAuthAnchorOpReturn is the right direction, and a key invariant holds: HTLC scriptPubKeys never start with 0x6a, so the OP_RETURN break can't false-trigger on a real HTLC. The fund-safety backstop — buildRefundPsbt's WASM fromFundedTransaction alignment plus the explicit HTLC-scriptPubKey cross-check — is unchanged, so this is primarily about correctness/clarity rather than closing a fund-loss hole. Tests pass (73/73).
Approving. Inline notes are mostly non-blocking; the one I'd genuinely gate on before merge is verifying countHtlcOutputs against the Rust/Solidity reference (CLAUDE.md critical-path rule).
One PR-description fix: the body still describes the first commit ("findAuthAnchorOpReturn now throws on multiple-hit"; test flips to /OP_RETURN PUSH32 outputs/). The final state (commit 6870a7c) deleted that function entirely and reads positionally — the tests now match /at least 2 outputs/ and /malformed/. The security stance also shifted from "refuse ambiguous multi-OP_RETURN" to "read positionally, ignore trailing OP_RETURNs", which is meaningfully different and worth calling out. Please update the description so the commit history isn't misleading.
| * (an `OP_RETURN || PUSH32 || <32 bytes>` output with value 0). | ||
| * Count the HTLC outputs at the head of a funded Pre-PegIn transaction. | ||
| * | ||
| * Mirrors btc-vault `PrePegInTx::count_htlc_outputs` and the contract's |
There was a problem hiding this comment.
[Critical path — must verify] This "mirrors btc-vault count_htlc_outputs / contract _countHtlcOutputs" equivalence is asserted in the comment but not proven anywhere. The load-bearing detail is excluding the last output as the CPFP anchor ([0, len-1)): for a no-anchor [HTLC, CPFP] this must return 1, whereas a reference scanning [0, len) would return 2. Per CLAUDE.md's critical-path rule, refund/auth-anchor logic needs a cross-check against the Rust/Solidity reference — ideally a golden fixture pinning a few funded-tx shapes so this can't silently drift from the protocol.
| } | ||
| if (output.value !== 0) return undefined; | ||
| // Not an OP_RETURN → no auth anchor at this position (legitimate). | ||
| if (script.length === 0 || script[0] !== OP_RETURN) return undefined; |
There was a problem hiding this comment.
[Minor] The whole positional scheme rests on the output at vout = htlcCount never being an OP_RETURN in the no-anchor case — i.e. the CPFP anchor's script never starts with 0x6a. Standard P2A anchors are OP_1 PUSH2 0x4e73 (51024e73, first byte 0x51), so this holds today, but it's implicit: if a CPFP anchor ever started with 0x6a (and weren't a clean PUSH32), a legitimate no-anchor refund would throw malformed and be blocked. Worth a one-line note asserting the CPFP anchor is never an OP_RETURN, since correctness depends on it.
| if (script.length === 0 || script[0] !== OP_RETURN) return undefined; | ||
|
|
||
| // It IS an OP_RETURN — it must be the canonical 32-byte push, or throw. | ||
| if (script.length !== OP_RETURN_PUSH32_SCRIPT_LEN || script[1] !== OP_PUSH32) { |
There was a problem hiding this comment.
[Question] The old findAuthAnchorOpReturn (and the prior reader) required output.value === 0; the sibling assertAuthAnchorOpReturn above still enforces it. This read path no longer checks value, so a non-zero-value OP_RETURN || PUSH32 at vout = htlcCount is now accepted as a valid anchor. Impact is contained (a real funded tx can't carry a non-standard non-zero OP_RETURN, and a mismatched template fails in fromFundedTransaction before broadcast — fail-safe), but it's an asymmetry with the assert path. Either re-add value !== 0 → throw (it IS an OP_RETURN, so "malformed") or add a one-line comment on why the read path intentionally omits it — with btc-vault's extract_auth_anchor_hash as the tiebreaker.
| return undefined; | ||
| } | ||
| export function countHtlcOutputs(fundedPrePeginTxHex: string): number { | ||
| const tx = bitcoin.Transaction.fromHex(stripHexPrefix(fundedPrePeginTxHex)); |
There was a problem hiding this comment.
[Minor] The old orchestrator wrapped a Transaction.fromHex failure in Failed to parse funded Pre-PegIn transaction hex: …. countHtlcOutputs now lets the raw bitcoinjs error propagate for hex-valid-but-unparseable input (BTC_HEX_BYTES_RE only checks hex-ness, so this path is reachable). UX-only — but the context that it's the funded Pre-PegIn tx is lost from the message.
| hash: script.slice(2).toString("hex").toLowerCase(), | ||
| }); | ||
| } | ||
| if (tx.outs.length < 2) { |
There was a problem hiding this comment.
[Sanity check] This tightens behavior: the old orchestrator only required outs.length >= batch.length (≥1 for a single vault), so a hypothetical CPFP-less 1-output funded tx was refundable; it now throws. Almost certainly fine (real funded Pre-PegIns always carry a CPFP anchor), but worth confirming no in-flight deposit stored an unsignedPrePeginTxHex with <2 outputs before this ships.
| return hits.length === 1 ? hits[0] : undefined; | ||
| let count = 0; | ||
| for (let i = 0; i < tx.outs.length - 1; i++) { | ||
| if (tx.outs[i].script[0] === OP_RETURN) break; |
There was a problem hiding this comment.
[Nit] tx.outs[i].script[0] isn't guarded against an empty script here, whereas readAuthAnchorOpReturn explicitly guards script.length === 0. An empty-script output yields script[0] === undefined !== OP_RETURN → counted as an HTLC → fails the batch-size check → refuses, so it's harmless, but closing the inconsistency with the sibling reader would be nice.
3 participants
Follow-up to #1688 (already merged). Closes a fail-open gap codex flagged on that PR:
findAuthAnchorOpReturnreturnedundefinedfor both "no anchor"(legacy, expected) and "multiple OP_RETURN PUSH32 hits" (malformed/ambiguous), so the refund orchestrator silently fell through as legacy single-vault and
reconstructed against
authAnchorHash = undefined.Changes
findAuthAnchorOpReturnnow throws on multiple-hit.undefinedis reserved for zero matches / unparseable hex.toBeUndefined()→toThrow(/OP_RETURN PUSH32 outputs/).buildRefundPsbtis not called.