fix: prevent KV-to-KVValueHash proof value forgery attack (C1)

[QuantumExplorer]

Summary

Severity: Critical — Fixes a proof value forgery vulnerability in the merk proof verification layer.

The Attack

The merk proof system uses different node types to encode key-value data:

• KV(key, value) — the verifier computes value_hash = blake3(value) and uses it to build the Merkle tree hash
• KVValueHash(key, value, value_hash) — the proof provides the value_hash directly, and the verifier trusts it

The hash computation for both is algebraically identical:

kv_hash(key, value)              = blake3(key_len || key || value_hash(value))
kv_digest_to_kv_hash(key, vh)    = blake3(key_len || key || vh)

When vh = value_hash(value), these produce the same Merkle root. An attacker can:

1. Intercept a valid proof containing KV(key, real_value)
2. Replace it with KVValueHash(key, fake_value, value_hash(real_value))
3. The Merkle root still verifies correctly because the value_hash hasn't changed
4. The verifier returns fake_value to the caller, believing it is authentic

The Fix

Two new boolean fields are added to ProvedKeyOptionalValue and ProvedKeyValue:

• value_hash_is_computed: bool — true when the merk verifier independently computed the value_hash from the value bytes (KV, KVCount nodes), false when the hash was provided by the proof (KVValueHash, KVRefValueHash, etc.)
• is_reference_result: bool — true for reference node results (KVRefValueHash, KVRefValueHashCount) where the value is the dereferenced target and the hash is the reference node's hash (secured via combine_hash in tree.rs)

At the GroveDB verification layer, for item elements where value_hash_is_computed == false && is_reference_result == false, the verifier now independently computes value_hash(value_bytes) and checks it matches the proof's hash. A mismatch indicates proof tampering and returns an error.

This check correctly excludes:

• Subtree elements (Trees, SumTrees, etc.) — their value_hash is a combine_hash of the element hash and subtree root, verified separately
• Reference elements — their value_hash is the reference node's hash, with the dereferenced value secured via combine_hash in tree.rs
• KV/KVCount nodes — already safe because the verifier computes the hash

Files Changed

File	Change
merk/src/proofs/query/verify.rs	Added value_hash_is_computed and is_reference_result fields to ProvedKeyOptionalValue and ProvedKeyValue; updated execute_node closure to propagate both flags; set flags correctly per node type
grovedb/src/operations/proof/verify.rs	Added value_hash verification check for items with untrusted hashes in both V0 and V1 proof paths
grovedb/src/operations/proof/util.rs	Updated test constructors
grovedb/src/tests/proof_coverage_tests.rs	Updated test constructors
grovedb/src/tests/misc_coverage_tests.rs	Updated test constructors

Test plan

