docs(audit): record six verified-rejected findings; correct the #693 invariant

docs/audit-non-issues.md

@@ -31,9 +31,19 @@ counts**, both of which are capped far below the wrap point:
 - Value/element sizes are capped orders of magnitude below 4 GiB by platform
   state-transition and document size limits.
 - Tree element counts are accumulated one append at a time and committed into
-  hash-verified `Element` bytes; a verifier reads the count from bytes that
-  are checked against the trusted root **before** any arithmetic runs, so a
-  forged count is rejected as a bad proof, not fed into the math.
+  `Element` bytes that the proof chain binds to the trusted root, so a forged
+  count is ultimately rejected as a bad proof.
+
+  Be precise about *when* that binding happens, though: for the non-Merk
+  lower layers it is **after** the arithmetic, not before. `verify.rs:1953-2016`
+  dispatches the MMR / bulk-append / dense verifiers using an `Element`
+  decoded from the proof's own `value_bytes`, and only at `verify.rs:2018`
+  does `combine_hash(value_hash(value_bytes), &lower_hash)` check it against
+  the parent-committed hash. A forged count therefore *does* reach the math;
+  what makes that safe is that the release build wraps and the chain check
+  then rejects the result. Do not lean on "verified before the arithmetic"
+  as the invariant — and note that a build with `overflow-checks = true`
+  panics instead of wrapping.
 
 Verified-and-rejected instances:
 
