Security review for v1.0

Executive summary

This is the formal security review of secp256k1-native conducted ahead of the v1.0 release. The review fired the full toolkit of an opportunistic attacker — static analysis of all C and Ruby, AddressSanitizer/UndefinedBehaviorSanitizer, differential fuzzing against an independently written reference, deterministic constant-time verification by secret-poisoning (ctgrind/valgrind) corroborated by dudect, and four static-analysis engines — across tens of millions of cases. The core cryptographic machinery is sound: the Montgomery ladder (the documented constant-time primitive behind Point#mul) is correct, memory-safe, and constant-time under deterministic poisoning; the field and point arithmetic is correct on all in-contract inputs; and no memory-safety defect or key-leak path was found anywhere. The review confirmed 20 findings: 1 high, 1 medium, 6 low, and 12 informational. The single most serious issue is a dropped-carry bug in scalar_reduce_limbs (scalar.c:201) that makes Secp256k1Native.scalar_mul return a silently-wrong, in-range result for attacker-influenceable non-canonical operands. Every confirmed correctness defect is in standalone, exposed-but-not-internally-reached scalar/field primitives and is fixable with small, localised changes; none is reachable through the gem’s own Point/SEC1 API. The library is release-ready for v1.0 once the high- and medium-severity scalar defects are fixed, subject to the standing caveat that this review is not a substitute for a professional cryptographic audit.

1. Scope and threat model

The brief for this review was deliberately narrow and adversarial: find everything an opportunistic attacker armed with the same commodity tooling we have — fuzzing, sanitizers, differential testing, and timing analysis — would find first. A finding only counts if it names a specific function and line, the exact triggering input, the observable wrong behaviour, and a plausible exploitation path.

In scope:

The C extension in full: field.c, scalar.c, jacobian.c, secp256k1_native.c, and the shared header.
The pure-Ruby reference and public API: lib/secp256k1.rb and lib/secp256k1/version.rb.
The Ruby↔C marshalling boundary (rb_to_uint256, uint256_to_rb, rb_fred).
Functional correctness, memory safety, input validation, the fail-closed contract, and constant-time behaviour of the documented constant-time primitive.

Out of scope, and explicitly so:

This is not a professional cryptographic audit. It is a structured adversarial review by the same class of tooling an opportunistic attacker would deploy. It establishes empirical lower bounds on robustness; it does not establish their absence of all defects. A formal audit by a specialist cryptographic firm — with mathematical review of the reduction algorithms, a wider side-channel model, and microarchitectural analysis — remains a prerequisite before this library secures material value. See risks.md for the broader evidence base on why custom cryptographic arithmetic warrants this caution.
Higher-level constructions (ECDSA, Schnorr, key derivation, hashing) are deliberately absent from the library (principle 3, minimal attack surface) and therefore from this review. Their absence materially reduces the reachability of several findings below.
Constant-time guarantees in the interpreted Ruby path are not claimable with existing tooling and are not claimed (see risks.md, “Side-channel attacks are practical, but verification is confined to compiled code”). The CT analysis here is confined to the compiled C extension.

The realistic adversary modelled is therefore an attacker who can (a) feed inputs to the public API or to a downstream consumer that calls the primitives directly, and (b) iterate with fuzzers, sanitizers, and timing harnesses. The review does not assume an attacker with physical access, microarchitectural co-residency beyond what dudect/ctgrind model, or the ability to read process memory.

2. Methodology and coverage

Every claim below was independently reproduced rather than inherited from a prior pass. The weapons fired, with concrete coverage:

Static review (all C + Ruby). Full line-by-line read of all five C translation units plus the header, and of lib/secp256k1.rb in full (674 lines; confirmed lib/secp256k1/ holds only version.rb). The marshalling boundary was traced by hand to confirm rb_integer_pack’s overflow return is checked before any limb is consumed; the two rb_integer_pack call sites are the only Ruby→C entry points.

ASan + UBSan. A standalone harness built per the project’s certified compile line plus -g -fsanitize=address,undefined -fno-omit-frame-pointer, exercising all 16 internal ops over structured edge cases and millions of random inputs under UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1. Field/scalar mode ran clean through ~7,000,000 iterations (~49M+ field calls, ~14M Fermat inversions); Jacobian/ladder mode completed 400,000 iterations clean (≈12.8M jp_add + 12.8M jp_double from 50k ladder runs, plus 2.4M edge ops). Aliasing forms used by the ladder (jp_add(r,r,q), jp_double(r,r), jp_add(r,r,r)) were exercised explicitly. Inputs were deliberately fed unreduced (≥P, ≥N, up to 2²⁵⁶−1) to stress carry paths. The Ruby C extension was additionally rebuilt under ASan/UBSan and the full rspec suite (368 examples, incl. 474 Wycheproof ECDSA vectors) run under libasan with detect_leaks=0. Result: zero sanitizer diagnostics on every path. The marshalling boundary’s no-OOB property was separately confirmed with rb_integer_pack packing 2²⁵⁶…2⁴⁰⁹⁶ into red-zone-bracketed exact-size buffers — every over-wide case returned ret=2 with no write past numwords.

Differential fuzzing vs an independent reference. An independent secp256k1 reference was written from scratch in Python directly from the curve parameters (field mod P, scalar mod N, Jacobian/affine ops, Montgomery ladder), cross-validated against true affine double-and-add as a separate code path. The C *_internal functions were driven directly via the certified standalone build. Confirmed zero-mismatch case counts (representative, across multiple independent harnesses): fred 2.2M, scalar_reduce 1.2M+, scalar_mul 2.0M+5M, scalar_add 3M, scalar_inv 1M+, fadd/fsub/fmul/fsqr/fneg/finv ~2M each, fsqrt 1.5M (balanced QR/non-QR), jp_double 800k, jp_add 280k structured + ~1.95M bulk, jp_neg 400k, scalar_multiply_ct 100k+ vs independent affine k·G, plus exhaustive structured-corner grids (0, 1, P−1, P, P+1, 2²⁵⁶−1, N−1, N, N+1, c_N, small G multiples, infinity [0,1,0], Y=0 points). A strictly stronger geometric oracle (de-projectivise C output via independent modular inverse, compare against the true affine group law) was used for point ops to avoid a reference that merely mirrors the C special-case selection. Aggregate: tens of millions of cases, zero correctness mismatches on in-contract inputs.

ctgrind secret-poisoning + dudect. Primary (deterministic): a valgrind/memcheck harness marked secret scalars and field/point operands UNDEFINED via VALGRIND_MAKE_MEM_UNDEFINED, then ran each function under --error-exitcode=1 --track-origins=yes. The Montgomery ladder scalar_multiply_ct_internal (k fully poisoned) is clean, as are all field ops, jp_add/jp_double/jp_neg, and scalar_add. The only secret-dependent control flow found is in scalar_reduce_limbs (scalar.c:166, :193) — not on the EC scalar-multiply path. Corroboration (statistical): dudect Welch t-tests, all far below the 4.5 threshold — scalar_multiply_ct t ≈0.31–0.46, fred 0.034, fsub 0.30, fneg 0.069, fadd 1.14, jp_add 0.42 (n up to 1.5M for field ops). Assembly of the ladder loop confirms the only in-loop conditional jump is the public loop counter; the secret bit reaches only cswap’s branchless mask.

clang static analysis (four engines). clang-tidy 18 (clang-analyzer-*, bugprone-*, cert-*), clang --analyze (default + alpha.core/alpha.security/security/nullability), gcc 13 -fanalyzer, and a high-warning compile under both gcc and clang with -Wall -Wextra -Wconversion -Wsign-conversion -Wshadow -Wcast-qual -Wpointer-arith. The high-warning compile produced zero warnings on all four TUs — every narrowing/widening conversion in the limb arithmetic is explicitly cast. The only substantive finding is a single benign dead store at field.c:186. 33 style/capacity items (swappable-parameter notes on Ruby wrappers, -Wanalyzer-too-complex budget exhaustion) were triaged as noise.

3. Findings

Severities follow the threat model: a finding’s reachability through the gem’s own public API is decisive. Several confirmed arithmetic defects are rated low/info because SEC1 from_bytes and the canonical-in→canonical-out property of the field layer make them unreachable inside the library — but they remain real contract violations that a downstream consumer calling the raw primitives can hit.

High

H-1. Dropped top carry in scalar_reduce_limbs yields silently-wrong scalar_mul for non-canonical operands. Location: ext/secp256k1_native/scalar.c:201 (residual t[4] fold), surfacing via scalar_mul_internal (scalar.c:257) and rb_scalar_mul (scalar.c:358). Mechanism: After two c_N folds, a residual carry of 1 is folded back as r += carry3 * c_N. When that fold makes r equal exactly 2²⁵⁶ (i.e. the pre-fold value is N and carry3=1), line 201 (r->d[3] += (a2>>64)) discards the carry out of bit 255, leaving r=0. The single branchless conditional subtraction then sees r=0 < N and keeps 0. The correct result is c_N. The result is wrong by exactly c_N and still in [0,N), so no range check catches it. Reproduction: Secp256k1Native.scalar_mul(2²⁵⁶−1, N+2) returns 0x14551231950b75fc4402da1732fc9bebd; the correct (a*b) % N is 0x28aa24632a16ebf88805b42e65f937d7c. Directly, scalar_reduce(hi=N+1, lo=0) returns 0x0 versus the correct c_N = 0x14551231950b75fc4402da1732fc9bebf. The failing band is exactly hi == N+1 with lo ∈ [0, c_N−1] (~2¹²⁸·³⁵ inputs), density ~2⁻³⁸⁴ at random — which is why 2M+ random cases find zero failures and only structured edge inputs reveal it. Exploitation: rb_scalar_mul does not reduce operands mod N before multiplying, and the marshalling layer accepts any value in [0, 2²⁵⁶). An adversary controlling a multiplicand (e.g. a public tweak or blinding factor passed as b = N+2) can deterministically steer hi to N+1 and force a wrong, in-range, attacker-known scalar in a signing-path primitive. The C result diverges from the pure-Ruby reference (a*b) % N, violating the dual-implementation contract. Unreachable when both operands are already reduced (a,b < N ⟹ hi < N), but neither the contract nor the reference imposes a reduced-input precondition. Recommended fix: In scalar_reduce_limbs, capture the carry dropped at scalar.c:201 and fold it into the final conditional subtraction, keeping the path branchless: compute the 257-bit residual, derive topcarry, and select r−N whenever topcarry is set or r ≥ N (mask = -(uint64_t)((borrow != 0) | (topcarry != 0))). Verified in Python to correct the entire hi==N+1 band and pass 500k random inputs. Defence in depth: reduce both operands mod N inside rb_scalar_mul (mirroring rb_scalar_inv). Add a structured regression vector (scalar_mul(2²⁵⁶−1, N+2), scalar_reduce(N+1, k)) since random tests provably cannot reach this band.

H-2. Compiler-reconstructed secret-dependent branch in the Montgomery ladder (uint256_select), found on bare metal. — FIXED. Found: the v1 bare-metal dudect pass (issue #25), after the original review, on an AMD Ryzen 9 9950X / GCC 15.2.0 / -O2. Not present in the review’s measurements because the review’s toolchain compiled the same source to branchless code — this is a compiled-artefact defect, invisible to source inspection and toolchain-dependent. Location: ext/secp256k1_native/secp256k1_native.h (uint256_select), inlined into jp_add_internal (jacobian.c:269-289) inside scalar_multiply_ct_internal — i.e. the default secret-scalar path Point#mul. Mechanism: uint256_select is written branchlessly as r = (a & ~mask) | (b & mask) with mask = -(uint64_t)(flag != 0). GCC 15.2 at -O2 (the shipped optimisation level) recognises this all-0s/all-1s select idiom, reconstructs the original boolean flag, and emits je/jne to out-of-line copy blocks — a secret-dependent conditional jump. The flag here is derived from the ladder accumulators, which depend on the secret scalar, so the branch leaks. This silently undid the |t|=875 fix (I-9), which had relied on uint256_select being branchless. Reproduction: bare-metal dudect, fixed scalar vs random scalar, measured |t| ≈ 21 (stable across every run, ~560 ns separation). ctgrind/valgrind reports “Conditional jump or move depends on uninitialised value” at the uint256_select source lines via jp_add_internal → scalar_multiply_ct_internal; objdump -dl confirms je/jne attributed to those lines. (The previous toolchain emitted cmov/pure bitops here, so neither its ctgrind nor its dudect run flagged it — the regression is entirely in the GCC 15.2 code generation.) Exploitation: a timing side-channel on the secret scalar (private key / nonce) in the library’s headline constant-time operation — the most serious side-channel class for an ECC primitive. Pre-1.0 with no published RubyGems dependents, so handled as fix-publish-and-document: full GHSA + CVE record, without a coordinated downstream-notification timeline. Resolution (applied, verified on the same machine): introduce ct_value_barrier_u64() (empty volatile asm, the libsecp256k1/BoringSSL value-barrier technique) and a ct_mask_u64() helper, and route every all-0s/all-1s select mask in the extension through it (uint256_select, fred/fadd/fsub/fneg, scalar_reduce/scalar_add, the jp_double infinity select, and the ladder cswap). Only uint256_select actively branchified under GCC 15.2; the rest are hardened as defence-in-depth against future reconstruction. Post-fix: disassembly shows 0 conditional jumps/cmov at any select line; ctgrind clean (exit 0); dudect scalar_multiply_ct |t| 21 → 0.68 mean (0/20 runs over 4.5); rspec 416/0. See the advisory. The lasting control this finding adds: bare-metal dudect is now a required pre-tag release gate (a constant-time source is not a constant-time binary; CI’s deterministic CT checks run against CI’s compiler, not the user’s).

Medium

M-1. scalar_add_internal under-reduces for operands ≥ N, returning out-of-range/wrong values. Location: ext/secp256k1_native/scalar.c:273-291 (scalar_add_internal), via rb_scalar_add (scalar.c:401). Mechanism: Computes sum = a + b with a 257th overflow bit, then subtracts N at most once — correct only when a,b ∈ [0,N) (so sum < 2N). rb_scalar_add performs no input reduction, so for arbitrary 256-bit operands sum can reach ~2²⁵⁷ (≈8N) while only one subtraction is applied. Reproduction: scalar_add(N, N) returns N (0xff…0364141) instead of 0 — note the result equals N, i.e. is not even canonical, breaking downstream range checks. scalar_add(2²⁵⁶−1, 2²⁵⁶−1) returns 0x14551231950b75fc4402da1732fc9bebd instead of 0x28aa24632a16ebf88805b42e65f937d7c. Diverges from the documented (a+b) mod N and from the pure-Ruby reference. The sibling primitives scalar_mul/scalar_mod/scalar_inv all correctly accept ≥N inputs (they fully reduce), so scalar_add is the lone outlier — and a consumer who reasonably assumes parity (e.g. ECDSA-style s = scalar_add(scalar_mul(r,d), z) with a raw hash z ≥ N) gets a wrong/non-canonical s. Exploitation: scalar_add has no internal callers and is unreachable via Point#mul (the gem ships no ECDSA), so there is no in-library attack path or key-leak. Worst-case downstream harm is an invalid/non-canonical signature scalar (availability/integrity), not secret compromise — hence medium, not high. The existing suite tests only canonical inputs and never catches it. Recommended fix: Reduce each operand to [0,N) before adding (e.g. scalar_reduce(&ua, &zero, &ua) in rb_scalar_add), mirroring scalar_mod/scalar_inv; after which sum < 2N and the single conditional subtraction is correct. Add regression tests feeding operands ≥ N against (a+b) % N.

Low

L-1. Float / #to_int objects silently truncated at C field & scalar entry points. Location: field.c:27-39 (rb_to_uint256); affects all 16 C-backed wrappers. Mechanism: rb_to_uint256 marshals via rb_integer_pack, which calls rb_to_int, coercing any #to_int object and truncating Floats toward zero with no type check. Secp256k1Native.fmul(1.5, 2) ⟹ 2; fmul(Sneaky#to_int=5, 1) ⟹ 5; scalar_mul(1.9, 3) ⟹ 3; fmul(Rational(3,2), 2) ⟹ 2. Reaches the public Secp256k1.fmul too, and diverges from pure Ruby, which computes fred(1.5*2) = 3.0. Violates principle 1 (fail closed). Not memory-unsafe; negative/over-256-bit guards remain intact. Low because the inputs must be locally constructed, not attacker-controlled bytes, and no key/memory is affected. Recommended fix: Reject non-Integers at the boundary: if (!RB_INTEGER_TYPE_P(rb_int)) rb_raise(rb_eTypeError, ...). One line covers all wrappers. Guard the pure-Ruby twins with kind_of?(Integer) to eliminate the divergence.

L-2. Point#mul silently truncates non-Integer (Float/Rational) scalars. Location: lib/secp256k1.rb:554-571 (Point#mul). Mechanism: No type check on scalar; a Float satisfies zero? and %= N (staying a Float) and flows into native scalar_multiply_ct, where rb_integer_pack truncates toward zero. mul(5.9) == mul(5) (round-toward-zero, not rounding); result is a valid on-curve point for the truncated integer. The native default path is the only silent-truncation entry point — pure-Ruby scalar_multiply_ct (5.0 >> 1) and mul_vt (odd?) both raise NoMethodError, so behaviour also diverges between backends. Out-of-range Floats (−3.0, NaN, Infinity) still raise. Contradicts principles 1 and 4; requires an upstream consumer bug to produce a Float scalar, so low. Recommended fix: raise ArgumentError, "scalar must be an Integer" unless scalar.is_a?(Integer) at the top of Point#mul and Point#mul_vt, and/or harden rb_to_uint256 per L-1.

L-3. fadd/fsub/fneg do not reduce non-canonical (≥ P) inputs; fadd can return exactly P. Location: field.c fadd_internal (311-331), fsub_internal (338-354), fneg_internal (361-374); wrappers rb_fadd (517), rb_fsub (533), rb_fneg (549). Mechanism: Each assumes operands in [0,P) and does a single conditional add/sub of P; the wrappers (unlike rb_finv/rb_fsqrt, which fred first) do not reduce. fadd(P−1, P+1) returns exactly P (true value 0); fadd(2²⁵⁶−1, 2²⁵⁶−1) returns 0x1000003cf (true 0x2000007a0); fsub/fneg similarly wrong. A 2M-iteration sweep over canonical inputs produced zero non-canonical outputs, isolating the defect strictly to non-canonical inputs. Exploitation: Unreachable via the Point/scalar API — from_bytes rejects x,y ≥ P and all intermediates are built from fmul/fsqr (full 512-bit reduction) and reduced operands, so canonical-in→canonical-out keeps these functions safe. A downstream consumer feeding raw ≥P field elements gets silently corrupted (sometimes exactly-P) results. Note: the pure-Ruby fsub/fneg are also wrong on ≥P inputs (returning negative integers), so the divergence is “C wrong vs Ruby differently-wrong”; only fadd’s pure-Ruby twin is correct. Recommended fix: fred the inputs in rb_fadd/rb_fsub/rb_fneg (minimal, keeps the hot internals branch-light for CT callers that guarantee canonical operands). Fix pure-Ruby fsub/fneg to canonicalise. Add a differential spec with operands ≥ P (incl. == P and == 2²⁵⁶−1).

L-4. scalar_mod (C) rejects positive inputs ≥ 2²⁵⁶, diverging from pure Ruby and from its own negative-input path. Location: scalar.c:328-350 (rb_scalar_mod). Mechanism: Pre-reduces in Ruby only when the input is negative; a large positive value is passed raw to rb_to_uint256, which raises ArgumentError "value exceeds 256 bits" for any value ≥ 2²⁵⁶. So scalar_mod(2⁶⁰⁰) raises while scalar_mod(−(2⁶⁰⁰)) succeeds, and pure-Ruby scalar_mod reduces both fine. Fail-closed (raises, never returns a truncated scalar), so the impact is availability/backend-inconsistency, not a cryptographic compromise — a consumer using scalar_mod as a hash-to-scalar reduction on a wide positive input crashes only with the C extension loaded. The authors already work around this in spec/secp256k1_cross_property_spec.rb:111. Recommended fix: Apply the existing Ruby % pre-reduction unconditionally (a_norm = a % n_rb) so a_norm < N for both signs; remove the “stay within range” test constraint and add a cross-parity case for inputs ≥ 2²⁵⁶.

L-5. Point.new and add/mul/mul_vt/negate accept off-curve coordinates without validation. Location: lib/secp256k1.rb:440-443 (initialize), 584-595 (mul_vt), 601-612 (add), 617-621 (negate), 554-571 (mul). Mechanism: initialize stores arbitrary x,y unchecked; all arithmetic proceeds on and returns off-curve points. Because the short-Weierstrass group law is independent of b, scalar multiplication on an off-curve point stays self-consistent within the invalid curve y² = x³ + b' — the textbook precondition for invalid-curve key recovery via small-order points + CRT. Not reachable from untrusted input: the sole decode entry point from_bytes validates on_curve? and rejects off-curve input, and on_curve? is invoked nowhere else. The risk lives entirely in hypothetical downstream code that calls Point.new on attacker-controlled coordinates and then a secret-scalar mul. Recommended fix: Add a validating Point.from_coordinates(x, y) that raises unless on-curve, document it as the required entry point for caller-supplied coordinates, and document in YARD/risks.md that callers must only operate on Points from from_bytes/from_coordinates/generator. (Do not validate in initialize — it is used internally for always-on-curve intermediates.)

L-6. fsqrt_internal does not write *r on the non-residue path, contradicting its docstring. Location: field.c:407-442 (fsqrt_internal), docstring at 403-405. Mechanism: *r is written only on the QR path (uint256_copy(r, &result) at line 440, after the return 0 at line 438), so on a non-residue input the function returns 0 and leaves *r holding the caller’s stale/uninitialised buffer — while the docstring wrongly promises “The result is written to *r in both cases.” A poisoned-buffer harness confirmed the poison survives for a=3 (non-residue). Not exploitable in the shipped library: the only caller, rb_fsqrt (field.c:603), discards r when ok==0, so no uninitialised value reaches Ruby. A footgun for future C callers who trust the docstring. The arithmetic itself is correct (flag always correct across 1.5M cases). Recommended fix: Write a defined value before return 0 (e.g. uint256_copy(r, &result) or zero), honouring the contract — preferred for a crypto library — or correct the docstring to state *r is left unmodified on failure.

Informational

I-1. Marshalling boundary overflow detection is correct — no OOB write (positive assurance / refutes a memory-safety hypothesis). rb_to_uint256 (field.c:27-44) and rb_fred (field.c:455-478) pack into fixed-size stack arrays via rb_integer_pack, check result < 0 (reject negative) and result > 1 (reject overflow) before consuming any limb. Empirically verified with red-zone-bracketed exact-size buffers: every over-wide value (2²⁵⁶…2⁴⁰⁹⁶) returned ret=2 with no write past numwords and zero ASan/UBSan diagnostics. These are the only two rb_integer_pack call sites. The boundary is sound. Optional hardening: reject coercible non-Integer numerics (see L-1) and add a regression spec asserting fmul(2**256, …)/fred(2**512) raise.

I-2. scalar_reduce returns wrong result for hi=N+1 — unreachable via any public API. Same root cause as H-1 viewed at the scalar_reduce primitive. Every public entry point (rb_scalar_mod, rb_scalar_inv) calls scalar_reduce with hi=0, and rb_to_uint256 caps lo < 2²⁵⁶, so the hi==N+1 row is unreachable from Ruby — hence info rather than high for this primitive. Still must be fixed before v1.0 (the fix in H-1 resolves both): a future caller passing a non-zero hi (e.g. a 512-bit reduction helper) would silently produce wrong scalars used in signing. Add a regression asserting scalar_reduce(N+1, 0) == c_N.

I-3. fneg_internal returns incorrect results for unreduced inputs (a ≥ P); diverges from pure-Ruby fneg. negated = uint256_sub(FIELD_P, a) with only a zero-mask special case; for a ∈ [P, 2²⁵⁶) the subtraction underflows. fneg(2²⁵⁶−1) returns 0x…fefffffc30 vs correct 0x…fdfffff85f; pure-Ruby fneg returns a negative integer — all three (C, Ruby, canonical) diverge. The lone field op that assumes reduced input without enforcing it. Not reachable: every Point/SEC1 path negates already-reduced coordinates (from_bytes parity-flip, jp_neg, mul_vt table). Same fix shape as L-3 (reduce before negating); also add raise ArgumentError unless y < P to Point#initialize to close the Point.new(x, out-of-range y).negate avenue.

I-4. Malformed from_bytes inputs raise NoMethodError/arity-error instead of a clean ArgumentError. lib/secp256k1.rb:466-499. bytes.encoding runs before any type check (so nil/Integer raise NoMethodError), and an empty string reaches nil.to_s(16) (arity-error ArgumentError). Fails closed in every case (always raises, never returns a malformed point); pure robustness/contract hygiene. Fix: reject non-String/empty input with a clear ArgumentError at the top of from_bytes.

I-5. wNAF table cache is FIFO, not LRU as documented. lib/secp256k1.rb:290-331. Eviction (delete(keys.first)) runs only on a cache miss and removes the oldest inserted key; a hit never refreshes recency. Proven empirically: a touched-then-evicted entry confirms FIFO. Correctly bounded (600 distinct bases ⟹ size 512). Zero security impact: the cache key is derived solely from the public base-point coordinates (never the scalar), and the only caller mul_vt is documented variable-time/public-scalar-only; the constant-time ladder never touches this cache. Documentation nit only — correct the “simple LRU” comments to FIFO, or refresh recency on hit if true LRU is wanted.

I-6. Benign dead store in fred_internal. field.c:186 (acc = 0;) is immediately overwritten by = (not +=) at line 189 on every loop iteration, with no read in between. clang’s deadcode.DeadStores flags it; 20k+ differential fred cases confirm zero effect on the result. Unconditional store on an input-independent path — no leak, no timing impact. Optional cleanliness: delete the line (keep carry = 0; at line 187, which is read).

Positive-assurance findings (confirmed sound)

I-7. C field/scalar/point arithmetic and the Montgomery-ladder scalar multiplication are correct across all in-contract inputs. The primary assurance result, independently reproduced and strengthened with a geometric oracle (true affine group law, not a special-case-mirroring reference). Zero mismatches across tens of millions of cases including fred/scalar_reduce at the theoretical worst case hi = 2²⁵⁶−1 and at exact P/N multiples ±1 (all outputs canonical), fsqrt over a balanced QR/non-QR split, jp_add’s security-relevant corners (P+(−P)→∞ with mixed Z, equal-points-different-Z→double, infinity passthrough), and scalar_multiply_ct for k across [0,N) and k≥N vs independent affine k·G. ASan+UBSan clean over 729,300 mixed ops incl. off-curve/infinity inputs. Live: 237 compliance+native rspec examples pass; 2×1000 secret-scalar live differential runs report 0 fails. Recommendation: promote the geometric harness into the repo’s differential suite.

I-8. The Montgomery ladder cannot leak the scalar via add/double path or control flow. jacobian.c:384-436. The loop compiles to a fixed cswap; jp_add; jp_double; cswap; dec; jnb sequence; the only in-loop conditional jump is the public counter, and the secret bit reaches only cswap’s branchless mask (negq→pand/pxor, no data-dependent branch). All callees use mask-based selection. Differential (72 scalars incl. edge values + random) matches an independent double-and-add, 0 fails; dudect fixed-k vs random t ≈0.46–1.55 ≪ 4.5; ASan+UBSan clean across edge scalars that trigger the mid-ladder select_inf and aliased jp_add(r1,r0,r1). Recommendation: a CI assembly-invariant guard (exactly one conditional jump in the loop body; cswap stays branchless). Caveat (H-2): the “structurally branchless / t ≪ 4.5” result here was toolchain-specific — GCC 15.2/-O2 later reconstructed a branch in the uint256_select callee, pushing the ladder to t ≈ 21 until the value-barrier fix. The assembly-invariant guard this finding recommends is therefore load-bearing, not optional, and must run on the shipping compiler.

**I-9. jp_add_internal branchless special-case selection is correct and constant-time; the historical t =875 leak is confirmed remediated.** jacobian.c:180-290. 4190 differential cases (incl. forced h==0→double, h==0→infinity with random Z, and ladder aliasing forms) pass 0 fails. The only conditional jump in the symbol range is the stack canary; the six special-case selects compile to 24 cmove. A dudect Welch test over 3M measurements (normal add vs special-case paths) gives t =1.89 — direct confirmation the historical infinity-branch leak documented in risks.md is gone. Caveat (H-2): “the selects compile to 24 cmove” held on the review’s toolchain; under GCC 15.2/-O2 the same uint256_select calls compiled to conditional jumps instead. The value-barrier fix restores the cmove/pure-bitop codegen and is verified on GCC 15.2.

I-10. jp_double_internal and jp_neg_internal are correct and branchless. jacobian.c:95-141, 303-308. 6020 differential cases (random projective + Y=0, Z=0, infinity, non-trivial-Z) pass 0 fails; both infinity paths verified. Disassembly: jp_neg 0 conditional jumps, jp_double 2 (constant-bound select-loop counter + stack canary). ASan/UBSan clean over 200k iters incl. in-place aliasing and the neg(neg(p))==p involution.

I-11. Secret-dependent branches in scalar_reduce_limbs (scalar.c:166, :193) — a constant-time discipline gap, outside the documented CT scope. Valgrind poisoning deterministically reports secret-dependent control flow at scalar.c:166 (if (h == 0) continue;) and :193 (if (carry3)), reachable via scalar_mul/scalar_reduce/scalar_inv but not via the Montgomery ladder (jacobian.c contains no scalar-arithmetic calls). A material correction to the original triage: the branches fire frequently, not rarely — h==0 on 47.4% of limb iterations and carry3!=0 on 56.6% of calls over 3M random products, so the signal is far more measurable than “~2⁻⁶⁴” would suggest. Severity remains low because (1) security.md scopes the CT claim strictly to Point#mul/the ladder and the dudect table to field ops + scalar_multiply_ct — standalone scalar arithmetic is not claimed constant-time; (2) the branched-on value is a deeply-mixed function of the full 512-bit product, not a clean secret bit; (3) correctness is unaffected; (4) the fix is zero-cost. A consumer building ECDSA-style k⁻¹ on a secret nonce (Fermat scalar_inv loops scalar_mul ~256× over secret operands) would traverse these branches repeatedly. Fix (verified bit-identical over 2904 differential cases and clean under poisoning): delete if (h == 0) continue; at line 166 and make the carry3 fold at line 193 unconditional — both bodies are no-ops when the guard is false. Extend rake timing:verify to cover scalar_mul/scalar_reduce and update security.md to record the scalar layer as branchless.

Refuted

One prior claim was investigated and partially overturned — itself evidence of door-slamming:

R-1. “scalar_reduce is arithmetically correct, verified to the extremes” (prior info/not-a-bug verdict). Refuted for the scalar path. That conclusion was wrong: scalar_reduce_limbs has the real dropped-carry defect now tracked as H-1/I-2. The prior pass correctly verified the field reduction fred_internal (its fold constant c = 0x1000003D1 is only ~33 bits, so its third micro-fold cannot re-overflow — confirmed over 77,284 structured + 3,000,000 random cases) but incorrectly extended that assurance to the scalar reduction, whose fold constant c_N is ~129 bits and can re-overflow. Lumping field and scalar arithmetic together as “verified correct” masked a genuine high-severity defect. The split matters: field-arith assurance stands; scalar-arith did not, and now has a confirmed fix.

No other claims required refutation; the remaining hypotheses (marshalling OOB, ladder leakage, jp_* correctness) were each investigated and confirmed sound — recorded above as positive-assurance findings I-1 and I-7 through I-10.

4. Assurance and residual risk

What now has empirical backing:

Memory safety: zero ASan/UBSan diagnostics across every internal op over tens of millions of structured + random inputs (including deliberately unreduced, off-curve, infinity, and aliased inputs), plus the full rspec suite under sanitizers and a red-zone proof that the marshalling boundary never writes past its buffers. No memory-safety defect exists in the shipped C extension — the single largest vulnerability class in C/C++ crypto libraries (see risks.md).
Functional correctness on in-contract inputs: every field, scalar, and point operation, and the Montgomery-ladder scalar multiply, matches an independently-written reference (and the true affine group law via a geometric oracle) across tens of millions of cases including worst-case carry chains and security-relevant point corners.

Constant-time behaviour of the documented primitive: Point#mul / scalar_multiply_ct_internal is clean under deterministic secret-poisoning and well under the dudect threshold after the H-2 value-barrier fix — verified on bare metal (GCC 15.2, mean t = 0.68 over 20 runs) and clean under ctgrind. Two distinct CT regressions in this primitive have now been found empirically and remediated: the historical t =875 infinity-branch leak (source-level) and the t ≈21 GCC-15 compiler-reconstructed branch (H-2, compiled-level). The pairing is the case for keeping bare-metal dudect as a release gate.

What remains unverified or out of scope:

The five confirmed correctness defects (H-1, M-1, L-3, L-4, I-3) and the CT-discipline gap (I-11) are present in the reviewed v1.0 tree. The arithmetic assurance above is conditioned on in-contract inputs; H-1 and M-1 demonstrate that out-of-contract inputs reach silently-wrong results, and these must be fixed before release.
Timing measurements now have a bare-metal anchor (issue #25). The review’s original dudect numbers were taken in a virtualised environment and flagged as corroborative-only. They have since been re-run on quiet bare metal (AMD Ryzen 9 9950X, GCC 15.2, turbo off / performance governor / SMT off / pinned + RT-scheduled) — and that run is precisely what surfaced H-2, a leak the VM run had masked. This validates the original caution: deterministic ctgrind remains the primary CT evidence (hardware-independent data-flow), but it runs against CI’s compiler, and the bare-metal dudect run is the only thing that observes the shipping compiler’s timing — hence its promotion to a pre-tag gate. The marginal jp_add_internal ≈7.5 / fred ≈ operand-value artefacts (Z=1 vs non-trivial-Z, and operand-magnitude-asymmetric test classes) are measurement artefacts, not data-dependent branches (ctgrind-clean), and are catalogued in security.md and risks.md.
Interpreted-path side channels are not addressable with existing tooling and are not claimed (see risks.md). The pure-Ruby fallback’s constant-time properties are unverified by construction.
Standalone scalar arithmetic is not claimed constant-time and (per I-11) is not, pending the branchless fix.

Standing recommendation: this review establishes that the library withstands the class of attack an opportunistic adversary with commodity tooling would mount first, and that its core EC machinery is correct and side-channel-clean under deterministic analysis. It does not replace a professional cryptographic audit. Before this library is used to secure material value, commission a specialist audit covering the reduction algorithms’ mathematical correctness, a broader microarchitectural side-channel model, and the consumer integration patterns the primitives invite.

5. Reproducibility

All harnesses are preserved so every check above can be re-run and wired into CI:

Differential fuzzer: the independent Python reference (dfuzz_ref.py / geometric verify_drive.py) plus the standalone C harness (dfuzz_harness.c) built per the project’s certified compile line. The geometric (true-affine) oracle is strictly stronger than a special-case-mirroring reference and should be promoted into the repo’s differential test suite (it would have caught H-1’s class of defect from a structured-corner seed).
ctgrind / secret-poisoning: the valgrind/memcheck harness (ctgrind_harness.c, poison.c) that marks secrets UNDEFINED and runs under --error-exitcode=1. This is the deterministic CT check and should gate releases; it currently flags exactly the I-11 branches, which the verified branchless fix clears to “0 errors”.
Sanitizer sweep: asan_sweep.c, promotable into timing/, compiled with -fsanitize=address,undefined -fno-omit-frame-pointer and run under UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1.
Static analysis: the four-engine invocation (clang-tidy, clang --analyze, gcc -fanalyzer, high-warning compile) is a deterministic CI step.

Bare-metal dudect (statistical, off-CI): the standalone timing/ harness, built with the harness-portability fixes for modern GCC/glibc, run on a quiet frequency-pinned physical machine per the bare-metal runbook. This is the only check that observes the compiled timing on the shipping compiler and is what found H-2; it is a required pre-tag gate, re-run whenever the known-good compiler version changes (record CPU/microcode/kernel/compiler + per-op t with the run, as in security.md). Unlike the deterministic gates, it cannot run trustworthily in CI’s shared VM.

Recommended CI additions for v1.0: the differential geometric oracle with the H-1/M-1/L-3/L-4 structured regression vectors; the ctgrind release gate; the ASan/UBSan sweep; and an assembly-invariant guard on the ladder loop (I-8). Random testing provably cannot reach H-1’s failing band (density ~2⁻³⁸⁴) — the structured vectors are load-bearing, not optional.