zkvm: refuse a block or vertex that spends one shielded note twice

verifyTransaction checks each nullifier against nullifierDB, which only holds nullifiers spent in ACCEPTED state; a block's own nullifiers are marked spent in Accept, after Verify. So two transactions in one block sharing a nullifier both passed — neither was spent yet — and one shielded note funded two spends, each creating outputs. Supply inflation, which is the exact event the nullifier set exists to prevent. A single transaction listing the same nullifier twice got through the same way: the mempool consults its conflict map before inserting the transaction's own nullifiers, so [n, n] passes admission too. Proven: Vertex.Verify on a vertex whose two txs share a nullifier returned nil. BuildVertex already refuses to batch conflicting txs (usedNullifiers), so the invariant was intended — it just was not enforced on anything that arrived over the wire, and BuildBlock never had it at all. Block.Verify and Vertex.Verify now require every nullifier in the block to be distinct, checked before proof verification because it is the cheaper gate. Also bounds the vertex decoder's counts by the bytes actually present. parentCount and txCount are attacker-controlled uint32 read straight into make(); the length checks came after the allocation. A 16-byte vertex claiming 2^32-1 parents requested 128 GiB, and a 20-byte one claiming 2^24 txs measurably allocated 671 MB before returning its error — remote unrecoverable out-of-memory from a payload smaller than a UDP header. Tests: TestBlockVerifyRejectsDuplicateNullifier, TestBlockVerifyRejectsNullifierRepeatedInOneTx, TestVertexVerifyRejectsDuplicateNullifier, TestDeserializeVertexBoundsParentCount, TestDeserializeVertexBoundsTxCount. Co-authored-by: Hanzo Dev <[email protected]>
2026-09-24 00:26:00 +00:00 · 2026-07-29 16:48:10 -07:00
parent df5ccabc99
commit f24ec103d2
3 changed files with 315 additions and 9 deletions
@@ -112,6 +112,21 @@ func (b *Block) Verify(ctx context.Context) error {
 		return errFutureBlock
 	}