@@ -42,7 +52,7 @@ Verified-and-rejected instances:
 | [#684](https://github.com/dashpay/grovedb/issues/684) (closed) | Unchecked size arithmetic undercounts storage costs | Overflow needs a single ~4 GiB length; cannot exist under key/value caps. Costs are bit-identical for every input that can occur. Its fix PR [#737](https://github.com/dashpay/grovedb/pull/737) was also closed: it converted `saturating_sub` to erroring `checked_sub` in fee-critical `paid_value_len` arithmetic — a semantic change resting on unverified never-underflows claims, with consensus-grade blast radius under a live protocol version. Do not re-propose checked arithmetic here unless narrowed to addition sites (behavior-identical by construction) and landed at the start of a protocol-version window. |
 | [#715](https://github.com/dashpay/grovedb/issues/715) (closed) | `StorageCost::verify` passes after u32 wrap | Same: `added_bytes + replaced_bytes` ≈ 4 GiB required; inputs are real serialized lengths, not attacker-supplied. |
 | [#716](https://github.com/dashpay/grovedb/issues/716) (closed) | Sectioned removal totals wrap u32 | Same: one element's removals would have to exceed 4 GiB of actually-stored bytes. |
-| [#693](https://github.com/dashpay/grovedb/issues/693) (closed) | Bulk-append `2 * leaf_count` overflows near 2^63 | Append side: 2^63 real appends is physically impossible. Verify side: `total_count` comes from hash-verified `Element::BulkAppendTree(count, ...)` bytes — a forged count breaks the parent hash before the arithmetic runs. |
+| [#693](https://github.com/dashpay/grovedb/issues/693) (closed) | Bulk-append `2 * leaf_count` overflows near 2^63 | Append side: 2^63 real appends is physically impossible. Verify side: the release build wraps harmlessly and the forged element is then rejected by the `combine_hash` chain. **Note the ordering** — the element is *not* hash-verified before the arithmetic runs: `verify.rs:1953-2016` dispatches the lower-layer verifiers using an `Element` decoded from the proof's own `value_bytes`, and only at `verify.rs:2018` does `combine_hash(value_hash(value_bytes), &lower_hash)` bind it to the parent-committed hash. So `total_count` reaching `2 * leaf_count - popcount` is unauthenticated at that moment; what saves it is that the wrap produces a value the subsequent chain check rejects, not a prior verification. In a build with `overflow-checks = true` this is a panic instead. |
 
 **Becomes real if:** a code path feeds these computations an integer that is
 neither derived from actually-stored bytes nor hash-verified before use
@@ -76,10 +86,13 @@ observing an append error.
 | Proof encoding truncates keys ≥ 256 bytes | Unreachable: every public insert path (direct + batch) enforces the 255-byte key limit (PR #506), so oversized keys cannot reach proof encoding. The `debug_assert!` is sufficient. Raw-Merk hardening exists separately (#728). |
 | `feature_type` forgery in `KVValueHashFeatureType` proof nodes | The decoded `_feature_type` is discarded by the verifier; the canonical type/sum/count lives in the hash-verified `Element` bytes. Forged values never reach callers. Documented in `verify.rs` comments and `proof_exploit_tests.rs`. |
 | `saturating_sub` on negative `SumItem` values corrupts `sum_limit` | Correct as written: `sum_limit` tracks the remaining **net sum** budget; +7 and −4 must consume 3, not 11. Absolute-value math would be the bug. |
+| [#691](https://github.com/dashpay/grovedb/issues/691) (closed) — a dense proof carrying only `node_hashes: [(0, root)]` and no entries verifies against a non-empty tree | The behavior is real but no in-repo verifier accepts it as an absence claim. `verify_for_query` (`grovedb-dense-fixed-sized-merkle-tree/src/verify.rs:86`) derives `expected_positions` from the query and errors on any missing position (`:108`); GroveDB uses exactly this when `report_contents == true` (`proof/verify.rs:2669`). When `report_contents == false` (`verify.rs:2644`) the entries are discarded and the root is forced to the parent-committed child hash by `combine_hash` (`verify.rs:2018`). The BulkAppend/CommitmentTree path calls the unbound `verify_and_get_root`, but the prover-chosen `dense_root` must satisfy `compute_state_root` against the trusted root (`grovedb-bulk-append-tree/src/proof/mod.rs:517`), and both `verify_against_query` (`:592-610`) and `verify_bulk_append_lower_layer` (`proof/verify.rs:2498-2515`) run their own missing-position check. The sibling bypasses from the same audit batch were already pinned by `test_vuln1_node_hashes_{root,ancestor}_bypass_rejected` (`tests.rs:247,270`), which date to the crate's first commit `6e4855f6` (2026-02-23) and so predate the filing. |
 
-**Becomes real if:** a new write path bypasses the 255-byte key check, or a
+**Becomes real if:** a new write path bypasses the 255-byte key check, a
 verifier starts trusting a decoded field instead of the hash-verified
-element bytes.
+element bytes, or a consumer calls the dense crate's
+`verify_against_expected_root` / `verify_and_get_root` directly and treats
+"verified, zero entries" as proof of absence without a completeness check.
 
 ## Proof-envelope trailing bytes: strict rejection is canonical, do not re-gate leniency
 
@@ -94,3 +107,33 @@ Two related claim shapes, both resolved:
 lenient grovedb (< v5.0.0) under GROVE_V3, or a consumer surfaces with
 persisted/padded proof blobs that must keep verifying. Absent that, do not
 re-propose version-gated leniency for proof-envelope trailing bytes.
+
+## Trust-boundary findings: corrupt storage and caller-chosen paths
+
+GroveDB's trust boundary puts local storage *inside* it. Findings whose
+trigger is "corrupt local storage" or "the caller passed a bad argument"
+are not vulnerabilities — integrity auditing is delegated to
+`Merk::verify` and `GroveDb::verify_grovedb` on purpose.
+
+| Issue | Claim | Why not real |
+|---|---|---|
+| [#682](https://github.com/dashpay/grovedb/issues/682) (closed) | Lazy-loaded Merk references do not validate fetched node metadata | Accurate as described — `TreeNode::load` (`merk/src/tree/mod.rs:1570-1614`) copies the parent link's `hash`, `child_heights` and `aggregate_data` verbatim without verifying them, and `Walker::detach` (`merk/src/tree/walk/mod.rs:80-95`) discards the link before fetching, so the apply path checks nothing. But the trigger is corrupt storage or a hostile `Fetch` impl, and `Fetch` has exactly two in-tree implementations (`MerkSource`, `PanicSource`). No untrusted input reaches `load`; proofs over a corrupt child fail verifier-side anyway because the verifier recomputes `kv_hash` from the transmitted value. **Do not "fix" this without weighing cost:** validating `hash_for_link` on every link load adds `hash_node_calls` to `OperationCost` on the hot read path, and cost changes are replay-critical (Platform replays historical blocks using the estimate as an admission bound). Separately, the `debug_assert_eq!(tree.key(), link.key())` at `merk/src/tree/mod.rs:1606` is tautological and can never fire — `TreeNode::decode` overwrites the decoded key with the lookup key (`merk/src/tree/encoding.rs:136-137`) — so it is dead weight that reads as a safety check. |
+| [#700](https://github.com/dashpay/grovedb/issues/700) (closed) | `delete_checkpoint` can delete non-checkpoint GroveDB directories | Technically accurate — a checkpoint dir is structurally identical to a normal RocksDB dir, so `open_checkpoint` cannot distinguish them, and the `path.components().count() < 2` guard admits `/tmp`. But there is no confused deputy: the only input is a path the caller chose, and the only call sites are tests (`checkpoint_tests.rs:211`, `misc_coverage_tests.rs:1821,1832`). A live DB is protected by RocksDB's LOCK, so only closed DBs are reachable, and only by a caller who typed the path. Side effect worth knowing: `open_checkpoint` on a real DB dir mutates it (WAL replay / MANIFEST churn) before the delete decision. |
+
+**Becomes real if:** a `Fetch` implementation is exposed to untrusted data
+(e.g. a network-backed source), or grovedb itself calls `delete_checkpoint`
+with a path derived from configuration or network input rather than from an
+immediate caller.
+
+## Version-gating and build hygiene reported as vulnerabilities
+
+| Issue | Claim | Why not real |
+|---|---|---|
+| [#702](https://github.com/dashpay/grovedb/issues/702) (closed) | `GroveVersion::default` creates protocol version 0, satisfying v0 gates | Inert on two independent grounds. (1) `protocol_version` is **never read by any logic in the workspace** — grep finds it only in the four version-constant definitions and in test assertions; every gate reads a *feature* slot such as `grove_version.grovedb_versions.operations.insert.*`. (2) `GroveVersion::default()` is behaviorally identical to `GROVE_V1`: every `FeatureVersion` in V1 is `0`, and the only non-zero values in `v1.rs` are `protocol_version: 1` (never read) and `max_aggregate_sum_query_elements_scanned: 1024`, which is not a version slot and whose hand-written `impl Default` returns the same 1024. So the worst outcome is "the caller got V1 behavior", which is already legal via an explicit `GROVE_V1`. |
+| [#703](https://github.com/dashpay/grovedb/issues/703) (closed) | Public versioned APIs miss explicit version gates and could execute writes instead of returning `VersionError` | Three of the seven cited refs are stale line numbers resolving to `root_key`, `root_hash` and `verify_grovedb` — two accessors and a verification helper, none version-dependent, none a write path; gating them would break callers on any unrelated slot bump. The four typed non-Merk append entry points (`mmr_tree_append`, `bulk_append`, `dense_tree_insert`, `commitment_tree_insert`) do lack a *top-level* gate but already fail closed deeper and **before any mutation**, via their cost dispatchers (`grovedb-merkle-mountain-range/src/cost/mod.rs:88-98`, `grovedb-bulk-append-tree/src/cost/mod.rs:182-201`, `grovedb-dense-fixed-sized-merkle-tree/src/tree/root_maintenance/mod.rs:56-71`, `grovedb-commitment-tree/src/commitment_tree/cost/mod.rs:43,73`). Writes also land in a `StorageBatch` committed only on success. The genuine residual is narrower and different: those four types have no `element_creation` gate of the kind `PrivateDocumentStore` uses (`grovedb_versions.rs:180-186`, enabled only in `v4.rs:407`) — a design-consistency question about when they become *creatable*, worth filing separately if wanted. |
+| [#721](https://github.com/dashpay/grovedb/issues/721) (closed) | `grovedbg` build script downloads a release artifact at build time | Acceptable posture. Gated off by default (`default = ["full", "estimated_costs"]`; with `grovedbg` off, `build.rs` compiles to a literal no-op and `reqwest`/`sha2` are not even resolved), and integrity is enforced by a pinned version tag plus a pinned `GROVEDBG_SHA256` asserted at `build.rs:33`. A substituted, tampered, or 404 artifact fails the build — fail-closed. Real but minor DX defect not named in the issue: the `if !grovedbg_zip_path.exists()` guard at `:15` caches a bad download, so every later rebuild fails the SHA assert without re-fetching until `target/` is cleared. |
+
+**Becomes real if:** `protocol_version` gains a reader that dispatches
+behavior on it; one of the four append paths gains a mutation that runs
+before its cost dispatcher; or `grovedbg` is added to the default feature
+set or its SHA-256 pin is dropped.