• cargo build — compiles cleanly
• cargo test -p grovedb --lib -- proof — all 198 proof tests pass
• cargo test -p grovedb --lib -- reference — all 93 reference tests pass (verifies references aren't falsely rejected)
• cargo test -p grovedb-merk -- proof — all 155 merk proof tests pass
• cargo test -p grovedb --lib -- coverage — all 367 coverage tests pass
• Manual review: verify the attack scenario is blocked by constructing a tampered proof

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Tests

  • Expanded proof verification test coverage with new scenarios and refactored existing tests to use dynamic proof generation instead of hardcoded values.

• Bug Fixes

  • Strengthened proof verification logic to validate element consistency and reject invalid proof structures detected during verification.

[coderabbitai]

Warning

Rate limit exceeded

@QuantumExplorer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 9 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4661f2e5-a701-48f0-a8e3-e6c75f628aee

📥 Commits

Reviewing files that changed from the base of the PR and between 3111dd8 and c4c5111.

📒 Files selected for processing (4)

• grovedb/src/tests/proof_coverage_tests.rs
• grovedb/src/tests/sum_tree_tests.rs
• merk/src/proofs/query/merk_integration_tests.rs
• merk/src/proofs/query/verify.rs

📝 Walkthrough

Walkthrough

The changes enhance proof verification security by introducing detection logic for KV-to-KVValueHash tampering attacks, validate item element presence in KVValueHash nodes, and refactor tests to use dynamic proof generation instead of hardcoded proofs while maintaining equivalent verification coverage.

Changes

Cohort / File(s)	Summary
Proof Verification Security merk/src/proofs/query/verify.rs	Added element type validation to reject item elements in KVValueHash and KVValueHashFeatureType nodes, preventing KV-to-KVValueHash forgery attacks.
Proof Coverage Tests grovedb/src/tests/proof_coverage_tests.rs	Introduced new test demonstrating KV-to-KVValueHash forgery attack with helper function to tamper proof bytes and validate rejection behavior.
Integration & Refactoring Tests grovedb/src/tests/sum_tree_tests.rs, merk/src/proofs/query/merk_integration_tests.rs	Refactored sum_tree_tests to use dynamic proof generation; updated tampering verification in integration test to assert error conditions and validate error messages contain expected rejection text.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Whiskers twitching with delight,
We've caught the forgers in the night!
KV to KVValueHash? Oh what a jest,
Our proofs now verify the best! ✨
Dynamic tests, no hardcoded chains—
Security blooms through our domains! 🌳

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: preventing a KV-to-KVValueHash proof value forgery attack (C1). It is specific, concise, and directly reflects the primary security fix across all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/C1-proof-value-forgery-via-kvvaluehash

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 42.85714% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.73%. Comparing base (319dea5) to head (c4c5111).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
merk/src/proofs/query/verify.rs	42.85%	12 Missing ⚠️

❌ Your patch status has failed because the patch coverage (42.85%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop     #553       +/-   ##
============================================
+ Coverage    59.95%   90.73%   +30.78%     
============================================
  Files          182      182               
  Lines        51082    51103       +21     
============================================
+ Hits         30624    46367    +15743     
+ Misses       20458     4736    -15722     

Components	Coverage Δ
grovedb-core	88.92% <ø> (+31.41%)	⬆️
merk	91.98% <42.85%> (+29.94%)	⬆️
storage	86.36% <ø> (+15.95%)	⬆️
commitment-tree	96.41% <ø> (+26.16%)	⬆️
mmr	96.72% <ø> (+28.41%)	⬆️
bulk-append-tree	90.85% <ø> (+18.64%)	⬆️
element	97.55% <ø> (+48.24%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[QuantumExplorer]

Approved

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@grovedb/src/operations/proof/verify.rs`:
- Around line 619-634: Reject any proof node where
!proved_key_value.value_hash_is_computed &&
!proved_key_value.is_reference_result unless you have verified and folded a
lower-layer hash into the combined hash; specifically, replace the current
exemption that checks element.is_any_tree() with logic that only allows
tree-encoded payloads when you recompute and validate the combined hash by
calling combine_hash(value_hash(value_bytes), lower_hash) (or equivalent) and
confirming it matches the node’s combined hash. Ensure the same fix is applied
in the other occurrence (the tree-target branch that currently skips recomputing
the combined hash and the duplicate check around include_empty_trees_in_result)
so that unauthenticated tree bytes cannot bypass verification or suppress
existing items.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4a7ff0b8-9ac1-4a3f-84bd-27362d967c69

📥 Commits

Reviewing files that changed from the base of the PR and between 1b119d3 and 4c34799.

📒 Files selected for processing (5)

• grovedb/src/operations/proof/util.rs
• grovedb/src/operations/proof/verify.rs
• grovedb/src/tests/misc_coverage_tests.rs
• grovedb/src/tests/proof_coverage_tests.rs
• merk/src/proofs/query/verify.rs

[coderabbitai]

🧹 Nitpick comments (2)

merk/src/proofs/query/verify.rs (1)

323-337: Extract the item-element rejection into one helper.

The ElementType::from_serialized_value(...).has_simple_value_hash() guard is now security-critical and duplicated in both KVValueHash branches. Centralizing it would reduce the chance that one branch drifts from the other later.

♻️ Suggested shape

+fn reject_item_in_value_hash_node(value: &[u8], node_name: &str) -> Result<(), Error> {
+    let element_type = ElementType::from_serialized_value(value).map_err(|e| {
+        Error::InvalidProofError(format!(
+            "cannot determine element type in {node_name}: {e}"
+        ))
+    })?;
+    if element_type.has_simple_value_hash() {
+        return Err(Error::InvalidProofError(format!(
+            "{node_name} must not contain an item element"
+        )));
+    }
+    Ok(())
+}
...
-                    let element_type = ElementType::from_serialized_value(value).map_err(|e| {
-                        Error::InvalidProofError(format!(
-                            "cannot determine element type in KVValueHash node: {e}"
-                        ))
-                    })?;
-                    if element_type.has_simple_value_hash() {
-                        return Err(Error::InvalidProofError(
-                            "KVValueHash node must not contain an item element".to_string(),
-                        ));
-                    }
+                    reject_item_in_value_hash_node(value, "KVValueHash node")?;
...
-                    let element_type = ElementType::from_serialized_value(value).map_err(|e| {
-                        Error::InvalidProofError(format!(
-                            "cannot determine element type in KVValueHashFeatureType node: {e}"
-                        ))
-                    })?;
-                    if element_type.has_simple_value_hash() {
-                        return Err(Error::InvalidProofError(
-                            "KVValueHashFeatureType node must not contain an item element"
-                                .to_string(),
-                        ));
-                    }
+                    reject_item_in_value_hash_node(value, "KVValueHashFeatureType node")?;

Also applies to: 373-384

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@merk/src/proofs/query/verify.rs` around lines 323 - 337, Extract the
duplicated guard into a small helper (e.g., validate_kv_valuehash_no_item or
ensure_not_item_element_in_kv_value_hash) that takes the serialized value (the
same input passed to ElementType::from_serialized_value) and returns Result<(),
Error>; move the ElementType::from_serialized_value(...).has_simple_value_hash()
check into that helper and map the error to the existing InvalidProofError
messages, then call this helper from both KVValueHash branches (the blocks
containing the current ElementType::from_serialized_value(...) check) to ensure
a single canonical check is used everywhere.

grovedb/src/tests/proof_coverage_tests.rs (1)

5919-6037: Add a matching regression for KVValueHashFeatureType.

This test only drives the plain KVValueHash path, but the fix in merk/src/proofs/query/verify.rs adds the same rejection logic for KVValueHashFeatureType. A second forged proof that hits that branch would keep the other security-sensitive path covered too.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@grovedb/src/tests/proof_coverage_tests.rs` around lines 5919 - 6037, Add a
second regression in kv_to_kvvaluehash_forgery_is_detected that constructs and
tampers a proof to hit the KVValueHashFeatureType branch: duplicate the
tampering flow (using tamper_kv_to_kvvaluehash or a new helper) but replace the
target merk node to be a KVValueHashFeatureType-encoded entry (keeping
value_hash from the real element and swapping in a fake value) and then assert
GroveDb::verify_query_raw returns an error mentioning the item
rejection/value-hash mismatch; ensure the test invokes the same proof
decode/modify/encode path so the new case covers the KVValueHashFeatureType
branch in verify_query_raw.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@grovedb/src/tests/proof_coverage_tests.rs`:
- Around line 5919-6037: Add a second regression in
kv_to_kvvaluehash_forgery_is_detected that constructs and tampers a proof to hit
the KVValueHashFeatureType branch: duplicate the tampering flow (using
tamper_kv_to_kvvaluehash or a new helper) but replace the target merk node to be
a KVValueHashFeatureType-encoded entry (keeping value_hash from the real element
and swapping in a fake value) and then assert GroveDb::verify_query_raw returns
an error mentioning the item rejection/value-hash mismatch; ensure the test
invokes the same proof decode/modify/encode path so the new case covers the
KVValueHashFeatureType branch in verify_query_raw.

In `@merk/src/proofs/query/verify.rs`:
- Around line 323-337: Extract the duplicated guard into a small helper (e.g.,
validate_kv_valuehash_no_item or ensure_not_item_element_in_kv_value_hash) that
takes the serialized value (the same input passed to
ElementType::from_serialized_value) and returns Result<(), Error>; move the
ElementType::from_serialized_value(...).has_simple_value_hash() check into that
helper and map the error to the existing InvalidProofError messages, then call
this helper from both KVValueHash branches (the blocks containing the current
ElementType::from_serialized_value(...) check) to ensure a single canonical
check is used everywhere.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 111f69a6-07da-49a8-a87b-285e81596a02

📥 Commits

Reviewing files that changed from the base of the PR and between 4c34799 and 3111dd8.

📒 Files selected for processing (4)

• grovedb/src/tests/proof_coverage_tests.rs
• grovedb/src/tests/sum_tree_tests.rs
• merk/src/proofs/query/merk_integration_tests.rs
• merk/src/proofs/query/verify.rs

✅ Files skipped from review due to trivial changes (1)

• merk/src/proofs/query/merk_integration_tests.rs

[QuantumExplorer]

Approved and heavily worked on.