+	// Block-level shape: every nullifier in the block must be distinct.
+	// verifyTransaction below only sees nullifiers already spent in ACCEPTED
+	// state, so without this two txs in one block — or one tx listing a nullifier
+	// twice — spend the same shielded note and inflate supply. Checked before the
+	// proofs because it is the cheaper gate.
+	spentHere := make(map[string]struct{}, len(b.Txs))
+	for _, tx := range b.Txs {
+		for _, nullifier := range tx.Nullifiers {
+			if _, dup := spentHere[string(nullifier)]; dup {
+				return errDuplicateNullifier
+			}
+			spentHere[string(nullifier)] = struct{}{}
+		}
+	}
+
 	// Verify each transaction
 	for _, tx := range b.Txs {
 		if err := tx.ValidateBasic(); err != nil {
@@ -153,12 +168,7 @@ func (b *Block) Verify(ctx context.Context) error {
 	}

 	// Verify state root
-	expectedRoot, err := b.vm.computeStateRoot(b.Txs)
-	if err != nil {
-		return err
-	}
-
-	if !bytes.Equal(b.StateRoot, expectedRoot) {
+	if !bytes.Equal(b.StateRoot, b.vm.computeStateRoot(b.Txs)) {
 		return errInvalidStateRoot
 	}

@@ -340,4 +350,8 @@ var (
 	errInvalidHeight    = errors.New("invalid block height")
 	errInvalidTimestamp = errors.New("invalid block timestamp")
 	errInvalidStateRoot = errors.New("invalid state root")
+
+	// errDuplicateNullifier — the same nullifier appears twice inside one block or
+	// vertex, i.e. one shielded note spent twice. Fail closed.
+	errDuplicateNullifier = errors.New("nullifier spent twice in one block")
 )
@@ -0,0 +1,272 @@
+// Copyright (C) 2019-2026, Lux Industries Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package zkvm
+
+import (
+	"context"
+	"crypto/sha256"
+	"encoding/binary"
+	"errors"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/luxfi/ids"
+)
+
+// spendTx builds a shielded transfer that spends the given nullifiers.
+func spendTx(nullifiers ...[]byte) *Transaction {
+	tx := &Transaction{
+		Type:       TransactionTypeTransfer,
+		Version:    1,
+		Nullifiers: nullifiers,
+		Outputs: []*ShieldedOutput{
+			{
+				Commitment:      make([]byte, 32),
+				EncryptedNote:   make([]byte, 32),
+				EphemeralPubKey: make([]byte, 32),
+				OutputProof:     make([]byte, 32),
+			},
+		},
+		Proof: &ZKProof{ProofType: "groth16", ProofData: make([]byte, 32)},
+		Fee:   1,
+	}
+	tx.ID = tx.ComputeID()
+	return tx
+}
+
+func nullifier(b byte) []byte {
+	n := make([]byte, 32)
+	n[0] = b
+	return n
+}
+
+// acceptProofs makes each tx's proof verify, so a Verify failure can only come
+// from a block-level rule. Without it the test VM's dummy verifying keys reject
+// every tx first and the block-level gate is never reached — which is exactly the
+// confound these tests must not hide behind.
+func acceptProofs(vmImpl *VM, txs ...*Transaction) {
+	pv := vmImpl.proofVerifier
+	pv.dummyKeys = false
+	for _, tx := range txs {
+		pv.proofCache.Add(string(pv.hashProof(tx)), true)
+	}
+}
+
+// TestComputeStateRootIsPure pins the state root as a pure function of the
+// committed root plus the block's txs. Computing it must not mutate the tree,
+// because Block.Verify computes it: a second Verify of the same block, or a
+// Verify of the block that wins a poll after a loser was already verified, would
+// otherwise see a different root and reject a valid block.
+func TestComputeStateRootIsPure(t *testing.T) {
+	require := require.New(t)
+	vmImpl := setupTestVM(t)
+	defer vmImpl.Shutdown(context.Background())
+
+	txs := []*Transaction{spendTx(nullifier(1)), spendTx(nullifier(2))}
+
+	first := vmImpl.computeStateRoot(txs)
+	second := vmImpl.computeStateRoot(txs)
+	require.Equal(first, second, "computing the root twice must give the same root")
+
+	// Computing the root for a competing block must not disturb the root of the
+	// block that actually wins.
+	loser := []*Transaction{spendTx(nullifier(9))}
+	vmImpl.computeStateRoot(loser)
+	require.Equal(first, vmImpl.computeStateRoot(txs),
+		"a rejected block's root computation must not poison later ones")
+}
+
+// TestStateRootHasOneDefinition pins the consensus root to a single hash
+// function: SHA-256 over currentRoot ‖ commitments ‖ nullifiers. A second,
+// hardware-conditional digest (a GPU Poseidon path with a SHA-256 fallback)
+// would make the committed root depend on whether the validator has an
+// accelerator, and nodes with and without one would reject each other's blocks.
+func TestStateRootHasOneDefinition(t *testing.T) {
+	require := require.New(t)
+	vmImpl := setupTestVM(t)
+	defer vmImpl.Shutdown(context.Background())
+
+	txs := []*Transaction{spendTx(nullifier(1), nullifier(2)), spendTx(nullifier(3))}
+
+	h := sha256.New()
+	h.Write(vmImpl.stateTree.GetRoot())
+	for _, tx := range txs {
+		for _, out := range tx.Outputs {
+			h.Write(out.Commitment)
+		}
+	}
+	for _, tx := range txs {
+		for _, n := range tx.Nullifiers {
+			h.Write(n)
+		}
+	}
+
+	require.Equal(h.Sum(nil), vmImpl.computeStateRoot(txs))
+}
+
+// TestStateRootAdvancesOnlyOnFinalize proves the root moves only when a block is
+// accepted, and that after it moves the next block's root builds on the new one.
+func TestStateRootAdvancesOnlyOnFinalize(t *testing.T) {
+	require := require.New(t)
+	vmImpl := setupTestVM(t)
+	defer vmImpl.Shutdown(context.Background())
+
+	before := append([]byte(nil), vmImpl.stateTree.GetRoot()...)
+	txs := []*Transaction{spendTx(nullifier(4))}
+
+	root := vmImpl.computeStateRoot(txs)
+	require.Equal(before, vmImpl.stateTree.GetRoot(), "computing a root must not advance it")
+
+	require.NoError(vmImpl.stateTree.Finalize(root))
+	require.Equal(root, vmImpl.stateTree.GetRoot())
+	require.NotEqual(root, vmImpl.computeStateRoot(txs),
+		"the next block folds the same txs onto the new committed root")
+}
+
+// TestBlockVerifyRejectsDuplicateNullifier proves a block cannot spend the same
+// shielded note twice. Neither nullifier is spent in accepted state, so the
+// per-tx nullifier check cannot see the conflict — only the block-level check
+// can, and without it the note is double-spent and supply inflates.
+func TestBlockVerifyRejectsDuplicateNullifier(t *testing.T) {
+	require := require.New(t)
+	vmImpl := setupTestVM(t)
+	defer vmImpl.Shutdown(context.Background())
+
+	reused := nullifier(7)
+	require.False(vmImpl.nullifierDB.IsNullifierSpent(reused), "precondition: not yet spent")
+
+	first, second := spendTx(reused), spendTx(reused)
+	second.Fee = 2 // distinct tx, same spent note
+	second.ID = second.ComputeID()
+	acceptProofs(vmImpl, first, second)
+
+	blk := &Block{
+		ParentID_:      vmImpl.lastAcceptedID,
+		BlockHeight:    1,
+		BlockTimestamp: 1607144400,
+		Txs:            []*Transaction{first, second},
+		vm:             vmImpl,
+	}
+	blk.StateRoot = vmImpl.computeStateRoot(blk.Txs)
+
+	// Sanity: each tx on its own is a block this VM accepts, so the rejection
+	// below is caused by the pair sharing a nullifier and nothing else.
+	solo := &Block{
+		ParentID_:      vmImpl.lastAcceptedID,
+		BlockHeight:    1,
+		BlockTimestamp: 1607144400,
+		Txs:            []*Transaction{first},
+		vm:             vmImpl,
+	}
+	solo.StateRoot = vmImpl.computeStateRoot(solo.Txs)
+	require.NoError(solo.Verify(context.Background()))
+
+	require.ErrorIs(blk.Verify(context.Background()), errDuplicateNullifier)
+}
+
+// TestBlockVerifyRejectsNullifierRepeatedInOneTx covers the same defect within a
+// single transaction: the mempool's conflict map is consulted before the tx's own
+// nullifiers are inserted, so [n, n] passes admission.
+func TestBlockVerifyRejectsNullifierRepeatedInOneTx(t *testing.T) {
+	require := require.New(t)
+	vmImpl := setupTestVM(t)
+	defer vmImpl.Shutdown(context.Background())
+
+	reused := nullifier(8)
+	tx := spendTx(reused, reused)
+	require.NoError(vmImpl.mempool.AddTransaction(tx), "admission does not catch it")
+	acceptProofs(vmImpl, tx)
+
+	blk := &Block{
+		ParentID_:      vmImpl.lastAcceptedID,
+		BlockHeight:    1,
+		BlockTimestamp: 1607144400,
+		Txs:            []*Transaction{tx},
+		vm:             vmImpl,
+	}
+	blk.StateRoot = vmImpl.computeStateRoot(blk.Txs)
+
+	require.ErrorIs(blk.Verify(context.Background()), errDuplicateNullifier)
+}
+
+// TestVertexVerifyRejectsDuplicateNullifier is the DAG half of the same gate.
+// BuildVertex refuses to batch conflicting txs; a vertex off the wire must be
+// held to the same rule.
+func TestVertexVerifyRejectsDuplicateNullifier(t *testing.T) {
+	require := require.New(t)
+	vmImpl := setupTestVM(t)
+	defer vmImpl.Shutdown(context.Background())
+
+	reused := nullifier(6)
+	first, second := spendTx(reused), spendTx(reused)
+	second.Fee = 2
+	second.ID = second.ComputeID()
+	acceptProofs(vmImpl, first, second)
+
+	solo := &Vertex{height: 1, parents: []ids.ID{vmImpl.lastAcceptedID}, txs: []*Transaction{first}, vm: vmImpl}
+	require.NoError(solo.Verify(context.Background()))
+
+	v := &Vertex{
+		height:  1,
+		parents: []ids.ID{vmImpl.lastAcceptedID},
+		txs:     []*Transaction{first, second},
+		vm:      vmImpl,
+	}
+
+	require.ErrorIs(v.Verify(context.Background()), errDuplicateNullifier)
+}
+
+// TestDeserializeVertexBoundsParentCount proves the vertex decoder sizes its
+// slices from the bytes actually present. A 16-byte vertex claiming 2^32-1
+// parents asked for 128 GiB before this bound existed, which is an
+// unrecoverable out-of-memory rather than a rejected message — a remote kill
+// with a 16-byte payload.
+func TestDeserializeVertexBoundsParentCount(t *testing.T) {
+	require := require.New(t)
+
+	data := make([]byte, 16)
+	binary.BigEndian.PutUint64(data[0:], 1)      // height
+	binary.BigEndian.PutUint32(data[8:], 0)      // epoch
+	binary.BigEndian.PutUint32(data[12:], 1<<22) // parentCount: 4.2M IDs = 134 MiB
+	require.Equal(0, (len(data)-16)/32, "no parent bytes are present at all")
+
+	var before, after runtime.MemStats
+	runtime.GC()
+	runtime.ReadMemStats(&before)
+
+	v, err := deserializeVertex(data, nil)
+
+	runtime.ReadMemStats(&after)
+
+	require.Nil(v)
+	require.ErrorIs(err, errInvalidBlock)
+	require.Less(after.TotalAlloc-before.TotalAlloc, uint64(4<<20),
+		"rejecting the header must not allocate the claimed parents")
+}
+
+// TestDeserializeVertexBoundsTxCount is the same bound for the tx count.
+func TestDeserializeVertexBoundsTxCount(t *testing.T) {
+	require := require.New(t)
+
+	data := make([]byte, 20)
+	binary.BigEndian.PutUint64(data[0:], 1)      // height
+	binary.BigEndian.PutUint32(data[8:], 0)      // epoch
+	binary.BigEndian.PutUint32(data[12:], 0)     // parentCount
+	binary.BigEndian.PutUint32(data[16:], 1<<24) // txCount: 16.7M pointers = 134 MiB
+
+	var before, after runtime.MemStats
+	runtime.GC()
+	runtime.ReadMemStats(&before)
+
+	v, err := deserializeVertex(data, nil)
+
+	runtime.ReadMemStats(&after)
+
+	require.Nil(v)
+	require.True(errors.Is(err, errInvalidBlock))
+	require.Less(after.TotalAlloc-before.TotalAlloc, uint64(4<<20),
+		"rejecting the header must not allocate the claimed txs")
+}
@@ -40,6 +40,19 @@ func (v *Vertex) Txs() []ids.ID        { return v.txIDs }
 func (v *Vertex) Status() choices.Status { return v.status }

 func (v *Vertex) Verify(ctx context.Context) error {
+	// Every nullifier in the vertex must be distinct. verifyTransaction only sees
+	// accepted state; BuildVertex already refuses to batch conflicting txs, and a
+	// vertex that arrived on the wire is held to the same rule or one shielded
+	// note is spent twice.
+	spentHere := make(map[string]struct{}, len(v.txs))
+	for _, tx := range v.txs {
+		for _, nullifier := range tx.Nullifiers {
+			if _, dup := spentHere[string(nullifier)]; dup {
+				return errDuplicateNullifier
+			}
+			spentHere[string(nullifier)] = struct{}{}
+		}
+	}
 	for _, tx := range v.txs {
 		if err := tx.ValidateBasic(); err != nil {
 			return err
@@ -265,14 +278,17 @@ func deserializeVertex(data []byte, vm *VM) (*Vertex, error) {
 	epoch := binary.BigEndian.Uint32(data[pos:])
 	pos += 4

+	// Counts are attacker-controlled: bound each by the bytes that remain before
+	// allocating, or a 16-byte vertex claiming 2^32-1 parents asks for 128 GiB and
+	// the node dies on an unrecoverable out-of-memory.
 	parentCount := binary.BigEndian.Uint32(data[pos:])
 	pos += 4
+	if int(parentCount) > (len(data)-pos)/32 {
+		return nil, errInvalidBlock
+	}

 	parents := make([]ids.ID, parentCount)
 	for i := uint32(0); i < parentCount; i++ {
-		if pos+32 > len(data) {
-			return nil, errInvalidBlock
-		}
 		copy(parents[i][:], data[pos:pos+32])
 		pos += 32
 	}
@@ -282,6 +298,10 @@ func deserializeVertex(data []byte, vm *VM) (*Vertex, error) {
 	}
 	txCount := binary.BigEndian.Uint32(data[pos:])
 	pos += 4
+	// Every tx costs at least its 4-byte length prefix.
+	if int(txCount) > (len(data)-pos)/4 {
+		return nil, errInvalidBlock
+	}

 	txs := make([]*Transaction, 0, txCount)
 	txIDs := make([]ids.ID, 0, txCount)