Skip to content

VER-005: Maintainability and Documentation Findings - #6

Draft
gnosed wants to merge 1 commit into
mainfrom
audit/ver-005-maintainability-and-documentation
Draft

VER-005: Maintainability and Documentation Findings#6
gnosed wants to merge 1 commit into
mainfrom
audit/ver-005-maintainability-and-documentation

Conversation

@gnosed

@gnosed gnosed commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Remediates all ten items reported in Veridise VER-005 (issue #1290).

Important

This branch is stacked on VER-001 (#2). Its base is currently main, so
the diff shown here includes VER-001's five commits as well as this one.
Either retarget the base to audit/ver-001-nonstandard-0x29-signatures, or
merge #2 first — after which GitHub retargets this PR to main automatically
and the diff narrows to the single VER-005 commit.

The dependency is real, not incidental: item 7 edits verify.rs, the same
file VER-001 rewrites, and VER-005's original pass had rewritten the
"Accepted signature formats" module docs that VER-001 establishes were
factually wrong. VER-001's corrected text was taken wholesale for that
region.

What changed

# Finding Implementation
1 debug_assert!(v < Q) is dropped from release builds hash_to_point returns bool; verify_512 treats false as verification failure. See deviation below.
2 Unframed nonce || message absorption is undocumented Doc records the ambiguity and the fixed-40-byte-nonce requirement, plus a debug_assert_eq! on the length
3 Instance-storage footprint undocumented "Instance-storage footprint" section: 897 bytes, the 64 KiB shared entry cap, ~1.4%, and what a second key would cost
4 let logn = 9 duplicates FALCON_512_LOGN Local removed entirely; the shared constant is used in the compile-time assert
5 __check_auth length check is redundant Retained as documented defense in depth — it is what keeps the fixed-buffer copy panic-free
6 decode_pubkey may leave h partially written Documented: discard h on false
7 verify_raw_512 and ntt.rs over-exposed verify_raw_512 now private with its three caller-established invariants documented; ntt.rs is pub(crate) for exactly the six functions verify.rs imports, private for everything else
8 ni recomputed by halving R nine times FALCON_512_NI = 128 in lib.rs, used directly; a compile-time assert pins NI * N ≡ R (mod Q)
9 acc_len check can never fire Replaced with debug_assert_eq!(acc_len, 0) plus the 896 * 8 == 512 * 14 reasoning that holds it up
10 Events::publish is deprecated FalconInit / FalconRotate #[contractevent] types, wire shape unchanged so existing indexers keep working

Deviation from the recommendation (item 1)

The finding asks for "a runtime assertion." This PR does not use assert!.

assert! panics, and __check_auth must not: a panic there traps the host
call instead of returning Err(VerificationFailed), which is a worse on-chain
outcome than a clean rejection. hash_to_point therefore returns bool and
the caller fails verification. The safeguard runs in release builds either
way, which is what the finding is actually after.

Two notes on scope

Item 4 was satisfied by deletion rather than substitution. Item 8 removed
the halving loop that was logn's only consumer, so the duplicate local is
gone; FALCON_512_LOGN is still used in ntt.rs, in the assert pinning the
new constant.

Item 9 left one acc_len check in place, in decode_sig_compressed.
Signature bodies are variable-length, so its accumulator genuinely can hold
leftover bits. Only decode_pubkey's copy was ineffective, because the
896-byte gate makes that arithmetic exact.

Verification

  • falcon-512-core 7 tests, soroban-falcon-smart-account 18 (incl.
    testutils integration), soroban-falcon-verifier 8 — all passing
  • NIST KAT vectors still verify, confirming the FALCON_512_NI substitution
    and the visibility changes are behaviour-preserving
  • Both contracts build to WASM via stellar contract build
  • cargo clippy shows no new warnings

Also in this PR

Comments across the touched files were rewritten for clarity, and audit
finding IDs (VER-, DEC-, DRS-, F-00, PR references) were removed from
code comments — they belong in docs/audit/, not in the source, where they go
stale and mean nothing to a reader without the tracker. The reasoning each ID
pointed at was written out in place instead. One pointer to
docs/audit/constant-time-analysis.md is kept deliberately: it is the only
thing explaining why the modular reduction is four unrolled subtractions
rather than a loop that lowers to UDIV.

The regression test test_dec002_arbitrary_padding_rejected was renamed to
test_arbitrary_padding_rejected; the AUD-001 row in remediation-log.md
was updated to match.

@gnosed
gnosed force-pushed the audit/ver-005-maintainability-and-documentation branch 2 times, most recently from 01e38cf to c442b37 Compare August 26, 2026 11:15
Veridise VER-005 (#1290), all ten items, plus the tracking docs.

1. hash_to_point range-checks sampled coefficients in release builds,
   returning false (verification failure) rather than a debug-only assert,
   so __check_auth stays panic-free.
2. hash_to_point documents the unframed nonce||message absorption and the
   fixed 40-byte nonce requirement (plus a debug_assert on the length).
3. Smart-account module docs cover the 897-byte instance-storage footprint
   against the 64 KiB shared entry limit, as a design constraint for anyone
   adding instance state.
4/8. FALCON_512_NI = 128 (R/512 mod Q) in falcon-512-core lib.rs replaces
   the nine-step halving of R in ntt_inverse; a compile-time assert pins
   NI * N == R (mod Q) via FALCON_512_LOGN. field_halve deleted, dead once
   the loop went.
5. __check_auth pubkey-length check kept, documented as load-bearing: it is
   what makes the fixed-buffer copy panic-free.
6. decode_pubkey documents that h is unspecified and must be discarded when
   decoding fails.
7. verify_raw_512 made private with its caller-established invariants
   documented; ntt.rs narrowed to pub(crate)/private per actual callers.
9. Unreachable acc_len leftover-bits check replaced with
   debug_assert_eq!(acc_len, 0) plus the 896*8 = 512*14 tiling that holds
   the invariant up.
10. init/rotate events migrated off the deprecated Events::publish to
   FalconInit/FalconRotate #[contractevent] types, wire shape unchanged so
   existing indexers keep working.

Also reworks the surrounding comments: audit finding IDs dropped in favour
of the reasoning they pointed at, and the prose tightened throughout.

Tests (unit, KAT, testutils integration), clippy, and both WASM builds pass.
@gnosed
gnosed force-pushed the audit/ver-005-maintainability-and-documentation branch from c442b37 to 132b85f Compare August 27, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant