Remediate security audit findings with regression tests (#8)

Addresses the valid findings from the gnark-APK-Proofs security audit
(Dr. Cyprian Sakwa, 2026-04-22) across all layers, with tests.

Circuit / Go FFI:
- Enforce on-curve + prime-order subgroup checks on public keys at the
  FFI trust boundary (new apk.ParseG1; findings 1, 3, 14). All-zero is
  treated as the identity padding point.
- Validate participation indices (range + uniqueness) before deriving
  the bitlist, and require exact witness length (findings 5, 10, 12, 13).
  This is fail-loud input hygiene; the key set is committed and the
  bitlist/ExpectedApk are public, so it is not a soundness control.
- Document the 5-limb bitlist encoding and the validation trust boundary
  (findings 2; corrected misleading apk.go comments for 1, 3).

Rust prover:
- Validate every public key on-curve + in subgroup, reject duplicate
  indices (findings 15, 16).
- Check FFI output buffers for null/zero length before dereferencing
  raw pointers (finding 18).
- parse_public_inputs returns a structured error instead of panicking
  (findings 19, 20).

Rust PLONK verifier:
- Subgroup checks on all deserialized G1/G2 points, validate decompressed
  points (findings 25, 26).
- Validate proof structure lengths, domain-size power-of-two, and
  Lagrange index bounds (findings 27, 30, 34).
- Simplify batch-random field reduction to remove a potential panic
  (finding 31); remove the misleadingly-named endianness trait
  (finding 28); add RFC 9380 references (findings 32, 33).

Integration test:
- Replace cross-version mem::transmute with canonical-serialization
  conversion between arkworks 0.4 (w3f-bls) and 0.5 (finding 35).

Build system:
- Reproducible Go build (-trimpath, -buildvcs=false), cfg-gated system
  linking, and go.mod/go.sum rebuild tracking (findings 43, 46, 47).

Solidity:
- Document the EIP-2537 trust boundary and stateless/replay model
  (findings 50, 53).

Regression tests: 9 Go (apk), 10 Rust prover, 8 Rust verifier.

Author: seunlanlege

circuits/apk/apk.go

@@ -68,7 +68,17 @@ type ApkProofCircuit struct {
 
 // Define defines the circuit constraints
 func (circuit *ApkProofCircuit) Define(api frontend.API) error {
-	// Decompose bitlist into individual bits
+	// Decompose the bitlist into 1024 individual participation bits.
+	//
+	// Bitlist encoding (audit finding 2): the 1024-bit participation set is packed
+	// into 5 field-element limbs, little-endian within each limb:
+	//   - Bitlist[0..3]: 250 bits each  -> indices 0..999
+	//   - Bitlist[4]:    24 bits        -> indices 1000..1023
+	// Bit i of limb k maps to validator index (k*250 + i) for k<4, and (1000 + i)
+	// for k==4. api.ToBinary(x, n) constrains x < 2^n and enforces the canonical
+	// bit decomposition, so out-of-range limb values are rejected in-circuit.
+	// The Go/Rust witness builders MUST use this exact mapping
+	// (see apk.CreateBitlistFromIndices); it is the single canonical source.
 	var bits []frontend.Variable
 	for i := range len(circuit.Bitlist) {
 		if i == 4 {
@@ -95,9 +105,15 @@ func (circuit *ApkProofCircuit) Define(api frontend.API) error {
 	apk := &seed
 
 	// Hash public keys and aggregate in a single pass.
-	// No in-circuit on-curve or subgroup checks are needed: these are performed
-	// at validator registration time (PoP assumption), and the Poseidon2
-	// commitment binds the prover to exactly those validated keys.
+	//
+	// On-curve and prime-order subgroup validity of every public key are enforced
+	// outside the proving system, at the FFI trust boundary in apk.ParseG1 (audit
+	// findings 1, 3, 14), and independently in the Rust prover before serialization
+	// (finding 15). In-circuit subgroup checks over 1024 emulated BLS12-381 points
+	// are prohibitively expensive and are intentionally not performed here; the
+	// Poseidon2 commitment then binds the prover to exactly the validated key set.
+	// Proof of Possession at registration covers secret-key ownership only — it is
+	// a separate guarantee from the algebraic point validation done in ParseG1.
 	for i := range 1024 {
 		hasher.Write(circuit.PublicKeys[i].X.Limbs...)
 		hasher.Write(circuit.PublicKeys[i].Y.Limbs...)

circuits/apk/validation.go

@@ -0,0 +1,101 @@
+// Copyright 2026 Polytope Labs.
+// SPDX-License-Identifier: Apache-2.0
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package apk
+
+import (
+	"fmt"
+
+	bls12381 "github.com/consensys/gnark-crypto/ecc/bls12-381"
+	"github.com/consensys/gnark-crypto/ecc/bls12-381/fp"
+)
+
+// NumValidators is the fixed validator-set size of the APK circuit.
+const NumValidators = 1024
+
+// G1UncompressedSize is the byte length of a G1 point in the FFI/Solidity wire
+// format: X (48 bytes big-endian) || Y (48 bytes big-endian).
+const G1UncompressedSize = 96
+
+// ParseG1 deserializes a single G1 point from the 96-byte FFI wire format
+// (X || Y, big-endian) and validates it at the trust boundary.
+//
+// This is the first point at which untrusted public keys enter the system, so
+// it is where cryptographic validity is enforced (audit findings 1, 3, 14):
+//
+//   - The all-zero encoding is the canonical point at infinity, used to pad the
+//     validator set to NumValidators. It is accepted without curve checks,
+//     matching the Rust prover's identity encoding (g1_to_gnark_bytes).
+//   - Any other point must satisfy the curve equation y² = x³ + 4 and lie in the
+//     prime-order subgroup. Proof of Possession at registration proves ownership
+//     of the secret key but does NOT imply on-curve or subgroup membership —
+//     those are independent algebraic checks enforced here.
+func ParseG1(data []byte) (bls12381.G1Affine, error) {
+	var pt bls12381.G1Affine
+	if len(data) != G1UncompressedSize {
+		return pt, fmt.Errorf("G1 point must be %d bytes, got %d", G1UncompressedSize, len(data))
+	}
+
+	// All-zero bytes denote the point at infinity (the zero value of G1Affine).
+	allZero := true
+	for _, b := range data {
+		if b != 0 {
+			allZero = false
+			break
+		}
+	}
+	if allZero {
+		return pt, nil
+	}
+
+	var x, y fp.Element
+	x.SetBytes(data[0:48])
+	y.SetBytes(data[48:96])
+	pt.X = x
+	pt.Y = y
+
+	if !pt.IsOnCurve() {
+		return pt, fmt.Errorf("G1 point not on curve")
+	}
+	if !pt.IsInSubGroup() {
+		return pt, fmt.Errorf("G1 point not in prime-order subgroup")
+	}
+	return pt, nil
+}
+
+// ValidateParticipationIndices checks that every participation index is within
+// [0, numKeys) and that there are no duplicates (audit findings 5, 12, 16).
+//
+// This is fail-loud input validation, not a soundness control. The circuit binds
+// the key set via PublicKeysCommitment and exposes Bitlist/ExpectedApk as public
+// inputs, so a malformed index list cannot make a verifier accept a wrong APK.
+// And the bitlist and the aggregation set are derived from the same indices with
+// the same range filter (and both are idempotent), so they cannot diverge or
+// double-count. What this prevents is the prover *silently* building a proof for
+// a different participation set than the caller intended: the previous helpers
+// dropped out-of-range indices and collapsed duplicates without complaint.
+func ValidateParticipationIndices(indices []int, numKeys int) error {
+	seen := make(map[int]bool, len(indices))
+	for _, idx := range indices {
+		if idx < 0 || idx >= numKeys {
+			return fmt.Errorf("participant index %d out of range [0, %d)", idx, numKeys)
+		}
+		if seen[idx] {
+			return fmt.Errorf("duplicate participant index %d", idx)
+		}
+		seen[idx] = true
+	}
+	return nil
+}

circuits/apk/validation_test.go

@@ -0,0 +1,177 @@
+// Copyright 2026 Polytope Labs.
+// SPDX-License-Identifier: Apache-2.0
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package apk
+
+import (
+	"strings"
+	"testing"
+
+	bls12381 "github.com/consensys/gnark-crypto/ecc/bls12-381"
+	"github.com/consensys/gnark-crypto/ecc/bls12-381/fp"
+)
+
+// encodeG1 serializes a point into the 96-byte FFI wire format (X || Y, big-endian).
+func encodeG1(pt bls12381.G1Affine) []byte {
+	xb := pt.X.Bytes()
+	yb := pt.Y.Bytes()
+	out := make([]byte, G1UncompressedSize)
+	copy(out[0:48], xb[:])
+	copy(out[48:96], yb[:])
+	return out
+}
+
+// onCurveNotInSubgroup deterministically finds a point that satisfies the curve
+// equation but lies outside the prime-order subgroup (BLS12-381 G1 has a large
+// cofactor, so almost every on-curve point qualifies).
+func onCurveNotInSubgroup(t *testing.T) bls12381.G1Affine {
+	t.Helper()
+	var x, one, four fp.Element
+	x.SetUint64(2)
+	one.SetUint64(1)
+	four.SetUint64(4)
+	for i := 0; i < 1000; i++ {
+		var x3, rhs, y fp.Element
+		x3.Square(&x).Mul(&x3, &x) // x³
+		rhs.Add(&x3, &four)        // x³ + 4
+		if y.Sqrt(&rhs) != nil {
+			var pt bls12381.G1Affine
+			pt.X = x
+			pt.Y = y
+			if pt.IsOnCurve() && !pt.IsInSubGroup() {
+				return pt
+			}
+		}
+		x.Add(&x, &one)
+	}
+	t.Fatal("failed to construct an on-curve, non-subgroup point")
+	return bls12381.G1Affine{}
+}
+
+// TestParseG1_ValidGenerator confirms a canonical subgroup point round-trips.
+func TestParseG1_ValidGenerator(t *testing.T) {
+	_, _, g1, _ := bls12381.Generators()
+	pt, err := ParseG1(encodeG1(g1))
+	if err != nil {
+		t.Fatalf("generator rejected: %v", err)
+	}
+	if !pt.Equal(&g1) {
+		t.Fatal("parsed point does not equal generator")
+	}
+}
+
+// TestParseG1_Identity confirms the all-zero padding encoding is accepted as
+// the point at infinity (audit finding 24 / identity encoding).
+func TestParseG1_Identity(t *testing.T) {
+	pt, err := ParseG1(make([]byte, G1UncompressedSize))
+	if err != nil {
+		t.Fatalf("identity rejected: %v", err)
+	}
+	if !pt.IsInfinity() {
+		t.Fatal("all-zero encoding did not decode to point at infinity")
+	}
+}
+
+// TestParseG1_OffCurve rejects a point that fails the curve equation (finding 14).
+func TestParseG1_OffCurve(t *testing.T) {
+	_, _, g1, _ := bls12381.Generators()
+	bad := g1
+	var one fp.Element
+	one.SetUint64(1)
+	bad.Y.Add(&bad.Y, &one) // perturb Y so y² ≠ x³ + 4
+	_, err := ParseG1(encodeG1(bad))
+	if err == nil || !strings.Contains(err.Error(), "not on curve") {
+		t.Fatalf("expected on-curve rejection, got %v", err)
+	}
+}
+
+// TestParseG1_NotInSubgroup rejects an on-curve point outside the prime-order
+// subgroup (audit findings 1, 3 enforced at the FFI boundary).
+func TestParseG1_NotInSubgroup(t *testing.T) {
+	pt := onCurveNotInSubgroup(t)
+	if !pt.IsOnCurve() {
+		t.Fatal("test point should be on curve")
+	}
+	_, err := ParseG1(encodeG1(pt))
+	if err == nil || !strings.Contains(err.Error(), "subgroup") {
+		t.Fatalf("expected subgroup rejection, got %v", err)
+	}
+}
+
+// TestParseG1_WrongLength rejects malformed inputs.
+func TestParseG1_WrongLength(t *testing.T) {
+	for _, n := range []int{0, 48, 95, 97, 192} {
+		if _, err := ParseG1(make([]byte, n)); err == nil {
+			t.Fatalf("expected error for %d-byte input", n)
+		}
+	}
+}
+
+func TestValidateParticipationIndices_Valid(t *testing.T) {
+	if err := ValidateParticipationIndices([]int{0, 1, 250, 999, 1000, 1023}, NumValidators); err != nil {
+		t.Fatalf("valid indices rejected: %v", err)
+	}
+	if err := ValidateParticipationIndices(nil, NumValidators); err != nil {
+		t.Fatalf("empty indices rejected: %v", err)
+	}
+}
+
+func TestValidateParticipationIndices_OutOfRange(t *testing.T) {
+	for _, idx := range []int{-1, NumValidators, NumValidators + 5} {
+		err := ValidateParticipationIndices([]int{0, idx}, NumValidators)
+		if err == nil || !strings.Contains(err.Error(), "out of range") {
+			t.Fatalf("index %d: expected out-of-range error, got %v", idx, err)
+		}
+	}
+}
+
+func TestValidateParticipationIndices_Duplicate(t *testing.T) {
+	err := ValidateParticipationIndices([]int{3, 7, 3}, NumValidators)
+	if err == nil || !strings.Contains(err.Error(), "duplicate") {
+		t.Fatalf("expected duplicate error, got %v", err)
+	}
+}
+
+// TestBitlistMapping_EdgeIndices locks the index→bit mapping at limb boundaries
+// (audit finding 6: missing edge-case coverage) by round-tripping through decode.
+func TestBitlistMapping_EdgeIndices(t *testing.T) {
+	cases := [][]int{
+		{0},         // first bit
+		{249},       // last bit of limb 0
+		{250},       // first bit of limb 1
+		{999},       // last bit of limb 3
+		{1000},      // first bit of limb 4
+		{1023},      // last valid bit
+		{0, 1023},   // both extremes
+		{249, 250},  // limb 0/1 boundary
+		{999, 1000}, // limb 3/4 boundary
+	}
+	for _, want := range cases {
+		bitlist := CreateBitlistFromIndices(want)
+		got := DecodeBitlist(bitlist)
+		if len(got) != len(want) {
+			t.Fatalf("indices %v: decoded %v", want, got)
+		}
+		set := make(map[int]bool)
+		for _, i := range got {
+			set[i] = true
+		}
+		for _, i := range want {
+			if !set[i] {
+				t.Fatalf("indices %v: missing %d in decoded %v", want, i, got)
+			}
+		}
+	}
+}