From ceca33f150e305573c13d9eb1c4de9dd257a273b Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 8 Jun 2026 15:08:06 +0200 Subject: [PATCH 1/4] fix(drive): strict merged-query verification for unshield & shielded withdrawal proofs The `Unshield` and `ShieldedWithdrawal` execution-proof verify paths checked their sub-proofs with subset verification, which tolerates extra subtree data in the proof. PR #3793 (commit 63c31f7c6e) tightened the sibling `ShieldFromAssetLock` surplus path to a single strict verification against the exact merged query the prover builds; this brings the two remaining shielded arms to the same pattern. Both arms now reconstruct the prover's exact merged `PathQuery` (same sub-queries, same cleared limits, same `PathQuery::merge`), set a non-`None` limit (`u16::MAX`, unreachable by the legitimate result set) since the strict verifier requires one, verify it once with `GroveDb::verify_query_with_absence_proof`, and partition the returned `proved_key_values` by path into the typed results (nullifier spend statuses + address balance / withdrawal document). This is defense-in-depth / canonicality, not a known exploit: the verified keys are derived from the signed transition and the sub-proofs are bound by a shared root hash, so a padded proof could not forge or misroute. The verifier now accepts exactly the data it asked for and rejects the rest. Adds a padded-proof negative test for each arm (mirroring `test_shield_from_asset_lock_padded_proof_is_rejected_by_strict_verify`) that proves a superset query padded with an unrelated `Pools` subtree and asserts the strict merged verify rejects it while the old subset verify would accept it. The existing nullifier/address/document roundtrip tests continue to pass. Closes #3812 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../shielded_withdrawal/tests.rs | 245 ++++++++++++++++++ .../state_transitions/unshield/tests.rs | 225 ++++++++++++++++ .../v0/mod.rs | 242 +++++++++++++---- 3 files changed, 662 insertions(+), 50 deletions(-) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs index 2f978377c2..3a42917939 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs @@ -1138,6 +1138,251 @@ mod tests { "withdrawal document should be present (not absent)" ); } + + /// NEGATIVE test for the strict-merged tightening (mirrors + /// `test_shield_from_asset_lock_padded_proof_is_rejected_by_strict_verify`): a proof + /// that carries EXTRA data beyond `{nullifiers, withdrawal-document}` MUST be rejected by + /// the production verifier. + /// + /// The production verify path rebuilds the merged `{nullifiers, withdrawal-document}` + /// query and verifies it with the STRICT `verify_query_with_absence_proof`, whose + /// succinctness check rejects any proof that descends into a subtree (a lower layer) the + /// query did not require. Here we execute a real shielded withdrawal, then have the + /// (honest, but over-broad) prover generate a SUPERSET proof for `{nullifiers, + /// withdrawal-document, + the genesis-populated Pools subtree}` and verify it against the + /// production merged query. The strict verifier must reject it because the proof carries + /// an extra root-level lower layer (`Pools`) the production query never touches. + /// + /// For contrast we also confirm the SUBSET verifier (the looser primitive this change + /// replaced) would have ACCEPTED the same padded proof — demonstrating exactly the hole + /// the strict merged verification closes. + #[test] + fn test_shielded_withdrawal_padded_proof_is_rejected_by_strict_verify() { + use drive::drive::shielded::paths::shielded_credit_pool_nullifiers_path_vec; + use drive::drive::RootTree; + use drive::grovedb::{GroveDb, PathQuery, Query, SizedQuery}; + use drive::query::{SingleDocumentDriveQuery, SingleDocumentDriveQueryContestedStatus}; + + let platform_version = PlatformVersion::latest(); + let platform = setup_platform(); + insert_dummy_encrypted_notes(&platform, 250); + let mut rng = StdRng::seed_from_u64(0); + let pk = get_proving_key(); + + // --- Create keys --- + let sk = SpendingKey::from_bytes([0u8; 32]).unwrap(); + let fvk = FullViewingKey::from(&sk); + let recipient = fvk.address_at(0u32, Scope::External); + let ask = SpendAuthorizingKey::from(&sk); + + // --- Create a spendable note with value 500M --- + let rho_bytes: [u8; 32] = { + let mut b = [0u8; 32]; + b[0] = 1; + b + }; + let rho = Rho::from_bytes(&rho_bytes).unwrap(); + let rseed = RandomSeed::from_bytes([42u8; 32], &rho).unwrap(); + let note = + Note::from_parts(recipient, NoteValue::from_raw(500_000_000), rho, rseed).unwrap(); + + // --- Build commitment tree and get anchor + merkle path --- + let cmx = ExtractedNoteCommitment::from(note.commitment()); + let mut tree = ClientMemoryCommitmentTree::new(100); + tree.append(cmx.to_bytes(), Retention::Marked).unwrap(); + tree.checkpoint(0u32).unwrap(); + let anchor = tree.anchor().unwrap(); + let merkle_path = tree.witness(Position::from(0u64), 0).unwrap().unwrap(); + + // --- Build bundle: spend 500M -> output 5K (value_balance = 499,995,000) --- + let mut builder = Builder::::new(BundleType::DEFAULT, anchor); + builder.add_spend(fvk.clone(), note, merkle_path).unwrap(); + builder + .add_output(None, recipient, NoteValue::from_raw(5_000), [0u8; 36]) + .unwrap(); + + let (unauthorized, _) = builder.build::(&mut rng).unwrap().unwrap(); + + let output_script = create_output_script(); + let unshielding_amount = 499_995_000u64; + let extra_sighash_data = dpp::shielded::shielded_withdrawal_extra_sighash_data( + output_script.as_bytes(), + unshielding_amount, + 1, + Pooling::Never, + ); + let bundle_commitment: [u8; 32] = unauthorized.commitment().into(); + let sighash = compute_platform_sighash(&bundle_commitment, &extra_sighash_data); + + let proven = unauthorized.create_proof(pk, &mut rng).unwrap(); + let bundle = proven.apply_signatures(rng, sighash, &[ask]).unwrap(); + + let (actions, value_balance, anchor_bytes, proof_bytes, binding_sig) = + serialize_authorized_bundle_i64(&bundle); + assert_eq!(value_balance, 499_995_000); + + insert_anchor_into_state(&platform, &anchor_bytes); + set_pool_total_balance(&platform, 500_000_000); + + let transition = create_shielded_withdrawal_transition( + actions, + value_balance as u64, + anchor_bytes, + proof_bytes, + binding_sig, + 1, + Pooling::Never, + output_script.clone(), + ); + + let transition_bytes = transition + .serialize_to_bytes() + .expect("should serialize transition"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![transition_bytes], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }] + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit transaction"); + + // --- Reconstruct the PRODUCTION merged query exactly as the verifier does --- + // {nullifiers} ∪ {withdrawal-document}, each with cleared limits, then a limit that + // can never truncate the legitimate result set. + let StateTransition::ShieldedWithdrawal(ref st) = transition else { + unreachable!(); + }; + let nullifier_keys: Vec> = st.nullifiers(); + + // Compute withdrawal document ID deterministically (same as prove/verify sides). + let first_nullifier = nullifier_keys + .first() + .expect("should have at least one nullifier"); + let mut entropy = Vec::new(); + entropy.extend_from_slice(first_nullifier); + entropy.extend_from_slice(output_script.as_bytes()); + let document_id = Document::generate_document_id_v0( + &withdrawals_contract::ID, + &withdrawals_contract::OWNER_ID, + withdrawal::NAME, + &entropy, + ); + + let mut nf_query = Query::new(); + nf_query.insert_keys(nullifier_keys); + let nullifier_pq = PathQuery::new( + shielded_credit_pool_nullifiers_path_vec(), + SizedQuery::new(nf_query, None, None), + ); + + let doc_query = SingleDocumentDriveQuery { + contract_id: withdrawals_contract::ID.to_buffer(), + document_type_name: withdrawal::NAME.to_string(), + document_type_keeps_history: false, + document_id: document_id.to_buffer(), + block_time_ms: None, + contested_status: SingleDocumentDriveQueryContestedStatus::NotContested, + }; + let mut doc_pq = doc_query + .construct_path_query(platform_version) + .expect("construct doc path query"); + doc_pq.query.limit = None; + + let mut production_pq = PathQuery::merge( + vec![&nullifier_pq, &doc_pq], + &platform_version.drive.grove_version, + ) + .expect("merge production query"); + production_pq.query.limit = Some(u16::MAX); + + // Sanity: an HONEST proof for the production query verifies strictly (liveness). + let honest_proof = platform + .drive + .grove_get_proved_path_query( + &production_pq, + None, + &mut vec![], + &platform_version.drive, + ) + .expect("honest production proof"); + GroveDb::verify_query_with_absence_proof( + &honest_proof, + &production_pq, + &platform_version.drive.grove_version, + ) + .expect("strict verify of honest production proof must succeed"); + + // --- Build a SUPERSET (padded) proof: {nullifiers, document, + an extra subtree} --- + // Pad by ALSO descending the genesis-populated `Pools` root subtree, which the + // production query never touches; the padded proof carries an extra root-level layer. + let mut pools_top = Query::new(); + pools_top.insert_key(vec![RootTree::Pools as u8]); + pools_top.set_subquery(Query::new_range_full()); + let pools_pq = PathQuery::new(vec![], SizedQuery::new(pools_top, None, None)); + + let mut superset_pq = PathQuery::merge( + vec![&nullifier_pq, &doc_pq, &pools_pq], + &platform_version.drive.grove_version, + ) + .expect("merge superset query"); + superset_pq.query.limit = Some(u16::MAX); + + let padded_proof = platform + .drive + .grove_get_proved_path_query( + &superset_pq, + None, + &mut vec![], + &platform_version.drive, + ) + .expect("padded superset proof"); + + // The STRICT verifier (production behavior) MUST reject the padded proof. + let strict_result = GroveDb::verify_query_with_absence_proof( + &padded_proof, + &production_pq, + &platform_version.drive.grove_version, + ); + assert!( + strict_result.is_err(), + "strict verifier must reject a proof padded with an extra subtree layer, got {:?}", + strict_result + ); + + // Contrast: the SUBSET verifier (the looser primitive this change replaced) tolerates + // the extra layer and ACCEPTS the same padded proof — the exact hole now closed. + let subset_result = GroveDb::verify_subset_query_with_absence_proof( + &padded_proof, + &production_pq, + &platform_version.drive.grove_version, + ); + assert!( + subset_result.is_ok(), + "subset verifier was expected to tolerate the padded proof, got {:?}", + subset_result + ); + } } mod credit_conservation { diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs index 3fe96a5e19..c1142691d2 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs @@ -968,5 +968,230 @@ mod tests { output_address ); } + + /// NEGATIVE test for the strict-merged tightening (mirrors + /// `test_shield_from_asset_lock_padded_proof_is_rejected_by_strict_verify`): a proof + /// that carries EXTRA data beyond `{nullifiers, output-address}` MUST be rejected by the + /// production verifier. + /// + /// The production verify path rebuilds the merged `{nullifiers, output-address}` query + /// and verifies it with the STRICT `verify_query_with_absence_proof`, whose succinctness + /// check rejects any proof that descends into a subtree (a lower layer) the query did not + /// require. Here we execute a real unshield, then have the (honest, but over-broad) + /// prover generate a SUPERSET proof for `{nullifiers, output-address, + the + /// genesis-populated Pools subtree}` and verify it against the production merged query. + /// The strict verifier must reject it because the proof carries an extra root-level lower + /// layer (`Pools`) the production query never touches. + /// + /// For contrast we also confirm the SUBSET verifier (the looser primitive this change + /// replaced) would have ACCEPTED the same padded proof — demonstrating exactly the hole + /// the strict merged verification closes. + #[test] + fn test_unshield_padded_proof_is_rejected_by_strict_verify() { + use drive::drive::shielded::paths::shielded_credit_pool_nullifiers_path_vec; + use drive::drive::RootTree; + use drive::grovedb::{GroveDb, PathQuery, Query, SizedQuery}; + + let platform_version = PlatformVersion::latest(); + let platform = setup_platform(); + insert_dummy_encrypted_notes(&platform, 250); + let mut rng = StdRng::seed_from_u64(0); + let pk = get_proving_key(); + + let spend_amount = 500_000_000u64; + let output_amount = 5_000u64; + + // --- Create keys --- + let sk = SpendingKey::from_bytes([0u8; 32]).unwrap(); + let fvk = FullViewingKey::from(&sk); + let recipient = fvk.address_at(0u32, Scope::External); + let ask = SpendAuthorizingKey::from(&sk); + + // --- Create a spendable note --- + let rho_bytes: [u8; 32] = { + let mut b = [0u8; 32]; + b[0] = 1; + b + }; + let rho = Rho::from_bytes(&rho_bytes).unwrap(); + let rseed = RandomSeed::from_bytes([42u8; 32], &rho).unwrap(); + let note = + Note::from_parts(recipient, NoteValue::from_raw(spend_amount), rho, rseed).unwrap(); + + // --- Build commitment tree and get anchor + merkle path --- + let cmx = ExtractedNoteCommitment::from(note.commitment()); + let mut tree = ClientMemoryCommitmentTree::new(100); + tree.append(cmx.to_bytes(), Retention::Marked).unwrap(); + tree.checkpoint(0u32).unwrap(); + let anchor = tree.anchor().unwrap(); + let merkle_path = tree.witness(Position::from(0u64), 0).unwrap().unwrap(); + + // --- Build bundle: spend 500M -> output 5K (value_balance = 499,995,000) --- + let mut builder = Builder::::new(BundleType::DEFAULT, anchor); + builder.add_spend(fvk.clone(), note, merkle_path).unwrap(); + builder + .add_output( + None, + recipient, + NoteValue::from_raw(output_amount), + [0u8; 36], + ) + .unwrap(); + + let (unauthorized, _) = builder.build::(&mut rng).unwrap().unwrap(); + + let output_address = create_output_address(); + let unshielding_amount = 499_995_000u64; + let extra_sighash_data = dpp::shielded::unshield_extra_sighash_data( + &output_address.to_bytes(), + unshielding_amount, + ); + let bundle_commitment: [u8; 32] = unauthorized.commitment().into(); + let sighash = compute_platform_sighash(&bundle_commitment, &extra_sighash_data); + + let proven = unauthorized.create_proof(pk, &mut rng).unwrap(); + let bundle = proven.apply_signatures(rng, sighash, &[ask]).unwrap(); + + let (actions, value_balance, anchor_bytes, proof_bytes, binding_sig) = + serialize_authorized_bundle_i64(&bundle); + assert_eq!(value_balance, 499_995_000); + + insert_anchor_into_state(&platform, &anchor_bytes); + set_pool_total_balance(&platform, 500_000_000); + + let transition = create_unshield_transition( + output_address.clone(), + actions, + value_balance as u64, + anchor_bytes, + proof_bytes, + binding_sig, + ); + + let transition_bytes = transition + .serialize_to_bytes() + .expect("should serialize transition"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![transition_bytes], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }] + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit transaction"); + + // --- Reconstruct the PRODUCTION merged query exactly as the verifier does --- + // {nullifiers} ∪ {output address}, each with cleared limits, then a limit that can + // never truncate the legitimate result set. + let StateTransition::Unshield(ref st) = transition else { + unreachable!(); + }; + let nullifier_keys: Vec> = st.nullifiers(); + + let mut nf_query = Query::new(); + nf_query.insert_keys(nullifier_keys); + let nullifier_pq = PathQuery::new( + shielded_credit_pool_nullifiers_path_vec(), + SizedQuery::new(nf_query, None, None), + ); + + let mut address_pq = + Drive::balances_for_clear_addresses_query(std::iter::once(&output_address)); + address_pq.query.limit = None; + + let mut production_pq = PathQuery::merge( + vec![&nullifier_pq, &address_pq], + &platform_version.drive.grove_version, + ) + .expect("merge production query"); + production_pq.query.limit = Some(u16::MAX); + + // Sanity: an HONEST proof for the production query verifies strictly (liveness). + let honest_proof = platform + .drive + .grove_get_proved_path_query( + &production_pq, + None, + &mut vec![], + &platform_version.drive, + ) + .expect("honest production proof"); + GroveDb::verify_query_with_absence_proof( + &honest_proof, + &production_pq, + &platform_version.drive.grove_version, + ) + .expect("strict verify of honest production proof must succeed"); + + // --- Build a SUPERSET (padded) proof: {nullifiers, address, + an extra subtree} --- + // Pad by ALSO descending the genesis-populated `Pools` root subtree, which the + // production query never touches; the padded proof carries an extra root-level layer. + let mut pools_top = Query::new(); + pools_top.insert_key(vec![RootTree::Pools as u8]); + pools_top.set_subquery(Query::new_range_full()); + let pools_pq = PathQuery::new(vec![], SizedQuery::new(pools_top, None, None)); + + let mut superset_pq = PathQuery::merge( + vec![&nullifier_pq, &address_pq, &pools_pq], + &platform_version.drive.grove_version, + ) + .expect("merge superset query"); + superset_pq.query.limit = Some(u16::MAX); + + let padded_proof = platform + .drive + .grove_get_proved_path_query( + &superset_pq, + None, + &mut vec![], + &platform_version.drive, + ) + .expect("padded superset proof"); + + // The STRICT verifier (production behavior) MUST reject the padded proof. + let strict_result = GroveDb::verify_query_with_absence_proof( + &padded_proof, + &production_pq, + &platform_version.drive.grove_version, + ); + assert!( + strict_result.is_err(), + "strict verifier must reject a proof padded with an extra subtree layer, got {:?}", + strict_result + ); + + // Contrast: the SUBSET verifier (the looser primitive this change replaced) tolerates + // the extra layer and ACCEPTS the same padded proof — the exact hole now closed. + let subset_result = GroveDb::verify_subset_query_with_absence_proof( + &padded_proof, + &production_pq, + &platform_version.drive.grove_version, + ); + assert!( + subset_result.is_ok(), + "subset verifier was expected to tolerate the padded proof, got {:?}", + subset_result + ); + } } } diff --git a/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs b/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs index 3b8caf885c..2901cd9b49 100644 --- a/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs +++ b/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs @@ -1133,18 +1133,110 @@ impl Drive { Ok((root_hash, VerifiedAddressInfos(balances))) } StateTransition::Unshield(st) => { + use crate::drive::shielded::paths::shielded_credit_pool_nullifiers_path_vec; use dpp::state_transition::proof_result::StateTransitionProofResult::VerifiedShieldedNullifiersWithAddressInfos; use dpp::state_transition::unshield_transition::accessors::UnshieldTransitionAccessorsV0; + use grovedb::Element; let nullifier_keys: Vec> = st.nullifiers(); - let (root_hash_nf, statuses) = Drive::verify_shielded_nullifiers( - proof, - &nullifier_keys, - true, - platform_version, + // The prove side merges the nullifier spend-status sub-query with the + // output-address balance sub-query into a SINGLE multi-root proof (clearing each + // sub-query limit before the merge, because `PathQuery::merge` rejects limited + // sub-queries). Reconstruct the byte-identical merged query here and verify it + // STRICTLY: the strict verifier accepts a proof that matches the merged query + // exactly and REJECTS any proof carrying extra branches, so a malicious/buggy + // prover cannot pad the proof with unrelated state. (See the `ShieldFromAssetLock` + // arm below for the full rationale on why a single strict merged verify is sound.) + let mut nf_query = grovedb::Query::new(); + nf_query.insert_keys(nullifier_keys); + let nullifier_pq = grovedb::PathQuery::new( + shielded_credit_pool_nullifiers_path_vec(), + grovedb::SizedQuery::new(nf_query, None, None), + ); + + let mut address_pq = + Drive::balances_for_clear_addresses_query(std::iter::once(st.output_address())); + address_pq.query.limit = None; + + let mut merged_pq = grovedb::PathQuery::merge( + vec![&nullifier_pq, &address_pq], + &platform_version.drive.grove_version, )?; + // `verify_query_with_absence_proof` (the STRICT verifier) requires a limit, but + // `PathQuery::merge` clears it. The merged query targets a fixed, tiny set of + // explicit keys ({nullifiers} ∪ {output address}), so set a limit that can never + // be exhausted by the legitimate result set. An unreachable limit guarantees every + // layer is fully traversed and every extra branch is caught by the succinctness + // check (which is independent of the limit value). + merged_pq.query.limit = Some(u16::MAX); + + let (root_hash, proved_key_values) = + grovedb::GroveDb::verify_query_with_absence_proof( + proof, + &merged_pq, + &platform_version.drive.grove_version, + )?; + + // Partition the proved key/values by path: entries under the nullifiers tree are + // spend statuses, entries under the clear-address pool are address balances. + let nullifiers_path = shielded_credit_pool_nullifiers_path_vec(); + let addresses_path = Drive::clear_addresses_path(); + + let mut statuses: Vec<(Vec, bool)> = Vec::new(); + let mut balances: BTreeMap> = + BTreeMap::new(); + + for (path, key, element) in proved_key_values { + if path == nullifiers_path { + // A present element means the nullifier is spent; absence means unspent. + statuses.push((key, element.is_some())); + } else if path == addresses_path { + // Mirror `verify_addresses_infos_v0`: reconstruct the address from the key + // and decode the `ItemWithSumItem` (nonce, balance) element. + let address = PlatformAddress::from_bytes(&key).map_err(|e| { + Error::Proof(ProofError::CorruptedProof(format!( + "failed to deserialize output PlatformAddress: {}", + e + ))) + })?; + + let balance_info = element + .map(|element| { + let Element::ItemWithSumItem(nonce_vec, balance_i64, _) = element + else { + return Err(Error::Proof(ProofError::CorruptedProof( + "expected an item with sum item element".to_string(), + ))); + }; + + let nonce_bytes: [u8; 4] = nonce_vec.try_into().map_err(|_| { + Error::Proof(ProofError::IncorrectValueSize( + "nonce should be 4 bytes", + )) + })?; + let nonce = AddressNonce::from_be_bytes(nonce_bytes); + + if balance_i64 < 0 { + return Err(Error::Proof(ProofError::CorruptedProof( + "balance cannot be negative".to_string(), + ))); + } + + Ok((nonce, balance_i64 as Credits)) + }) + .transpose()?; + + balances.insert(address, balance_info); + } else { + return Err(Error::Proof(ProofError::CorruptedProof( + "unshield proof contained an entry outside the nullifier and address subtrees".to_string(), + ))); + } + } + + // Every nullifier must be present and marked spent. for (nf, is_spent) in &statuses { if !is_spent { return Err(Error::Proof(ProofError::IncorrectProof(format!( @@ -1154,25 +1246,8 @@ impl Drive { } } - let (root_hash_addr, balances): ( - RootHash, - BTreeMap>, - ) = Drive::verify_addresses_infos( - proof, - std::iter::once(st.output_address()), - true, - platform_version, - )?; - - if root_hash_nf != root_hash_addr { - return Err(Error::Proof(ProofError::CorruptedProof( - "unshield proof root hashes do not match between nullifiers and address" - .to_string(), - ))); - } - Ok(( - root_hash_nf, + root_hash, VerifiedShieldedNullifiersWithAddressInfos(statuses, balances), )) } @@ -1202,31 +1277,18 @@ impl Drive { Ok((root_hash, VerifiedShieldedNullifiers(statuses))) } StateTransition::ShieldedWithdrawal(st) => { + use crate::drive::shielded::paths::shielded_credit_pool_nullifiers_path_vec; use dpp::data_contracts::withdrawals_contract; use dpp::data_contracts::withdrawals_contract::v1::document_types::withdrawal; + use dpp::document::serialization_traits::DocumentPlatformConversionMethodsV0; use dpp::document::Document; use dpp::state_transition::proof_result::StateTransitionProofResult::VerifiedShieldedNullifiersWithWithdrawalDocument; use dpp::state_transition::shielded_withdrawal_transition::accessors::ShieldedWithdrawalTransitionAccessorsV0; + use grovedb::Element; let nullifier_keys: Vec> = st.nullifiers(); - let (root_hash_nf, statuses) = Drive::verify_shielded_nullifiers( - proof, - &nullifier_keys, - true, - platform_version, - )?; - - for (nf, is_spent) in &statuses { - if !is_spent { - return Err(Error::Proof(ProofError::IncorrectProof(format!( - "nullifier {} was not found as spent in the shielded withdrawal proof", - hex::encode(nf) - )))); - } - } - - // Compute withdrawal document ID deterministically (same as prove side) + // Compute withdrawal document ID deterministically (same as prove side). let first_nullifier = nullifier_keys.first().ok_or_else(|| { Error::Proof(ProofError::InvalidTransition( "shielded withdrawal has no nullifiers".to_string(), @@ -1268,25 +1330,105 @@ impl Drive { contested_status: SingleDocumentDriveQueryContestedStatus::NotContested, }; - let (root_hash_doc, maybe_doc) = - doc_query.verify_proof(true, proof, document_type, platform_version)?; + // The prove side merges the nullifier spend-status sub-query with the + // withdrawal-document sub-query into a SINGLE multi-root proof (clearing each + // sub-query limit before the merge, because `PathQuery::merge` rejects limited + // sub-queries). Reconstruct the byte-identical merged query here and verify it + // STRICTLY: the strict verifier rejects any proof carrying extra subtree branches, + // so a malicious/buggy prover cannot pad the proof with unrelated state. (See the + // `ShieldFromAssetLock` arm below for the full rationale.) + let mut nf_query = grovedb::Query::new(); + nf_query.insert_keys(nullifier_keys); + let nullifier_pq = grovedb::PathQuery::new( + shielded_credit_pool_nullifiers_path_vec(), + grovedb::SizedQuery::new(nf_query, None, None), + ); - if root_hash_nf != root_hash_doc { - return Err(Error::Proof(ProofError::CorruptedProof( - "shielded withdrawal proof root hashes do not match between nullifiers and document" - .to_string(), - ))); + let mut doc_pq = doc_query.construct_path_query(platform_version)?; + doc_pq.query.limit = None; + let document_path = doc_pq.path.clone(); + + let mut merged_pq = grovedb::PathQuery::merge( + vec![&nullifier_pq, &doc_pq], + &platform_version.drive.grove_version, + )?; + + // `verify_query_with_absence_proof` (the STRICT verifier) requires a limit, but + // `PathQuery::merge` clears it. The merged query targets a fixed, tiny set of + // explicit keys ({nullifiers} ∪ {withdrawal document}), so set a limit that can + // never be exhausted by the legitimate result set; this guarantees every layer is + // fully traversed and every extra branch is caught by the succinctness check. + merged_pq.query.limit = Some(u16::MAX); + + let (root_hash, proved_key_values) = + grovedb::GroveDb::verify_query_with_absence_proof( + proof, + &merged_pq, + &platform_version.drive.grove_version, + )?; + + // Partition the proved key/values by path: entries under the nullifiers tree are + // spend statuses; the single entry under the withdrawal-document tree is the + // proven document. + let nullifiers_path = shielded_credit_pool_nullifiers_path_vec(); + + let mut statuses: Vec<(Vec, bool)> = Vec::new(); + let mut document_element: Option> = None; + + for (path, key, element) in proved_key_values { + if path == nullifiers_path { + statuses.push((key, element.is_some())); + } else if path == document_path { + if document_element.is_some() { + return Err(Error::Proof(ProofError::CorruptedProof( + "shielded withdrawal proof contained more than one withdrawal document".to_string(), + ))); + } + document_element = Some(element); + } else { + return Err(Error::Proof(ProofError::CorruptedProof( + "shielded withdrawal proof contained an entry outside the nullifier and document subtrees".to_string(), + ))); + } } - let doc = maybe_doc.ok_or_else(|| { + // Every nullifier must be present and marked spent. + for (nf, is_spent) in &statuses { + if !is_spent { + return Err(Error::Proof(ProofError::IncorrectProof(format!( + "nullifier {} was not found as spent in the shielded withdrawal proof", + hex::encode(nf) + )))); + } + } + + let document_element = document_element.ok_or_else(|| { Error::Proof(ProofError::CorruptedProof( - "shielded withdrawal was executed but withdrawal document is missing from proof".to_string(), + "shielded withdrawal document key absent from proof".to_string(), )) })?; + + let doc = match document_element { + Some(Element::Item(serialized, _)) => Document::from_bytes( + serialized.as_slice(), + document_type, + platform_version, + )?, + Some(_) => { + return Err(Error::Proof(ProofError::CorruptedProof( + "expected an item element for withdrawal document".to_string(), + ))); + } + None => { + return Err(Error::Proof(ProofError::CorruptedProof( + "shielded withdrawal was executed but withdrawal document is missing from proof".to_string(), + ))); + } + }; let documents = BTreeMap::from([(document_id, Some(doc))]); Ok(( - root_hash_nf, + root_hash, VerifiedShieldedNullifiersWithWithdrawalDocument(statuses, documents), )) } From 92ba30679b62e50bc627870926e9acedcfa68f6c Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 8 Jun 2026 22:29:55 +0700 Subject: [PATCH 2/4] refactor(drive): extract verify_merged_query_strict for shielded merged proofs The Unshield, ShieldedWithdrawal, and ShieldFromAssetLock (surplus) arms each inlined the same merged multi-root proof verification: clear every sub-query limit, merge, set an unreachable u16::MAX limit, then run the strict verify_query_with_absence_proof. Collapse the three copies (and their copy-pasted soundness rationale) into a single Drive::verify_merged_query_strict helper, mirroring the existing Drive::verify_shielded_nullifiers factoring. No behavior change: the helper clears sub-query limits and sets the same u16::MAX merged limit the arms set before. Each arm still partitions the returned key/values against its own sub-query paths. Addresses review feedback to deduplicate this block. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v0/mod.rs | 164 ++++++++---------- 1 file changed, 74 insertions(+), 90 deletions(-) diff --git a/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs b/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs index 2901cd9b49..c316b8a6bd 100644 --- a/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs +++ b/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs @@ -1140,14 +1140,10 @@ impl Drive { let nullifier_keys: Vec> = st.nullifiers(); - // The prove side merges the nullifier spend-status sub-query with the - // output-address balance sub-query into a SINGLE multi-root proof (clearing each - // sub-query limit before the merge, because `PathQuery::merge` rejects limited - // sub-queries). Reconstruct the byte-identical merged query here and verify it - // STRICTLY: the strict verifier accepts a proof that matches the merged query - // exactly and REJECTS any proof carrying extra branches, so a malicious/buggy - // prover cannot pad the proof with unrelated state. (See the `ShieldFromAssetLock` - // arm below for the full rationale on why a single strict merged verify is sound.) + // Reconstruct the prove side's merged query — nullifier spend-status ∪ + // output-address balance — and verify it strictly. See + // `verify_merged_query_strict` for why a single strict merged verify is + // sound and rejects proofs padded with extra subtree branches. let mut nf_query = grovedb::Query::new(); nf_query.insert_keys(nullifier_keys); let nullifier_pq = grovedb::PathQuery::new( @@ -1155,30 +1151,15 @@ impl Drive { grovedb::SizedQuery::new(nf_query, None, None), ); - let mut address_pq = + let address_pq = Drive::balances_for_clear_addresses_query(std::iter::once(st.output_address())); - address_pq.query.limit = None; - let mut merged_pq = grovedb::PathQuery::merge( - vec![&nullifier_pq, &address_pq], - &platform_version.drive.grove_version, + let (root_hash, proved_key_values) = Self::verify_merged_query_strict( + proof, + vec![nullifier_pq, address_pq], + platform_version, )?; - // `verify_query_with_absence_proof` (the STRICT verifier) requires a limit, but - // `PathQuery::merge` clears it. The merged query targets a fixed, tiny set of - // explicit keys ({nullifiers} ∪ {output address}), so set a limit that can never - // be exhausted by the legitimate result set. An unreachable limit guarantees every - // layer is fully traversed and every extra branch is caught by the succinctness - // check (which is independent of the limit value). - merged_pq.query.limit = Some(u16::MAX); - - let (root_hash, proved_key_values) = - grovedb::GroveDb::verify_query_with_absence_proof( - proof, - &merged_pq, - &platform_version.drive.grove_version, - )?; - // Partition the proved key/values by path: entries under the nullifiers tree are // spend statuses, entries under the clear-address pool are address balances. let nullifiers_path = shielded_credit_pool_nullifiers_path_vec(); @@ -1330,13 +1311,10 @@ impl Drive { contested_status: SingleDocumentDriveQueryContestedStatus::NotContested, }; - // The prove side merges the nullifier spend-status sub-query with the - // withdrawal-document sub-query into a SINGLE multi-root proof (clearing each - // sub-query limit before the merge, because `PathQuery::merge` rejects limited - // sub-queries). Reconstruct the byte-identical merged query here and verify it - // STRICTLY: the strict verifier rejects any proof carrying extra subtree branches, - // so a malicious/buggy prover cannot pad the proof with unrelated state. (See the - // `ShieldFromAssetLock` arm below for the full rationale.) + // Reconstruct the prove side's merged query — nullifier spend-status ∪ + // withdrawal document — and verify it strictly. See + // `verify_merged_query_strict` for why a single strict merged verify is + // sound and rejects proofs padded with extra subtree branches. let mut nf_query = grovedb::Query::new(); nf_query.insert_keys(nullifier_keys); let nullifier_pq = grovedb::PathQuery::new( @@ -1344,29 +1322,15 @@ impl Drive { grovedb::SizedQuery::new(nf_query, None, None), ); - let mut doc_pq = doc_query.construct_path_query(platform_version)?; - doc_pq.query.limit = None; + let doc_pq = doc_query.construct_path_query(platform_version)?; let document_path = doc_pq.path.clone(); - let mut merged_pq = grovedb::PathQuery::merge( - vec![&nullifier_pq, &doc_pq], - &platform_version.drive.grove_version, + let (root_hash, proved_key_values) = Self::verify_merged_query_strict( + proof, + vec![nullifier_pq, doc_pq], + platform_version, )?; - // `verify_query_with_absence_proof` (the STRICT verifier) requires a limit, but - // `PathQuery::merge` clears it. The merged query targets a fixed, tiny set of - // explicit keys ({nullifiers} ∪ {withdrawal document}), so set a limit that can - // never be exhausted by the legitimate result set; this guarantees every layer is - // fully traversed and every extra branch is caught by the succinctness check. - merged_pq.query.limit = Some(u16::MAX); - - let (root_hash, proved_key_values) = - grovedb::GroveDb::verify_query_with_absence_proof( - proof, - &merged_pq, - &platform_version.drive.grove_version, - )?; - // Partition the proved key/values by path: entries under the nullifiers tree are // spend statuses; the single entry under the withdrawal-document tree is the // proven document. @@ -1465,47 +1429,20 @@ impl Drive { match surplus_output { Some(surplus_address) => { - // The prove side merged the outpoint sub-query with the surplus-address - // balance sub-query into a SINGLE multi-root proof (clearing each sub-query - // limit before the merge, because `PathQuery::merge` rejects limited - // sub-queries). Reconstruct the byte-identical merged query here and verify - // it STRICTLY: the strict verifier accepts a proof that matches the merged - // query exactly and REJECTS any proof carrying extra branches, so a - // malicious/buggy prover cannot pad the proof with unrelated data. - let mut outpoint_pq = outpoint_pq; - outpoint_pq.query.limit = None; - - let mut address_pq = Drive::balances_for_clear_addresses_query( + // Reconstruct the prove side's merged query — asset-lock outpoint ∪ + // surplus-address balance — and verify it strictly. See + // `verify_merged_query_strict` for why a single strict merged verify + // is sound and rejects proofs padded with extra subtree branches. + let address_pq = Drive::balances_for_clear_addresses_query( std::iter::once(surplus_address), ); - address_pq.query.limit = None; - let mut merged_pq = grovedb::PathQuery::merge( - vec![&outpoint_pq, &address_pq], - &platform_version.drive.grove_version, + let (root_hash, proved_key_values) = Self::verify_merged_query_strict( + proof, + vec![outpoint_pq, address_pq], + platform_version, )?; - // `verify_query_with_absence_proof` (the STRICT verifier) requires a limit - // to be set, but `PathQuery::merge` leaves the merged limit at `None`. The - // merged query targets a fixed, tiny set of explicit keys ({outpoint} ∪ - // {surplus address}), so we set a limit that can never be exhausted by the - // legitimate result set. This is load-bearing for soundness: the succinctness - // check that rejects extra proof layers runs per-layer AFTER that layer's - // result loop, and the result loop only breaks early once the limit hits 0. - // A limit smaller than the real result count could break before every layer's - // succinctness check runs (falsely rejecting honest proofs); an unreachable - // limit guarantees every layer is fully traversed and every extra branch is - // caught. The limit does NOT relax the extra-data rejection — that is the - // succinctness check, which is independent of the limit value. - merged_pq.query.limit = Some(u16::MAX); - - let (root_hash, proved_key_values) = - grovedb::GroveDb::verify_query_with_absence_proof( - proof, - &merged_pq, - &platform_version.drive.grove_version, - )?; - // Partition the proved key/values: exactly one entry is the asset-lock // outpoint (36-byte key), every other entry is a surplus-address balance // (21-byte key). Anything else is a corrupted proof. @@ -1649,6 +1586,53 @@ impl Drive { } } } + + /// Reconstruct the prove side's merged multi-root query and verify it STRICTLY. + /// + /// The shielded prove paths merge several sub-queries into a SINGLE multi-root + /// proof. [`grovedb::PathQuery::merge`] rejects sub-queries that still carry a + /// limit, so every sub-query's limit is cleared here before the merge. The strict + /// [`grovedb::GroveDb::verify_query_with_absence_proof`] in turn *requires* a + /// limit, but it must never be exhausted by the legitimate result set: the + /// per-layer succinctness check that rejects extra proof branches runs AFTER that + /// layer's result loop, and the result loop only breaks early once the limit hits + /// 0 — so a limit smaller than the real result count could break before a layer's + /// succinctness check runs and falsely reject an honest proof. Every shielded + /// merged query targets a fixed, tiny set of explicit keys ({nullifiers} plus one + /// address/document/outpoint), so an unreachable `u16::MAX` limit is sound: it + /// guarantees every layer is fully traversed while the (limit-independent) + /// succinctness check still rejects any proof padded with extra subtree branches. + /// + /// Returns the proof root hash and every proved `(path, key, element)` trio, left + /// for the caller to partition against the sub-query paths. + #[allow(clippy::type_complexity)] + fn verify_merged_query_strict( + proof: &[u8], + mut sub_queries: Vec, + platform_version: &PlatformVersion, + ) -> Result< + ( + RootHash, + Vec, + ), + Error, + > { + for sub_query in &mut sub_queries { + sub_query.query.limit = None; + } + + let mut merged_pq = grovedb::PathQuery::merge( + sub_queries.iter().collect(), + &platform_version.drive.grove_version, + )?; + merged_pq.query.limit = Some(u16::MAX); + + Ok(grovedb::GroveDb::verify_query_with_absence_proof( + proof, + &merged_pq, + &platform_version.drive.grove_version, + )?) + } } #[cfg(feature = "server")] From fcfd1fe59675e4da63aa2625bd523d3e4098942b Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 8 Jun 2026 22:30:07 +0700 Subject: [PATCH 3/4] test(drive-abci): assert padded shielded proofs fail the Drive verifier entry point The unshield and shielded_withdrawal padded-proof tests only proved that the low-level GroveDb::verify_query_with_absence_proof primitive rejects a superset proof. That left the dispatch site this change actually rewrote untested: a regression that rebuilt the merged query differently, or fell back to the subset verifier inside the Unshield/ShieldedWithdrawal arm, would keep both tests green. Route padded_proof through Drive::verify_state_transition_was_executed_with_proof and assert it errors, locking the production code path into the tests. The shielded_withdrawal case loads the withdrawals system contract for the lookup fn, mirroring its positive round-trip test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../shielded_withdrawal/tests.rs | 29 +++++++++++++++++++ .../state_transitions/unshield/tests.rs | 19 ++++++++++++ 2 files changed, 48 insertions(+) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs index 3a42917939..7ecaff6c1d 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs @@ -1370,6 +1370,35 @@ mod tests { strict_result ); + // And the PRODUCTION entry point — the dispatch site this change actually + // rewrote — MUST reject the padded proof too. Asserting only against the GroveDB + // primitive above would stay green if the ShieldedWithdrawal arm regressed to + // rebuild a different merged query or fall back to the subset verifier; routing the + // padded proof through `Drive::verify_state_transition_was_executed_with_proof` + // locks the real code path into the test. + let withdrawals_data_contract = Arc::new( + load_system_data_contract(SystemDataContract::Withdrawals, platform_version) + .expect("should load withdrawals contract"), + ); + let production_result = Drive::verify_state_transition_was_executed_with_proof( + &transition, + &BlockInfo::default(), + &padded_proof, + &|id| { + if *id == withdrawals_contract::ID { + Ok(Some(Arc::clone(&withdrawals_data_contract))) + } else { + Ok(None) + } + }, + platform_version, + ); + assert!( + production_result.is_err(), + "production shielded withdrawal verifier must reject a padded proof, got {:?}", + production_result + ); + // Contrast: the SUBSET verifier (the looser primitive this change replaced) tolerates // the extra layer and ACCEPTS the same padded proof — the exact hole now closed. let subset_result = GroveDb::verify_subset_query_with_absence_proof( diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs index c1142691d2..cf86527d95 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs @@ -1180,6 +1180,25 @@ mod tests { strict_result ); + // And the PRODUCTION entry point — the dispatch site this change actually + // rewrote — MUST reject the padded proof too. Asserting only against the GroveDB + // primitive above would stay green if the Unshield arm regressed to rebuild a + // different merged query or fall back to the subset verifier; routing the padded + // proof through `Drive::verify_state_transition_was_executed_with_proof` locks the + // real code path into the test. + let production_result = Drive::verify_state_transition_was_executed_with_proof( + &transition, + &BlockInfo::default(), + &padded_proof, + &|_| Ok(None), + platform_version, + ); + assert!( + production_result.is_err(), + "production unshield verifier must reject a padded proof, got {:?}", + production_result + ); + // Contrast: the SUBSET verifier (the looser primitive this change replaced) tolerates // the extra layer and ACCEPTS the same padded proof — the exact hole now closed. let subset_result = GroveDb::verify_subset_query_with_absence_proof( From f98de59d52dac1bc97a41846bbc0a56418778998 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 8 Jun 2026 22:41:33 +0700 Subject: [PATCH 4/4] refactor(drive): reject duplicate output-address entry in Unshield proof verify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Unshield arm did balances.insert(address, ..) on each clear-address entry, which would silently last-write-wins if a second address entry ever appeared, while ShieldedWithdrawal explicitly returns CorruptedProof on a second document entry. Both sub-queries target a single key under a singleton subtree, so make the invariant uniform: Unshield now also rejects a second entry instead of overwriting. This guards a state the strict verifier already makes unreachable (the address sub-query carries exactly one key, so verify_query_with_absence_proof returns at most one address entry), so it is defense-in-depth for maintainer clarity, not a reachable bug fix — hence no test exercises the new branch. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v0/mod.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs b/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs index c316b8a6bd..72af4fbd4d 100644 --- a/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs +++ b/packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs @@ -1209,6 +1209,15 @@ impl Drive { }) .transpose()?; + // Mirror ShieldedWithdrawal's singleton-subtree invariant: the + // address sub-query targets exactly one key, so a second entry under + // the clear-address pool is a malformed proof, not last-write-wins. + if balances.contains_key(&address) { + return Err(Error::Proof(ProofError::CorruptedProof( + "unshield proof contained more than one output-address entry" + .to_string(), + ))); + } balances.insert(address, balance_info); } else { return Err(Error::Proof(ProofError::CorruptedProof(