security: add input validation guards across precompile, RPC, and pallets

client/rpc-v2/src/privacy.rs

@@ -32,7 +32,12 @@ use std::{marker::PhantomData, sync::Arc};
 // ============================================================================
 
 const PALLET: &[u8] = b"ShieldedPool";
-const TREE_DEPTH: usize = 20;
+
+/// Maximum number of Merkle leaves the RPC will load per request.
+/// Prevents DoS via unbounded O(n) storage reads on deep trees.
+/// At tree depth 20 the theoretical maximum is 2^20 ≈ 1M leaves;
+/// we cap far below that to keep RPC latency bounded.
+const MAX_RPC_LEAVES: u32 = 100_000;
 
 /// Builds `twox_128(pallet) ++ twox_128(item)` (32 bytes — `StorageValue` key).
 fn value_key(item: &[u8]) -> Vec<u8> {
@@ -168,6 +173,25 @@ fn pool_not_initialized() -> ErrorObject<'static> {
 	)
 }
 
+fn pool_is_empty() -> ErrorObject<'static> {
+	ErrorObject::owned(
+		ErrorCode::InternalError.code(),
+		"Shielded pool has no commitments",
+		None::<()>,
+	)
+}
+
+fn too_many_leaves(size: u32) -> ErrorObject<'static> {
+	ErrorObject::owned(
+		ErrorCode::InternalError.code(),
+		format!(
+			"tree_size {size} exceeds RPC limit {MAX_RPC_LEAVES}; \
+				use an archive node or paginated access"
+		),
+		None::<()>,
+	)
+}
+
 // ============================================================================
 // Storage helper — shared across all handler methods
 // ============================================================================
@@ -186,15 +210,15 @@ fn read_storage<B: BlockT, C: ScStorageProvider<B, BE>, BE: sc_client_api::Backe
 // Pure Merkle path builder — extracted for testability
 //
 // Mirrors `IncrementalMerkleTree::generate_proof` in the pallet exactly.
