Repository navigation
Bind SubmitFinalization signatures to authorized spends - #126
chris-ricketts merged 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughFinalization now rejects expired or invalid intent messages before signing. It binds each signed input to the authorized Arkade signer, prevout, and Taproot leaf. Forfeits also validate connector data, anchors, output count, and payouts. Tests cover valid and rejected cases. ChangesFinalization authorization and output validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SubmitFinalization
participant InputValidation
participant ConnectorLookup
participant TaprootSigner
Client->>SubmitFinalization: submit finalization intent and PSBTs
SubmitFinalization->>InputValidation: validate intent and authorized inputs
InputValidation->>ConnectorLookup: resolve connector leaf output
ConnectorLookup->>InputValidation: return validated connector output
InputValidation->>TaprootSigner: sign validated Taproot inputs
TaprootSigner->>SubmitFinalization: return signatures
SubmitFinalization->>Client: return finalized signatures
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/application/finalization_test.go (2)
126-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the table to cover the remaining branches.
Three branches of
validateForfeitOutputsare not covered: the output-count check, the nil connector witness UTXO, and a wrong anchor value with a correct anchorPkScript.💚 Proposed additional cases
{ name: "missing anchor", mutate: func(ptx *psbt.Packet) { ptx.UnsignedTx.TxOut[1].PkScript = []byte{txscript.OP_TRUE} }, wantErr: "is not the anchor output", }, + { + name: "wrong anchor value", + mutate: func(ptx *psbt.Packet) { + ptx.UnsignedTx.TxOut[1].Value = txutils.ANCHOR_VALUE + 1 + }, + wantErr: "is not the anchor output", + }, + { + name: "missing connector witness utxo", + mutate: func(ptx *psbt.Packet) { + ptx.Inputs[1].WitnessUtxo = nil + }, + wantErr: "does not match the connector tree", + }, + { + name: "extra output", + mutate: func(ptx *psbt.Packet) { + ptx.UnsignedTx.AddTxOut(&wire.TxOut{Value: 1, PkScript: []byte{txscript.OP_TRUE}}) + }, + wantErr: "expected 2 outputs", + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/finalization_test.go` around lines 126 - 153, Extend the tests table for validateForfeitOutputs with cases covering an invalid output count, a nil connector WitnessUtxo, and an anchor output retaining the correct PkScript but using an incorrect value. Set each case’s mutate function and expected error substring to exercise the corresponding validation branch.
63-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where the
PkScriptdiffers.The "different prevout" case changes only
Value. Thebytes.Equal(input.WitnessUtxo.PkScript, a.prevout.PkScript)branch invalidateFinalizationInputstays uncovered. A regression that drops thePkScriptcomparison would still pass this table.💚 Proposed additional case
{ name: "different prevout", proofLeaf: authorizedLeaf, commitmentLeaf: authorizedLeaf, commitment: &wire.TxOut{Value: prevout.Value - 1, PkScript: prevout.PkScript}, wantErr: "witness UTXO does not match intent proof", }, + { + name: "different pkScript", + proofLeaf: authorizedLeaf, + commitmentLeaf: authorizedLeaf, + commitment: &wire.TxOut{Value: prevout.Value, PkScript: []byte{txscript.OP_TRUE}}, + wantErr: "witness UTXO does not match intent proof", + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/finalization_test.go` around lines 63 - 69, Add a separate table case for “different prevout” in the finalization tests that keeps the value unchanged but uses a different PkScript, expecting “witness UTXO does not match intent proof.” This must exercise the bytes.Equal PkScript comparison branch in validateFinalizationInput.internal/application/finalization.go (1)
291-317: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftBind the connector tree inside
SubmitFinalization.
leafOutputonly checks that the connector outpoint exists infinalization.ConnectorTree; it does not check that that tree is the commitment transaction forfinalization.CommitmentTx. The gRPC handler currently callsverifyTreeRelatedToCommitmentbeforeSubmitFinalization, but the binding check belongs in the application API so the authorization invariant always applies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/finalization.go` around lines 291 - 317, Update SubmitFinalization to verify that finalization.ConnectorTree is bound to finalization.CommitmentTx before using leafOutput or authorizing finalization. Reuse verifyTreeRelatedToCommitment for this validation, and ensure the API rejects mismatched trees independently of the gRPC handler’s existing check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/application/finalization.go`:
- Around line 150-158: Update the Taproot leaf validation around tapLeaf and
input.TaprootLeafScript to inspect every entry instead of only index 0. Require
exactly one authorized leaf or verify that all entries match a.TapLeaf(), while
preserving the existing missing-script and mismatch error behavior.
- Around line 66-70: Update validateForfeitOutputs and its caller in
BatchFinalization to validate TxOut[0].PkScript against the service’s expected
forfeit script derived from configuration or arkdPubKey. Reject any mismatching
destination before accepting the forfeit, while preserving the existing output
value and anchor validations.
---
Nitpick comments:
In `@internal/application/finalization_test.go`:
- Around line 126-153: Extend the tests table for validateForfeitOutputs with
cases covering an invalid output count, a nil connector WitnessUtxo, and an
anchor output retaining the correct PkScript but using an incorrect value. Set
each case’s mutate function and expected error substring to exercise the
corresponding validation branch.
- Around line 63-69: Add a separate table case for “different prevout” in the
finalization tests that keeps the value unchanged but uses a different PkScript,
expecting “witness UTXO does not match intent proof.” This must exercise the
bytes.Equal PkScript comparison branch in validateFinalizationInput.
In `@internal/application/finalization.go`:
- Around line 291-317: Update SubmitFinalization to verify that
finalization.ConnectorTree is bound to finalization.CommitmentTx before using
leafOutput or authorizing finalization. Reuse verifyTreeRelatedToCommitment for
this validation, and ensure the API rejects mismatched trees independently of
the gRPC handler’s existing check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 343ce729-7dea-4b6b-a400-8e4ef9a880ba
📒 Files selected for processing (2)
internal/application/finalization.gointernal/application/finalization_test.go
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — #126
The vulnerability is real and the fix is structurally correct: signedInputAssociation now carries the proof's exact prevout and TapLeaf hash, both are verified before every sign call, and the arkd cosigner requirement is enforced on the proof's decoded closure. I have two mandatory items before this should land.
[C1] Forfeit validateFinalizationInput path is entirely untested
SubmitFinalization calls association.validateFinalizationInput(forfeit, inputIndex, s.arkdPubKey) inside the forfeit loop (the new block after if !ok { continue } in the forfeit iteration). Neither TestSubmitFinalizationValidatesAuthorizedInput (which passes no Forfeits) nor TestValidateForfeitOutputs (which calls validateForfeitOutputs directly, bypassing SubmitFinalization) exercises this code path.
The four rejection cases — foreign-leaf, prevout mismatch, no-arkd-signer, and connector mismatch — are tested only for the commitment-tx branch. The forfeit branch runs a different call site with a different *psbt.Packet argument (forfeit vs. finalization.CommitmentTx) and is completely dark.
A future refactor could delete or skip these checks on the forfeit path and the test suite would still pass. For a security fix this is not acceptable. A SubmitFinalization call with a real forfeit and a minimal connector tree is needed, covering at least: valid forfeit, foreign-leaf rejection, prevout-mismatch rejection.
[C2] leafOutput returns an aliased pointer into the TxTree (finalization.go)
output := node.Root.UnsignedTx.TxOut[outpoint.Index]
// ...
return outputwire.MsgTx.TxOut is []*wire.TxOut, so TxOut[outpoint.Index] is a live pointer into the tree's slice. This pointer is held as connectorOutput and then read in validateForfeitOutputs. If the connector tree is concurrently accessed or mutated between leafOutput and the value/pkscript comparisons, the validation sees inconsistent data. Return a defensive copy:
out := *node.Root.UnsignedTx.TxOut[outpoint.Index]
return &outThis is one extra allocation per forfeit and eliminates the aliasing entirely.
[N1] Zero-fee enforcement deserves a comment (finalization.go)
expectedValue := vtxoPrevout.Value + connectorOutput.Value - txutils.ANCHOR_VALUEWith ANCHOR_VALUE = 0 this reduces to vtxo + connector, enforcing zero in-band miner fees. This is correct — the P2A ephemeral anchor carries fees via CPFP — but the formula looks like it may have intended to leave room for fees. A one-line comment (// ANCHOR_VALUE is 0; fees are paid by CPFP through the ephemeral anchor) prevents future maintainers from treating this as a bug.
What is correct
- Core binding is sound: proof prevout + TapHash are stored in
signedInputAssociationand compared byte-for-byte before every sign call in both the forfeit and commitment-tx branches. containsPubKey(a.script.ClosurePubKeys(), arkdPubKey)checks the closure pubkeys decoded from the proof's tapscript — not the finalization PSBT's self-reported fields — so the arkd-cosigner requirement cannot be spoofed by crafting a finalization PSBT alone.- The defensive
PkScriptcopy ingetSignedInputAssociations(append([]byte(nil), input.WitnessUtxo.PkScript...)) is correct. TaprootLeafScript[0]is used consistently inReadArkadeScript,validateFinalizationInput, andsignInput; there is no inconsistency between what is validated and what is signed.- Connector WitnessUtxo is validated against the on-tree
connectorOutputvalue and pkscript, closing the connector-substitution vector. validateForfeitOutputscorrectly checks output count before indexingTxOut[1].- The deprecated-signer path still flows through
validateFinalizationInput, so key rotation does not create a bypass window.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/application/finalization_test.go (1)
391-391: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise ambiguous Taproot leaf descriptors.
Line 391 always inserts one non-nil descriptor. The tests cannot verify rejection when the input has no non-nil descriptor or has multiple non-nil descriptors.
Make the fixture accept a leaf slice. Add failure cases that assert an error and no
TaprootScriptSpendSig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/finalization_test.go` at line 391, Update the test fixture around Taproot leaf setup to accept a configurable leaf slice instead of always inserting one non-nil descriptor. Add cases for no non-nil descriptor and multiple non-nil descriptors, asserting each returns an error and leaves TaprootScriptSpendSig unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/application/finalization_test.go`:
- Line 391: Update the test fixture around Taproot leaf setup to accept a
configurable leaf slice instead of always inserting one non-nil descriptor. Add
cases for no non-nil descriptor and multiple non-nil descriptors, asserting each
returns an error and leaves TaprootScriptSpendSig unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eee6ce0-2447-494b-b2d7-d155ba45d995
📒 Files selected for processing (2)
internal/application/finalization_test.gointernal/application/signer.go
|
Addressed in 64c6557. C1 — valid. Added C2 — not applying. The N1 — skipped, repo convention is to keep comments to a minimum. Also hardened Integration suite is green against arkd v0.9.13 (21/21), which confirms real clients never attach more than one leaf script per input. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 new commit (64c6557) since last pass — C1 fixed, C2 still open (author declined), N1 still open (author declined by convention). No regressions introduced. Two minor new findings.
Prior findings closure
C1 — Fixed (64c6557). TestSubmitFinalizationValidatesForfeit drives SubmitFinalization with a real intent proof, a real forfeit PSBT, and a single-leaf connector tree. It covers all four rejection cases I required: foreign-leaf, prevout-mismatch, no-arkd-signer, and connector-outside-tree. The signed-output assertion on the success case is also present. This was the mandatory blocker; it is resolved.
C2 — Still open. leafOutput (finalization.go) still returns a raw pointer into the tree's TxOut slice — not a copy. Author's rebuttal (single-goroutine, tree built per-request) is accurate for the current implementation, so there is no live race today. I continue to flag it because the defensive copy is one allocation and permanently eliminates the aliasing if the request model ever changes. Marking as low-risk/informational this pass.
N1 — Still open. The zero-fee comment was skipped per repo convention. Acknowledged.
New findings in this diff
[N2] validateFinalizationInput guard is inconsistent with the hardened signInput guard — finalization.go + signer.go
signInput now rejects any input where len(TaprootLeafScript) != 1 (signer.go:31). validateFinalizationInput, added in the previous commit, still uses the old guard: len == 0 || [0] == nil (finalization.go, in the validateFinalizationInput block). An input carrying two leaf scripts passes validateFinalizationInput and is only caught later by signInput with a different error string.
There is no security hole — the input is still rejected before any signature is produced. The concern is maintenance: the two functions are now out of step, and the validation layer no longer fully describes what the signing layer accepts. If signInput is later relaxed or the ordering changes, the gap becomes exploitable.
Fix: update validateFinalizationInput's guard to len(input.TaprootLeafScript) != 1 || input.TaprootLeafScript[0] == nil to match signInput.
[N3] The new len != 1 rejection path in signInput has no test — finalization_test.go
TestSubmitFinalizationValidatesForfeit and TestSubmitFinalizationValidatesAuthorizedInput both wire exactly one leaf script per input (the happy path). There is no test case that submits a forfeit or commitment PSBT where len(TaprootLeafScript) == 0 or len == 2 and asserts that an error is returned without a signature. The new guard in 64c6557 is therefore not exercised by the test suite.
A minimal table entry in either test would close this:
{
name: "two taproot leaf scripts on forfeit input",
// ... same valid proof / prevout / connector,
// after building forfeit:
// forfeit.Inputs[0].TaprootLeafScript = append(
// forfeit.Inputs[0].TaprootLeafScript,
// forfeit.Inputs[0].TaprootLeafScript[0],
// )
wantErr: "expected exactly 1 taproot leaf script",
},What is correct in this diff
- The
len != 1change insignInputis the right fix for the ambiguity: silently signing index 0 when multiple leaves are declared could permit a requester to smuggle a leaf that passes validation but differs from the one that is signed. - The incremental diff additions to
TestValidateForfeitOutputs(missing connector witness UTXO, extra output, wrong anchor value) close the branch-coverage gaps I called out in the CodeRabbit thread. - The new "different prevout script" case in
TestSubmitFinalizationValidatesAuthorizedInputcloses the partial-match gap (value matches but script does not) that was missing before. finalizationConnectorTreecorrectly useschainhash.NewHashFromStr(ptx.UnsignedTx.TxID())rather than computing the hash from the wire-serialized tx directly, which matches how the production code identifies tree nodes.- The test for "connector outside the tree" uses
wire.OutPoint{Hash: chainhash.Hash{9}}, which cannot collide with the connector tx hash (derived from a fresh PSBT), so the test is deterministic.
N2 and N3 are minor; N2 is the more important of the two because it leaves the validation and signing layers out of step. I am requesting changes on that basis, but both items are small fixes.
76fd5b7 to
bd9764c
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 new commit (76fd5b7, "enforce intent proof validity window on finalization") since last pass. C1 remains closed. C2, N1, N2, N3 are all still open — none were touched by this commit. One new finding (N4).
Prior findings closure
C1 — Closed (64c6557). Test coverage for the forfeit finalization path was added and is adequate.
C2 — Still open. leafOutput (finalization.go:310) still returns a raw pointer into the live TxOut slice (return output where output := node.Root.UnsignedTx.TxOut[outpoint.Index]). Not touched by 76fd5b7. The aliasing risk is low-severity under the current single-goroutine model, but the defensive copy remains a one-line fix.
N1 — Still open. Zero-fee path skipped by repo convention. Acknowledged.
N2 — Still open. validateFinalizationInput (finalization.go, in the signedInputAssociation method added in the PR) guards with len(input.TaprootLeafScript) == 0 || input.TaprootLeafScript[0] == nil, while signInput (signer.go:38) now guards with len(input.TaprootLeafScript) != 1 || input.TaprootLeafScript[0] == nil. An input with two leaf scripts passes the validation layer but is rejected by the signing layer with a different error string. The two guards are now permanently out of step. 76fd5b7 does not change either function. Fix: change validateFinalizationInput to len(input.TaprootLeafScript) != 1.
N3 — Still open. No test submits a forfeit or commitment PSBT where len(TaprootLeafScript) == 0 or len == 2 and asserts an error without a signature. The len != 1 guard in signInput (signer.go:38) added in this PR is not exercised by any test case.
New findings in 76fd5b7
[N4] validateMessage rejects nil Intent.Message with a misleading error — finalization.go:22
if err := validateMessage(finalization.Intent.Message); err != nil {
return nil, fmt.Errorf("invalid message: %w", err)
}validateMessage is called with finalization.Intent.Message, which is an interface (IntentMessage). When the field is the zero value (nil interface), the type switch falls through to default and returns "unsupported intent message type". The caller sees "invalid message: unsupported intent message type" rather than anything that identifies the message as absent.
This is consistent with the identical guard in SubmitIntent (intent.go:19), so it is not a regression specific to this commit. However, 76fd5b7 is the first time the guard appears in the SubmitFinalization path, and any existing BatchFinalization caller that omits Intent.Message (nil) will now receive this error where previously the call succeeded. The change is intentionally breaking for that class of caller; the error message makes the cause opaque. A nil guard (if finalization.Intent.Message == nil { return nil, fmt.Errorf("missing intent message") }) before the validateMessage call would make the failure mode self-describing.
Additionally: validateMessage skips the expiry check when expireAt == 0, making any message with no ExpireAt field set unconditionally valid (intent.go:125: if expireAt > 0 && ...). This is existing behavior carried over from SubmitIntent, not introduced here, but it now also gates SubmitFinalization signing — a proof with ExpireAt == 0 is permanently authorizing. Worth a deliberate protocol decision record.
What is correct in 76fd5b7
- Placing
validateMessagebeforegetSignedInputAssociationsis correct: it short-circuits on expired proofs without doing any crypto work, and it closes the replay window that existed when an expired intent message could still authorize new forfeit or commitment signatures. TestSubmitFinalizationRejectsExpiredProofis well-constructed: it builds a valid proof (cryptographic check would pass), overwrites the message with a one-minute-past expiry, callsSubmitFinalization, asserts the specific error string, and asserts that noTaprootScriptSpendSigwas written to the PSBT. This is the right shape for a security regression test.- Updating
signedFinalizationIntentto carry a one-hour-future message keeps all pre-existing tests valid under the new guard without requiring per-test changes.
d42fc79 to
95ee9d7
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 new commit (95ee9d7, "rebase(master): merged tests and deduped") since last pass — C1 remains closed; C2, N1, N2, N3 still open; N4 regressed (the expiry check present in 76fd5b7 is gone in the rebased head). Two new findings (C3, N5). Note: PR is already merged — findings below require a follow-up fix commit.
Prior findings closure
C1 — Closed (64c6557). TestSubmitFinalizationValidatesForfeit remains in the rebased test file as TestSubmitFinalizationValidatesForfeitOutputs. Coverage of the forfeit finalization path is intact.
C2 — Still open. leafOutput (finalization.go:383) still returns output := node.Root.UnsignedTx.TxOut[outpoint.Index] — a live pointer into the tree's slice, not a copy. Not touched by 95ee9d7. Low-risk under the single-goroutine model but still trivially fixable.
N1 — Still open. No zero-fee comment added. Acknowledged per repo convention.
N2 — Still open. validateFinalizationInput (finalization.go:183) guards with len(input.TaprootLeafScript) == 0 || input.TaprootLeafScript[0] == nil; signInput (signer.go:39) guards with len(input.TaprootLeafScript) != 1 || input.TaprootLeafScript[0] == nil. The two-leaf case passes the validation layer and is only caught at signing with a different error. Both functions are present unchanged in 95ee9d7.
N3 — Still open. The new TestSignInputRejectsUncommittedTaprootLeaf tests a swapped leaf and a nil control block, but no test submits a PSBT with len(TaprootLeafScript) == 0 or len == 2 through validateFinalizationInput. The len != 1 guard in signInput added in this PR remains untested at the finalization-validation layer.
N4 — Regressed. validateMessage(finalization.Intent.Message) was added in 76fd5b7 and was the subject of a positive finding in the last review. The rebased head (95ee9d7) has dropped it entirely from SubmitFinalization. See C3 below.
New findings in 95ee9d7
[C3] validateMessage expiry check dropped from SubmitFinalization — finalization.go
finalization.go currently has no call to validateMessage. It was added in 76fd5b7 and TestSubmitFinalizationRejectsExpiredProof exercised it. Both are absent from 95ee9d7 — the rebase merged two branches and the expiry guard did not survive.
Impact: An intent proof with a past ExpireAt now authorises new forfeit and commitment-tx signatures indefinitely. The new indexer check (C3-adjacent) requires the commitment txid to be known to arkd and is a valuable gate, but it does not enforce the proof's time window. A proof created for a round that completed three months ago — and whose ExpireAt has long since passed — will be accepted by SubmitFinalization as long as its commitment tx still appears in the indexer.
Restore before any further merges:
// At the top of SubmitFinalization, before getSignedInputAssociations:
if err := validateMessage(finalization.Intent.Message); err != nil {
return nil, fmt.Errorf("invalid message: %w", err)
}And restore TestSubmitFinalizationRejectsExpiredProof.
[N5] Duplicate sighash guard in signInput — signer.go:26-30 and signer.go:56-61
// line 26-30: first check
if input.SighashType != txscript.SigHashDefault && input.SighashType != txscript.SigHashAll {
return fmt.Errorf("unsupported sighash type 0x%02x, cannot sign", uint32(input.SighashType))
}
// ... taproot check, leaf-count check, VerifyTaprootLeafCommitment ...
// line 56-61: second identical check
if input.SighashType != txscript.SigHashDefault && input.SighashType != txscript.SigHashAll {
return fmt.Errorf("unsupported sighash type 0x%02x, cannot sign", byte(input.SighashType))
}The second block (lines 56-61) is dead code: any sighash type that fails the first check at line 28 never reaches line 56. This is a rebase artefact from two PRs (#130 and the current) each adding the check independently. No security impact — the first check is strict — but the dead block causes TestSignInputRejectsPartialSighashTypes to test the first occurrence only. Remove lines 54-61.
What is correct in 95ee9d7
arkade.VerifyTaprootLeafCommitment(signer.go:48-52) is a genuine new security check: it verifies the declared TapLeaf is actually committed by the prevout's taptree using the control block, closing the leaf-swap vector where a requester could replace the leaf script after the prevout check.TestSignInputRejectsUncommittedTaprootLeafcovers the committed case, the swapped-to-outside-tree case, and the missing-control-block case.- The indexer-based commitment-tx gate (
retryWithBackoffagainstGetCommitmentTx,finalization.go:37-49) is a sound additional check: it prevents a replayed or fabricated proof from authorising a commitment tx that arkd never built.TestSubmitFinalizationRejectsUnknownCommitmentTxoverridescommitmentTxRetryConfigcorrectly viat.Cleanupand asserts no signature is produced. TestSignInputRejectsPartialSighashTypesandTestSignInputRejectsUnsafeSighashTypestogether cover all ANYONECANPAY and partial-commit sighash types.TestSubmitFinalizationValidatesForfeitOutputsis a comprehensive table-driven set covering the canonical forfeit, redirected value, split outputs, replaced anchor, short-pay, inflated prevout, forged connector prevout, and out-of-tree connector.s.activeDeprecatedSigners()replacings.deprecatedSignersis correct; the active-only filter ensures key-rotation gaps do not create bypass windows.
C3 is a mandatory fix. N5 is a cleanup. Both should land as a follow-up before the next protocol round.
Summary
Root cause
SubmitFinalization treated a valid emulator signature in an intent proof as authorization for any later transaction matching the same outpoint. signer.signInput then selected the tapleaf and transaction data from that later requester-controlled PSBT.
Impact
A requester could substitute another committed leaf containing the same tweaked emulator key and obtain a signature over a transaction that no Arkade script execution approved. The fix binds the reusable proof association to its exact leaf and prevout and ensures every finalization path retains an honest arkd cosigner.
Validation
Closes #120
Summary by CodeRabbit
Bug Fixes
Tests