From d28d1b037be66615c2460d79591f85d6596e80fb Mon Sep 17 00:00:00 2001 From: sputn1ck Date: Thu, 13 Aug 2026 15:57:00 +0200 Subject: [PATCH 1/2] arkscript: Reject unsafe condition predicates Opaque condition predicates were assumed to only add restrictions, but OP_SUCCESS makes tapscript succeed before the typed inner clause executes. A partial data push can also consume that inner clause as push data. Require predicates to parse as complete fragments, reject OP_SUCCESS, and compile each leaf during direct policy validation so every admission path fails closed. --- docs/arkscript_spec.md | 16 +++-- docs/policy_arkscript_review_guide.md | 12 ++-- lib/arkscript/node.go | 24 ++++++++ lib/arkscript/node_test.go | 85 +++++++++++++++++++++++++++ lib/arkscript/policy_template_test.go | 3 +- lib/arkscript/validate.go | 17 +++++- lib/arkscript/validate_test.go | 30 +++++++++- 7 files changed, 171 insertions(+), 16 deletions(-) diff --git a/docs/arkscript_spec.md b/docs/arkscript_spec.md index 3b4dba39b..23bdf21fe 100644 --- a/docs/arkscript_spec.md +++ b/docs/arkscript_spec.md @@ -111,7 +111,7 @@ type Condition struct { } ``` -Script encoding (from `Condition.Script`, `lib/arkscript/node.go:120-141`): +Script encoding (from `Condition.Script` in `lib/arkscript/node.go`): ``` @@ -119,7 +119,7 @@ Script encoding (from `Condition.Script`, `lib/arkscript/node.go:120-141`): `Condition` is the extension point for non-signature preconditions (hash locks, absolute locktimes, payment-hash preimages). Helper builders live in -`lib/arkscript/node.go:148-190`: +`lib/arkscript/node.go`: - `Hash160Condition(hash []byte)` — `HASH160 EQUALVERIFY`. - `AbsoluteLockTimeCondition(lock uint32)` — ` CLTV DROP`. @@ -127,9 +127,12 @@ locks, absolute locktimes, payment-hash preimages). Helper builders live in that also enforces the 32-byte preimage-size rule. The predicate bytes are opaque to the AST walker for the purposes of -`ContainsKey` and key extraction (`lib/arkscript/validate.go:208-218`). This -is intentional: the AST reasons about *who can sign*, not about *what -hashlock values are in play*. +`ContainsKey` and key extraction in `lib/arkscript/validate.go`. This is +intentional: the AST reasons about *who can sign*, not about *what hashlock +values are in play*. Compilation still requires the predicate to be a complete +script fragment and rejects every tapscript `OP_SUCCESSx` opcode. These checks +prevent a raw prefix from consuming the typed inner clause as push data or +making the leaf succeed before that clause executes. ### 2.3 What is NOT in the AST @@ -137,7 +140,8 @@ hashlock values are in play*. tapscript merkle tree already provides the OR semantics. - No `AND` nodes. Multisig is N-of-N; chain multiple signatures inside a single `Multisig`. CSV-gated signatures compose via `CSV{Inner: Multisig}`. -- No custom opcode escape hatch. A future node kind requires a code change +- No custom AST node escape hatch. `Condition` accepts raw predicate fragments, + subject to the safety checks above. A future node kind requires a code change in `lib/arkscript` and an encoding version bump (see §3.5). --- diff --git a/docs/policy_arkscript_review_guide.md b/docs/policy_arkscript_review_guide.md index d6b0b4eda..528d8563f 100644 --- a/docs/policy_arkscript_review_guide.md +++ b/docs/policy_arkscript_review_guide.md @@ -239,10 +239,12 @@ correctly be flagged as an ungated exit leaf and rejected. **What about Condition predicates that embed keys?** The `Condition.Predicate` is opaque bytes — the validator does not parse it for operator keys. This is -safe because the predicate only adds _restrictions_ (hashlocks, timelocks). A -predicate cannot _grant_ spending authority — that comes from the `Inner` -Multisig. The validator correctly checks only the Multisig nodes for key -presence. +safe because compilation rejects incomplete script fragments and all tapscript +`OP_SUCCESSx` opcodes. An incomplete push could otherwise consume the typed +inner script as data, while `OP_SUCCESSx` would make the leaf succeed before +the inner signature checks execute. Once those bypasses are excluded, the +predicate can only add conditions; spending authority still comes from the +`Inner` Multisig. **Edge case: empty Multisig in exit leaf?** Impossible — `Multisig.Script()` returns an error if `len(Keys) == 0`, and `PolicyTemplate.Compile()` would @@ -257,7 +259,7 @@ unspendable by anyone, which is safe (funds locked, not stolen). | Bypass CSV via key-path spend | Internal key is NUMS (unspendable) | | Fake operator key in policy | Operator validates own key presence at submit | | Ungated exit leaf injection | `ValidatePolicy` rejects non-CSV exit leaves | -| Predicate smuggling operator | Predicates add restrictions, not authority | +| Predicate bypasses inner node | Compile rejects partial pushes and `OP_SUCCESSx` | | Insufficient exit delay | `MinExitDelay` check enforces operator minimum | --- diff --git a/lib/arkscript/node.go b/lib/arkscript/node.go index 8d2239179..514a12f14 100644 --- a/lib/arkscript/node.go +++ b/lib/arkscript/node.go @@ -124,6 +124,9 @@ func (c *Condition) Script() ([]byte, error) { if len(c.Predicate) == 0 { return nil, fmt.Errorf("condition: predicate script is empty") } + if err := validateConditionPredicate(c.Predicate); err != nil { + return nil, err + } innerScript, err := c.Inner.Script() if err != nil { @@ -138,6 +141,27 @@ func (c *Condition) Script() ([]byte, error) { return builder.Script() } +// validateConditionPredicate ensures a raw predicate cannot change how the +// typed inner clause is parsed or bypass its execution. In particular, an +// incomplete data push could consume the inner script as pushed bytes, while +// OP_SUCCESS would make the entire tapscript succeed before the inner clause +// executes. +func validateConditionPredicate(predicate []byte) error { + tokenizer := txscript.MakeScriptTokenizer(0, predicate) + for tokenizer.Next() { + } + if err := tokenizer.Err(); err != nil { + return fmt.Errorf("condition: predicate is not a complete "+ + "script fragment: %w", err) + } + if txscript.ScriptHasOpSuccess(predicate) { + return fmt.Errorf("condition: predicate contains OP_SUCCESS " + + "opcode") + } + + return nil +} + // nodeSealed implements the Node interface. func (c *Condition) nodeSealed() {} diff --git a/lib/arkscript/node_test.go b/lib/arkscript/node_test.go index e8c613376..bd49790b9 100644 --- a/lib/arkscript/node_test.go +++ b/lib/arkscript/node_test.go @@ -207,6 +207,91 @@ func TestConditionNilInner(t *testing.T) { require.Contains(t, err.Error(), "inner node is nil") } +// TestConditionRejectsOpSuccess verifies that opaque predicates cannot use +// tapscript's immediate-success opcodes to bypass the typed inner clause. +func TestConditionRejectsOpSuccess(t *testing.T) { + t.Parallel() + + key, _ := testutils.CreateKey(1) + checked := 0 + + for opcode := 0; opcode <= 0xff; opcode++ { + predicate := []byte{byte(opcode)} + if !txscript.ScriptHasOpSuccess(predicate) { + continue + } + + checked++ + node := &Condition{ + Predicate: predicate, + Inner: &Multisig{ + Keys: []*btcec.PublicKey{ + key, + }, + }, + } + + _, err := node.Script() + require.Error(t, err, "opcode 0x%x", opcode) + require.Contains( + t, err.Error(), + "OP_SUCCESS", "opcode 0x%x", opcode, + ) + } + + require.NotZero(t, checked) +} + +// TestConditionAllowsOpSuccessByteInPushData verifies that an OP_SUCCESS byte +// carried as push data is not mistaken for an executable opcode. +func TestConditionAllowsOpSuccessByteInPushData(t *testing.T) { + t.Parallel() + + predicate, err := txscript.NewScriptBuilder(). + AddData([]byte{txscript.OP_RESERVED}). + AddOp(txscript.OP_DROP). + Script() + require.NoError(t, err) + require.False(t, txscript.ScriptHasOpSuccess(predicate)) + + key, _ := testutils.CreateKey(1) + node := &Condition{ + Predicate: predicate, + Inner: &Multisig{ + Keys: []*btcec.PublicKey{ + key, + }, + }, + } + + _, err = node.Script() + require.NoError(t, err) +} + +// TestConditionRejectsIncompletePush verifies that a predicate cannot consume +// the typed inner script as data by ending with an incomplete push opcode. +func TestConditionRejectsIncompletePush(t *testing.T) { + t.Parallel() + + key, _ := testutils.CreateKey(1) + node := &Condition{ + // A single-key Multisig compiles to exactly 34 bytes. Without + // standalone predicate validation, this opcode consumes that + // entire signature clause as push data. + Predicate: []byte{ + txscript.OP_DATA_34, + }, + Inner: &Multisig{ + Keys: []*btcec.PublicKey{ + key, + }, + }, + } + + _, err := node.Script() + require.ErrorContains(t, err, "not a complete script fragment") +} + // TestASTMatchesGoldenVectors verifies that the AST produces byte-identical // scripts to the golden test vectors from the current implementation. func TestASTMatchesGoldenVectors(t *testing.T) { diff --git a/lib/arkscript/policy_template_test.go b/lib/arkscript/policy_template_test.go index a2b86ac10..5cd2aab85 100644 --- a/lib/arkscript/policy_template_test.go +++ b/lib/arkscript/policy_template_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/btcsuite/btcd/btcec/v2" + "github.com/btcsuite/btcd/txscript/v2" "github.com/lightninglabs/wavelength/internal/testutils" "github.com/stretchr/testify/require" ) @@ -239,7 +240,7 @@ func nestedConditionNode(t *testing.T, depth int) Node { for i := 0; i < depth; i++ { node = &Condition{ Predicate: []byte{ - 0x01, + txscript.OP_NOP, }, Inner: node, } diff --git a/lib/arkscript/validate.go b/lib/arkscript/validate.go index 266658386..6966722c6 100644 --- a/lib/arkscript/validate.go +++ b/lib/arkscript/validate.go @@ -55,10 +55,21 @@ func ValidatePolicy(nodes []Node, opts PolicyValidationOpts) error { foundCSV bool ) - // Invariant 3 is enforced across every leaf, not just collab leaves, - // so a malformed exit leaf that nevertheless includes the operator - // is still rejected. + // Compile every leaf before reasoning about its structure. This keeps + // direct callers from accepting a Condition predicate that can bypass + // or consume the typed inner clause. for i, node := range nodes { + if node == nil { + return fmt.Errorf("leaf %d is nil", i) + } + + if _, err := node.Script(); err != nil { + return fmt.Errorf("leaf %d is invalid: %w", i, err) + } + + // Invariant 3 is enforced across every leaf, not just collab + // leaves, so a malformed exit leaf that nevertheless includes + // the operator is still rejected. if err := rejectOperatorUnilateral( node, opts.OperatorKey, ); err != nil { diff --git a/lib/arkscript/validate_test.go b/lib/arkscript/validate_test.go index 7ae5ceab1..3b9594555 100644 --- a/lib/arkscript/validate_test.go +++ b/lib/arkscript/validate_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/btcsuite/btcd/btcec/v2" + "github.com/btcsuite/btcd/txscript/v2" "github.com/lightninglabs/wavelength/internal/testutils" "github.com/lightningnetwork/lnd/lntypes" "github.com/stretchr/testify/require" @@ -232,6 +233,33 @@ func TestValidatePolicy(t *testing.T) { require.NoError(t, err) }) + t.Run("OP_SUCCESS condition predicate rejected", func(t *testing.T) { + t.Parallel() + + collabNode := &Condition{ + Predicate: []byte{ + txscript.OP_RESERVED, + }, + Inner: &Multisig{ + Keys: []*btcec.PublicKey{ + owner, + operator, + }, + }, + } + exitNode := &CSV{ + Lock: 100, + Inner: &Multisig{ + Keys: []*btcec.PublicKey{ + owner, + }, + }, + } + + err := ValidatePolicy([]Node{collabNode, exitNode}, opts) + require.ErrorContains(t, err, "OP_SUCCESS") + }) + t.Run("collab leaf missing operator key", func(t *testing.T) { t.Parallel() @@ -496,7 +524,7 @@ func TestValidatePolicy(t *testing.T) { } operatorOnlyCondition := &Condition{ Predicate: []byte{ - 0x01, + txscript.OP_NOP, }, Inner: &Multisig{ Keys: []*btcec.PublicKey{ From dfb8ad8b87439f8656d7d94803af138cc72e7a5a Mon Sep 17 00:00:00 2001 From: sputn1ck Date: Thu, 13 Aug 2026 16:50:13 +0200 Subject: [PATCH 2/2] arkscript: Reject noncanonical CSV locks Policy validation compared raw CSV operands even though consensus masks reserved bits and treats the disable bit as a no-op. Restrict typed CSV locks to canonical non-zero block delays so structural validation and script execution enforce the same value. --- docs/arkscript_spec.md | 8 ++++-- docs/policy_arkscript_review_guide.md | 10 +++++-- lib/arkscript/AGENTS.md | 3 ++ lib/arkscript/CLAUDE.md | 3 ++ lib/arkscript/checkpoint.go | 7 ++--- lib/arkscript/node.go | 22 ++++++++++++++- lib/arkscript/node_test.go | 37 +++++++++++++++++++++++++ lib/arkscript/standard_vtxo.go | 9 ++++-- lib/arkscript/standard_vtxo_test.go | 37 +++++++++++++++++++++++-- lib/arkscript/validate_test.go | 37 +++++++++++++++++++++++++ lib/arkscript/vhtlc.go | 24 ++++++++++++++++ lib/arkscript/vhtlc_test.go | 40 +++++++++++++++++++++++++++ lib/tx/checkpoint/build_test.go | 26 +++++++++++++++++ 13 files changed, 247 insertions(+), 16 deletions(-) diff --git a/docs/arkscript_spec.md b/docs/arkscript_spec.md index 23bdf21fe..db6bc0f32 100644 --- a/docs/arkscript_spec.md +++ b/docs/arkscript_spec.md @@ -86,12 +86,12 @@ witness script on the stack). ```go type CSV struct { - Lock uint32 // BIP-68 encoded sequence value + Lock uint32 // canonical block-mode BIP-68 value, 1..65535 Inner Node } ``` -Script encoding (from `CSV.Script`, `lib/arkscript/node.go:82-101`): +Script encoding (from `CSV.Script` in `lib/arkscript/node.go`): ``` OP_CHECKSEQUENCEVERIFY OP_DROP @@ -100,7 +100,9 @@ Script encoding (from `CSV.Script`, `lib/arkscript/node.go:82-101`): At runtime the spending transaction's input `nSequence` must satisfy the BIP-68 relative-lock relation to the UTXO's confirmation height. Callers read the required sequence from a `SpendPath.RequiredSequence` computed by -`DeriveSequence` (see §6). +`DeriveSequence` (see §6). Ark policies support block-mode CSV only. Compilation +rejects zero, the time-mode and disable flags, and every reserved high bit so +policy validation compares exactly the value consensus enforces. #### `Condition` — generic predicate prefix diff --git a/docs/policy_arkscript_review_guide.md b/docs/policy_arkscript_review_guide.md index 528d8563f..177c21326 100644 --- a/docs/policy_arkscript_review_guide.md +++ b/docs/policy_arkscript_review_guide.md @@ -147,7 +147,10 @@ the owner can recover funds without operator cooperation. `ExtractCSVDelay(node)` walks the AST for the outermost `CSV` node. If an exit leaf has no CSV wrapper, the policy is rejected. This is the critical safety -property — see the security analysis below. +property — see the security analysis below. Compilation also requires the CSV +operand to be a canonical block-mode BIP-68 value in `1..65535`. This rejects +the disable flag, time-mode flag, and reserved bits before structural policy +validation compares the delay. ### Invariant 4: Minimum exit delay @@ -213,7 +216,9 @@ unilateral refund leaf is reachable only after the invoice expiry. - The UTXO must have been confirmed for at least that many blocks. This means an exit spend is physically impossible until the CSV delay has - passed since the on-chain commitment. + passed since the on-chain commitment. The compiler's canonical block-mode + check is essential here: consensus treats a CSV operand with bit 31 set as + a NOP and masks reserved high bits during comparison. 5. During that delay window, the operator can observe the unilateral exit attempt and broadcast the appropriate **forfeit transaction** (which uses a @@ -260,6 +265,7 @@ unspendable by anyone, which is safe (funds locked, not stolen). | Fake operator key in policy | Operator validates own key presence at submit | | Ungated exit leaf injection | `ValidatePolicy` rejects non-CSV exit leaves | | Predicate bypasses inner node | Compile rejects partial pushes and `OP_SUCCESSx` | +| CSV flag or reserved-bit bypass | Compile permits only block delays `1..65535` | | Insufficient exit delay | `MinExitDelay` check enforces operator minimum | --- diff --git a/lib/arkscript/AGENTS.md b/lib/arkscript/AGENTS.md index fa0eccffa..bc7d5c52a 100644 --- a/lib/arkscript/AGENTS.md +++ b/lib/arkscript/AGENTS.md @@ -74,6 +74,9 @@ validated invariants. - CSV encoding uses `blockchain.LockTimeToSequence(false, exitDelay)` to store the BIP-68 block-mode sequence value, not the raw block count. Decoders that compare raw block counts against CSV lock values must convert accordingly. +- CSV values are canonical non-zero block-mode encodings in `1..65535`. + Time-mode, disable, and reserved high bits are rejected so structural delay + comparisons cannot diverge from the value enforced by consensus. - Canonical leaf ordering: sorted by version then lexicographic script bytes. - All taproot outputs use the unspendable ARK NUMS key for key path (no key-path spend possible). diff --git a/lib/arkscript/CLAUDE.md b/lib/arkscript/CLAUDE.md index fa0eccffa..bc7d5c52a 100644 --- a/lib/arkscript/CLAUDE.md +++ b/lib/arkscript/CLAUDE.md @@ -74,6 +74,9 @@ validated invariants. - CSV encoding uses `blockchain.LockTimeToSequence(false, exitDelay)` to store the BIP-68 block-mode sequence value, not the raw block count. Decoders that compare raw block counts against CSV lock values must convert accordingly. +- CSV values are canonical non-zero block-mode encodings in `1..65535`. + Time-mode, disable, and reserved high bits are rejected so structural delay + comparisons cannot diverge from the value enforced by consensus. - Canonical leaf ordering: sorted by version then lexicographic script bytes. - All taproot outputs use the unspendable ARK NUMS key for key path (no key-path spend possible). diff --git a/lib/arkscript/checkpoint.go b/lib/arkscript/checkpoint.go index b2685b389..75955198f 100644 --- a/lib/arkscript/checkpoint.go +++ b/lib/arkscript/checkpoint.go @@ -20,11 +20,8 @@ type CheckpointPolicy struct { // CSV unroll leaf. OperatorKey *btcec.PublicKey - // CSVDelay is the relative timelock enforced by the - // operator-controlled leaf. - // - // This is a raw BIP-68 sequence value interpreted by - // OP_CHECKSEQUENCEVERIFY. + // CSVDelay is the canonical block delay enforced by the + // operator-controlled leaf. It must be in the range 1..65535. CSVDelay uint32 } diff --git a/lib/arkscript/node.go b/lib/arkscript/node.go index 514a12f14..bc9aa5246 100644 --- a/lib/arkscript/node.go +++ b/lib/arkscript/node.go @@ -6,6 +6,7 @@ import ( "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcec/v2/schnorr" "github.com/btcsuite/btcd/txscript/v2" + "github.com/btcsuite/btcd/wire/v2" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lntypes" ) @@ -70,7 +71,7 @@ func (m *Multisig) nodeSealed() {} // The inner expression is evaluated first, then the CSV check is performed. // Canonical encoding: OP_CHECKSEQUENCEVERIFY OP_DROP. type CSV struct { - // Lock is the BIP-68 encoded relative locktime value. + // Lock is a canonical block-mode BIP-68 delay in the range 1..65535. Lock uint32 // Inner is the expression to gate with the timelock. @@ -82,6 +83,9 @@ func (c *CSV) Script() ([]byte, error) { if c.Inner == nil { return nil, fmt.Errorf("csv: inner node is nil") } + if err := validateCSVLock(c.Lock); err != nil { + return nil, err + } // Compile the inner expression first. innerScript, err := c.Inner.Script() @@ -99,6 +103,22 @@ func (c *CSV) Script() ([]byte, error) { return builder.Script() } +// validateCSVLock ensures a CSV operand is the canonical BIP-68 encoding of a +// non-zero block delay. Ark policies do not support time-mode CSV values, and +// accepting any high bit would let the raw validator comparison diverge from +// consensus. In particular, bit 31 makes OP_CHECKSEQUENCEVERIFY a NOP, while +// reserved bits are masked away during script execution. +func validateCSVLock(lock uint32) error { + const maxBlockDelay = uint32(wire.SequenceLockTimeMask) + + if lock == 0 || lock > maxBlockDelay { + return fmt.Errorf("csv: lock must be a canonical block delay "+ + "in range [1, %d], got 0x%08x", maxBlockDelay, lock) + } + + return nil +} + // nodeSealed implements the Node interface. func (c *CSV) nodeSealed() {} diff --git a/lib/arkscript/node_test.go b/lib/arkscript/node_test.go index bd49790b9..df1ca63d3 100644 --- a/lib/arkscript/node_test.go +++ b/lib/arkscript/node_test.go @@ -7,6 +7,7 @@ import ( "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcec/v2/schnorr" "github.com/btcsuite/btcd/txscript/v2" + "github.com/btcsuite/btcd/wire/v2" "github.com/lightninglabs/wavelength/internal/testutils" "github.com/stretchr/testify/require" ) @@ -124,6 +125,42 @@ func TestCSVNilInner(t *testing.T) { require.Contains(t, err.Error(), "inner node is nil") } +// TestCSVRejectsNonCanonicalLock verifies that CSV compilation accepts only +// canonical, non-zero block-mode BIP-68 values. Every bit above the consensus +// value mask either changes units, disables CSV, or is ignored by consensus. +func TestCSVRejectsNonCanonicalLock(t *testing.T) { + t.Parallel() + + key, _ := testutils.CreateKey(1) + compile := func(lock uint32) error { + node := &CSV{ + Lock: lock, + Inner: &Multisig{ + Keys: []*btcec.PublicKey{ + key, + }, + }, + } + + _, err := node.Script() + + return err + } + + require.ErrorContains(t, compile(0), "canonical block delay") + + for bit := 16; bit < 32; bit++ { + lock := uint32(144) | uint32(1)<