-// Returns a `TREE_DEPTH`-element sibling path (raw bytes).
+// Returns a `DEFAULT_TREE_DEPTH`-element sibling path (raw bytes).
 // ============================================================================
 
 fn build_merkle_path(leaves: &[[u8; 32]], leaf_index: usize) -> Vec<[u8; 32]> {
 	let mut current_level = leaves.to_vec();
-	let mut path = Vec::with_capacity(TREE_DEPTH);
+	let mut path = Vec::with_capacity(DEFAULT_TREE_DEPTH);
 	let mut target = leaf_index;
 
-	for level in 0..TREE_DEPTH {
+	for level in 0..DEFAULT_TREE_DEPTH {
 		// Pad odd levels with the canonical zero hash for this level.
 		if current_level.len() % 2 != 0 {
 			current_level.push(get_zero_hash_cached(level));
@@ -254,7 +278,10 @@ where
 		let tree_size = u32::decode(&mut &size_data[..]).map_err(internal_error)?;
 
 		if tree_size == 0 {
-			return Err(pool_not_initialized());
+			return Err(pool_is_empty());
+		}
+		if tree_size > MAX_RPC_LEAVES {
+			return Err(too_many_leaves(tree_size));
 		}
 		if leaf_index >= tree_size {
 			return Err(invalid_params(format!(
@@ -292,7 +319,7 @@ where
 			root: format!("0x{}", hex::encode(root.as_bytes())),
 			path,
 			leaf_index,
-			tree_depth: TREE_DEPTH as u32,
+			tree_depth: DEFAULT_TREE_DEPTH as u32,
 		})
 	}
 
@@ -316,7 +343,10 @@ where
 		let tree_size = u32::decode(&mut &size_data[..]).map_err(internal_error)?;
 
 		if tree_size == 0 {
-			return Err(pool_not_initialized());
+			return Err(pool_is_empty());
+		}
+		if tree_size > MAX_RPC_LEAVES {
+			return Err(too_many_leaves(tree_size));
 		}
 
 		// Load all leaves; find the matching commitment
@@ -358,7 +388,7 @@ where
 			root: format!("0x{}", hex::encode(root.as_bytes())),
 			path,
 			leaf_index,
-			tree_depth: TREE_DEPTH as u32,
+			tree_depth: DEFAULT_TREE_DEPTH as u32,
 		})
 	}
 
@@ -401,7 +431,7 @@ where
 		let commitment_count = u32::decode(&mut &size_data[..]).map_err(internal_error)?;
 
 		if commitment_count == 0 {
-			return Err(pool_not_initialized());
+			return Err(pool_is_empty());
 		}
 
 		// Next asset ID — drives the per-asset balance scan
@@ -550,8 +580,8 @@ mod tests {
 				let path = build_merkle_path(&leaves, 0);
 				assert_eq!(
 					path.len(),
-					TREE_DEPTH,
-					"path len must be {TREE_DEPTH} for n={n}"
+					DEFAULT_TREE_DEPTH,
+					"path len must be {DEFAULT_TREE_DEPTH} for n={n}"
 				);
 			}
 		}
@@ -613,7 +643,7 @@ mod tests {
 			// After level 0 the single leaf is hashed with zero_hash[0] to form the
 			// level-1 node. The sibling at level 1 must be zero_hash[1], and so on.
 			#[allow(clippy::needless_range_loop)]
-			for level in 1..TREE_DEPTH {
+			for level in 1..DEFAULT_TREE_DEPTH {
 				assert_eq!(path[level], get_zero_hash_cached(level));
 			}
 		}
@@ -868,4 +898,81 @@ mod tests {
 			assert_eq!(h256, H256::zero());
 		}
 	}
+
+	// -------------------------------------------------------------------------
+	// Error constructors
+	// -------------------------------------------------------------------------
+
+	mod error_constructors {
+		use super::*;
+		use jsonrpsee::types::error::ErrorCode;
+
+		#[test]
+		fn pool_not_initialized_uses_internal_error_code() {
+			let e = pool_not_initialized();
+			assert_eq!(e.code(), ErrorCode::InternalError.code());
+		}
+
+		#[test]
+		fn pool_is_empty_uses_internal_error_code() {
+			let e = pool_is_empty();
+			assert_eq!(e.code(), ErrorCode::InternalError.code());
+		}
+
+		#[test]
+		fn pool_is_empty_message_differs_from_not_initialized() {
+			let empty = pool_is_empty();
+			let uninit = pool_not_initialized();
+			// Must produce distinct messages so callers can distinguish the two states.
+			assert_ne!(empty.message(), uninit.message());
+		}
+
+		#[test]
+		fn too_many_leaves_includes_tree_size_in_message() {
+			let e = too_many_leaves(999_999);
+			assert!(
+				e.message().contains("999999"),
+				"message must contain the tree_size"
+			);
+		}
+
+		#[test]
+		fn too_many_leaves_includes_limit_in_message() {
+			let e = too_many_leaves(1);
+			assert!(
+				e.message().contains(&MAX_RPC_LEAVES.to_string()),
+				"message must contain MAX_RPC_LEAVES"
+			);
+		}
+
+		#[test]
+		fn too_many_leaves_uses_internal_error_code() {
+			let e = too_many_leaves(1);
+			assert_eq!(e.code(), ErrorCode::InternalError.code());
+		}
+	}
+
+	// -------------------------------------------------------------------------
+	// MAX_RPC_LEAVES constant sanity
+	// -------------------------------------------------------------------------
+
+	mod rpc_leaf_cap {
+		use super::*;
+
+		#[test]
+		fn max_rpc_leaves_is_below_tree_capacity() {
+			// Tree capacity = 2^DEFAULT_TREE_DEPTH. Cap must be strictly less
+			// to provide meaningful DoS protection.
+			let capacity: u64 = 1u64 << DEFAULT_TREE_DEPTH;
+			assert!(
+				(MAX_RPC_LEAVES as u64) < capacity,
+				"MAX_RPC_LEAVES {MAX_RPC_LEAVES} must be < tree capacity {capacity}"
+			);
+		}
+
+		#[test]
+		fn max_rpc_leaves_is_nonzero() {
+			const _: () = assert!(MAX_RPC_LEAVES > 0);
+		}
+	}
 }

frame/shielded-pool/src/operations/fees.rs

@@ -47,6 +47,8 @@ impl FeeOperation {
 
 		let amount_u128: u128 = amount.saturated_into();
 
+		ensure!(!amount.is_zero(), Error::<T>::InvalidAmount);
+
 		// amount must fit in u64 (circuit signal size)
 		ensure!(amount_u128 <= u64::MAX as u128, Error::<T>::InvalidAmount);
 
@@ -125,6 +127,8 @@ impl FeeOperation {
 
 		let amount_u128: u128 = amount.saturated_into();
 
+		ensure!(!amount.is_zero(), Error::<T>::InvalidAmount);
+
 		// Resolve the registered H160 for this validator.
 		let evm_address = T::Relayer::registered_evm_address(&validator)
 			.ok_or(Error::<T>::RelayerNotRegistered)?;
@@ -411,6 +415,32 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn claim_shielded_zero_amount_fails_with_invalid_amount() {
+		// amount == 0 must be rejected before the ZK check. A disclosure proof that
+		// encodes value=0 is cryptographically valid but inserts a worthless leaf into
+		// the Merkle tree — cheap spam that wastes tree capacity.
+		new_test_ext().execute_with(|| {
+			let validator: u64 = 1;
+			let asset_id = setup_asset();
+			mock_pending_fees_set(validator, asset_id, 500u128);
+
+			let commitment = make_commitment();
+			assert_noop!(
+				FeeOperation::claim_shielded::<Test>(
+					validator,
+					commitment,
+					0u128,
+					asset_id,
+					make_memo(),
+					make_proof(),
+					make_signals(&commitment, 0u128, asset_id),
+				),
+				crate::pallet::Error::<Test>::InvalidAmount
+			);
+		});
+	}
+
 	// ── ZK proof / public_signals validation ─────────────────────────────────
 
 	/// A proof of 128 zero bytes — the MockZkVerifier sentinel for "cryptographically
@@ -842,4 +872,23 @@ mod tests {
 			assert_eq!(mock_pending_fees_get(validator, asset_id), total - claim);
 		});
 	}
+
+	#[test]
+	fn claim_to_evm_zero_amount_fails() {
+		// amount == 0 must be rejected: a zero-value EVM claim is a no-op that wastes
+		// block space and emits a misleading event without moving any funds.
+		new_test_ext().execute_with(|| {
+			let validator: u64 = 1;
+			let asset_id = setup_asset();
+
+			mock_evm_address_set(validator, alice_evm());
+			mock_pending_fees_set(validator, asset_id, 500u128);
+			fund_pool(asset_id, 500u128);
+
+			assert_noop!(
+				FeeOperation::claim_to_evm::<Test>(validator, asset_id, 0u128),
+				crate::pallet::Error::<Test>::InvalidAmount
+			);
+		});
+	}
 }

frame/shielded-pool/src/operations/private_transfer.rs

@@ -24,6 +24,10 @@ impl PrivateTransferOperation {
 		fee: <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance,
 		relayer_evm: Option<sp_core::H160>,
 	) -> DispatchResult {
+		ensure!(
+			nullifiers.len() == commitments.len(),
+			Error::<T>::TooManyInputsOrOutputs
+		);
 		ensure!(
 			encrypted_memos.len() == commitments.len(),
 			Error::<T>::MemoCommitmentMismatch
@@ -572,4 +576,28 @@ mod tests {
 			);
 		});
 	}
+
+	#[test]
+	fn execute_nullifier_commitment_count_mismatch_fails() {
+		// 1 nullifier but 2 output commitments: structurally inconsistent with the
+		// fixed 2-in/2-out circuit. Must be rejected by the pallet before the ZK check
+		// so that benchmark mode (no verifier) cannot break value conservation.
+		new_test_ext().execute_with(|| {
+			MerkleRepository::add_historic_poseidon_root::<Test>(KNOWN_ROOT);
+
+			assert_noop!(
+				PrivateTransferOperation::execute::<Test>(
+					proof(),
+					KNOWN_ROOT,
+					nullifiers_of(&[0xF1]),        // 1 nullifier
+					commitments_of(&[0xF2, 0xF3]), // 2 commitments
+					memos_of(2),
+					0u32,
+					0u128,
+					None,
+				),
+				Error::<Test>::TooManyInputsOrOutputs
+			);
+		});
+	}
 }

frame/shielded-pool/src/operations/shield.rs

@@ -31,6 +31,11 @@ impl ShieldOperation {
 			Error::<T>::InvalidMemoSize
 		);
 
+		ensure!(
+			!CommitmentRepository::exists::<T>(&commitment),
+			Error::<T>::CommitmentAlreadyExists
+		);
+
 		T::Currency::transfer(
 			&depositor,
 			&Pallet::<T>::pool_account_id(),
@@ -318,4 +323,29 @@ mod tests {
 			assert!(!ShieldOperation::is_asset_verified::<Test>(id2));
 		});
 	}
+
+	#[test]
+	fn execute_duplicate_commitment_fails() {
+		// A second shield call with the same commitment bytes must be rejected to prevent
+		// Merkle-tree spam: an attacker could flood the tree with duplicate leaves,
+		// consuming tree capacity while the associated notes remain unspendable (a single
+		// nullifier can only be used once).
+		new_test_ext().execute_with(|| {
+			let asset_id = setup_asset();
+			let c = commitment(0xDE);
+
+			assert_ok!(ShieldOperation::execute::<Test>(
+				1u64,
+				asset_id,
+				500u128,
+				c,
+				memo_valid(),
+			));
+
+			assert_noop!(
+				ShieldOperation::execute::<Test>(1u64, asset_id, 500u128, c, memo_valid()),
+				crate::pallet::Error::<Test>::CommitmentAlreadyExists
+			);
+		});
+	}
 }