fix(cnight-observation): complete CIP-19 validation for Cardano reward addresses (#498)

changes/node/changed/cnight-observation-cip19-reward-address-validation.md

@@ -0,0 +1,28 @@
+#audit #hardening
+# Complete CIP-19 validation for Cardano reward addresses
+
+Consolidate Cardano reward-address validation into a single validated
+constructor `CardanoRewardAddressBytes::try_new(bytes, expected_network)` on
+the cNIGHT observation primitive, and route every entry point through it so a
+malformed, wrong-length, wrong-address-type, or wrong-network reward address is
+rejected at the trust boundary with a specific, named error instead of being
+stored and surfacing failures obscurely later. The constructor checks length
+first, then the CIP-19 header's type nibble (14 or 15) and network nibble, using
+`no_std` bit arithmetic; the length-only conversion is retained for test and
+benchmark callers.
+
+The chain follower's four reward-address construction sites (registration,
+deregistration, asset-create owner, asset-spend owner) now share one helper
+wrapping the validated constructor and skip a bad record gracefully rather than
+panicking, the previously scattered ad-hoc parsing collapses to a single error
+type, and the dead network-error variant is removed. Genesis build validates
+deserialized reward-address keys against the address type and the network
+derived from the mapping-validator address Bech32 prefix, failing fast on an
+invalid key. The mapping-validator address, a different Cardano address
+category, keeps its own structural validator returning a specific error and is
+applied on both the genesis build and the `set_mapping_validator_contract_address`
+extrinsic.
+
+PR: https://github.com/midnightntwrk/midnight-node/pull/1817
+Issue: https://github.com/shieldedtech/shielded-security-engineering/issues/498
+JIRA: https://input-output.atlassian.net/browse/PM-20267

pallets/cnight-observation/src/config.rs

@@ -70,8 +70,11 @@ mod mappings_serde {
 					access.next_entry::<String, Vec<MappingEntryGenesis>>()?
 				{
 					let bytes: Vec<u8> = hex::decode(&key).map_err(serde::de::Error::custom)?;
+					let byte_len = bytes.len();
 					let addr = CardanoRewardAddressBytes::try_from(bytes).map_err(|_| {
-						serde::de::Error::custom("invalid CardanoRewardAddressBytes length")
+						serde::de::Error::custom(alloc::format!(
+							"invalid CardanoRewardAddressBytes length: expected 29 bytes, found {byte_len}"
+						))
 					})?;
 					map.insert(addr, value);
 				}

pallets/cnight-observation/src/lib.rs

@@ -113,6 +113,27 @@ pub mod pallet {
 
 	pub type BoundedCardanoAddress = BoundedVec<u8, ConstU32<CARDANO_BECH32_ADDRESS_MAX_LENGTH>>;
 
+	/// CIP-19 network id for Cardano mainnet (reward-address network nibble).
+	const CARDANO_NETWORK_MAINNET: u8 = 1;
+	/// CIP-19 network id for Cardano testnets (preview / preprod / local).
+	const CARDANO_NETWORK_TESTNET: u8 = 0;
+
+	/// Derive the expected CIP-19 network id from a Cardano Bech32 address's human-
+	/// readable prefix: `addr_test...` (and `stake_test...`) are testnet, `addr...`
+	/// (and `stake...`) are mainnet. Returns `None` when the prefix is neither, so
+	/// genesis reward-address network validation is skipped rather than enforced
+	/// against a guessed network. The `_test` prefix must be checked first because
+	/// `addr` / `stake` are prefixes of `addr_test` / `stake_test`.
+	fn expected_network_from_bech32_hrp(address: &str) -> Option<u8> {
+		if address.starts_with("addr_test") || address.starts_with("stake_test") {
+			Some(CARDANO_NETWORK_TESTNET)
+		} else if address.starts_with("addr") || address.starts_with("stake") {
+			Some(CARDANO_NETWORK_MAINNET)
+		} else {
+			None
+		}
+	}
+
 	#[derive(
 		Debug,
 		Clone,
@@ -177,6 +198,8 @@ pub mod pallet {
 	pub enum Error<T> {
 		/// A Cardano Wallet address was sent, but was longer than expected
 		MaxCardanoAddrLengthExceeded,
+		/// The mapping-validator address is not a well-formed Cardano Bech32 address
+		InvalidMappingValidatorAddress,
 		/// A Cardano asset name contained non-ASCII bytes
 		NonAsciiAssetName,
 		/// Only one inherent is allowed per block
@@ -310,20 +333,16 @@ pub mod pallet {
 			// operator edits) and reads the cap from the destination BoundedVec type, so a
 			// startup-failure log points directly at the offending field.
 			MainChainMappingValidatorAddress::<T>::set(
-				self.config
-					.addresses
-					.mapping_validator_address
-					.as_bytes()
-					.to_vec()
-					.try_into()
-					.unwrap_or_else(|v: Vec<u8>| {
-						panic!(
-							"genesis: cNightObservation.config.addresses.mapping_validator_address \
-							 length {} bytes exceeds maximum {}",
-							v.len(),
-							BoundedCardanoAddress::bound(),
-						)
-					}),
+				Pallet::<T>::validate_mapping_validator_address(
+					self.config.addresses.mapping_validator_address.as_bytes().to_vec(),
+				)
+				.unwrap_or_else(|e| {
+					panic!(
+						"genesis: cNightObservation.config.addresses.mapping_validator_address \
+						 is invalid: {e:?} (max length {})",
+						BoundedCardanoAddress::bound(),
+					)
+				}),
 			);
 
 			CNightIdentifier::<T>::set((
@@ -370,6 +389,27 @@ pub mod pallet {
 					}),
 			);
 
+			// The serde deserializer for `mappings` can only check key length (it has
+			// no network context), so enforce the CIP-19 address type + network id
+			// here, where the expected network is derivable from the Bech32 HRP of
+			// the mapping-validator address (`addr...` = mainnet, `addr_test...` =
+			// testnet). Genesis fail-fast: an invalid reward-address key panics
+			// rather than starting the chain with an unbridgeable mapping.
+			let expected_network =
+				expected_network_from_bech32_hrp(&self.config.addresses.mapping_validator_address);
+			for addr in self.config.mappings.keys() {
+				if let Some(expected_network) = expected_network {
+					if let Err(e) =
+						CardanoRewardAddressBytes::try_new(addr.0.to_vec(), expected_network)
+					{
+						panic!(
+							"genesis: cNightObservation.config.mappings contains an invalid \
+							 reward-address key: {e}"
+						);
+					}
+				}
+			}
+
 			for (addr, entries) in &self.config.mappings {
 				for entry in entries {
 					Mapping::<T>::insert(
@@ -431,6 +471,22 @@ pub mod pallet {
 	}
 
 	impl<T: Config> Pallet<T> {
+		/// Structural validator for the mapping-validator address, which is a Shelley
+		/// payment address rather than a reward address and so cannot use the reward
+		/// `try_new`. Checks the bytes are a UTF-8 Cardano Bech32 address within the
+		/// bounded length, returning a specific error to distinguish malformed from
+		/// merely over-long.
+		fn validate_mapping_validator_address(
+			address: Vec<u8>,
+		) -> Result<BoundedCardanoAddress, Error<T>> {
+			let text = core::str::from_utf8(&address)
+				.map_err(|_| Error::<T>::InvalidMappingValidatorAddress)?;
+			if expected_network_from_bech32_hrp(text).is_none() {
+				return Err(Error::<T>::InvalidMappingValidatorAddress);
+			}
+			address.try_into().map_err(|_| Error::<T>::MaxCardanoAddrLengthExceeded)
+		}
+
 		fn get_data_from_inherent_data(
 			data: &InherentData,
 		) -> Result<Option<MidnightObservationTokenMovement>, InherentError> {
@@ -765,12 +821,9 @@ pub mod pallet {
 			address: Vec<u8>,
 		) -> DispatchResult {
 			ensure_root(origin)?;
-			MainChainMappingValidatorAddress::<T>::set(
-				address
-					.clone()
-					.try_into()
-					.map_err(|_| Error::<T>::MaxCardanoAddrLengthExceeded)?,
-			);
+			MainChainMappingValidatorAddress::<T>::set(Self::validate_mapping_validator_address(
+				address,
+			)?);
 
 			Ok(())
 		}

pallets/cnight-observation/tests/tests.rs

@@ -40,6 +40,7 @@ use pallet_cnight_observation_mock::mock::{
 };
 use rand::prelude::*;
 use sidechain_domain::{McBlockHash, McTxHash, UtxoId};
+use std::collections::BTreeMap;
 use test_log::test;
 
 fn create_inherent(
@@ -1697,50 +1698,153 @@ fn is_inherent_required_with_malformed_data_returns_error() {
 	});
 }
 
-// One representative regression guard for the operator-facing panic shape in
-// `BuildGenesisConfig::build`. The four call sites use a byte-identical format
-// string differing only in the dotted field path and the destination bound;
-// `cnight_policy_id` is not exercised because its source type `[u8; 28]` makes
-// the panic at that site unreachable from chain-spec deserialization.
-#[test]
-fn build_panics_with_field_path_and_bound_when_mapping_validator_address_too_long() {
-	let over_length = "a".repeat(CARDANO_BECH32_ADDRESS_MAX_LENGTH as usize + 1);
-	let genesis = pallet_cnight_observation::GenesisConfig::<Test> {
+// Build a genesis config with the given mapping-validator address and reward-key
+// mappings, defaulting the remaining address fields.
+fn genesis_with(
+	mapping_validator_address: &str,
+	mappings: BTreeMap<CardanoRewardAddressBytes, Vec<config::MappingEntryGenesis>>,
+) -> pallet_cnight_observation::GenesisConfig<Test> {
+	pallet_cnight_observation::GenesisConfig::<Test> {
 		config: pallet_cnight_observation::config::CNightGenesis {
 			addresses: CNightAddresses {
-				mapping_validator_address: over_length,
+				mapping_validator_address: mapping_validator_address.to_string(),
 				auth_token_asset_name: String::new(),
 				cnight_policy_id: [0u8; 28],
 				cnight_asset_name: String::new(),
 			},
+			mappings,
 			..Default::default()
 		},
 		_marker: Default::default(),
-	};
+	}
+}
 
+// Run `build()` and return the panic message, failing if the build did not panic.
+fn build_panic_message(genesis: pallet_cnight_observation::GenesisConfig<Test>) -> String {
 	let payload = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
 		sp_io::TestExternalities::default().execute_with(|| {
 			genesis.build();
 		});
 	}))
-	.expect_err("genesis build must panic on over-length mapping_validator_address");
+	.expect_err("genesis build must panic");
 
-	let msg = payload
+	payload
 		.downcast_ref::<String>()
 		.cloned()
 		.or_else(|| payload.downcast_ref::<&'static str>().map(|s| s.to_string()))
-		.expect("panic payload should be String or &'static str");
+		.expect("panic payload should be String or &'static str")
+}
+
+// Regression guard for the operator-facing over-length panic shape in
+// `BuildGenesisConfig::build`. The address has a valid `addr_test` prefix so it
+// passes the HRP check and reaches the length branch, which is the path that
+// names the destination bound.
+#[test]
+fn build_panics_with_field_path_and_bound_when_mapping_validator_address_too_long() {
+	let over_length =
+		format!("addr_test1{}", "q".repeat(CARDANO_BECH32_ADDRESS_MAX_LENGTH as usize));
+	let msg = build_panic_message(genesis_with(&over_length, BTreeMap::new()));
 
 	assert!(
 		msg.contains("cNightObservation.config.addresses.mapping_validator_address"),
 		"panic must name the chain-spec field path; got: {msg}"
 	);
 	assert!(
-		msg.contains(&format!("maximum {CARDANO_BECH32_ADDRESS_MAX_LENGTH}")),
+		msg.contains(&format!("max length {CARDANO_BECH32_ADDRESS_MAX_LENGTH}")),
 		"panic must name the destination bound; got: {msg}"
 	);
 }
 
+// A mapping-validator address with an unrecognized Bech32 prefix is rejected as
+// structurally invalid before the length branch, so the panic carries the
+// specific `InvalidMappingValidatorAddress` error rather than a length message.
+#[test]
+fn build_panics_with_invalid_error_when_mapping_validator_address_has_bad_prefix() {
+	let msg = build_panic_message(genesis_with("not_a_cardano_address", BTreeMap::new()));
+
+	assert!(
+		msg.contains("cNightObservation.config.addresses.mapping_validator_address"),
+		"panic must name the chain-spec field path; got: {msg}"
+	);
+	assert!(
+		msg.contains("InvalidMappingValidatorAddress"),
+		"panic must name the structural-validation error; got: {msg}"
+	);
+}
+
+// Genesis fail-fast on a reward-address mapping key whose network nibble does not
+// match the network derived from the mapping-validator address. The validator
+// prefix `addr_test` fixes the expected network to testnet (0), while the key is
+// a valid type-14 reward address on mainnet (network nibble 1), so the build
+// must panic naming the invalid key.
+#[test]
+fn build_panics_when_mapping_key_has_wrong_network() {
+	// header: upper nibble 14 (reward key hash), lower nibble 1 (mainnet).
+	let mut key_bytes = vec![(14u8 << 4) | 1u8];
+	key_bytes.extend(std::iter::repeat(0u8).take(28));
+	let key = CardanoRewardAddressBytes(key_bytes.try_into().unwrap());
+	let mut mappings = BTreeMap::new();
+	mappings.insert(key, Vec::new());
+
+	let msg = build_panic_message(genesis_with(
+		"addr_test1wplxjzranravtp574s2wz00md7vz9rzpucu252je68u9a8qzjheng",
+		mappings,
+	));
+
+	assert!(
+		msg.contains("invalid") && msg.contains("reward-address key"),
+		"panic must name the invalid reward-address key; got: {msg}"
+	);
+}
+
+// The `set_mapping_validator_contract_address` extrinsic rejects a bad-prefix
+// address with the specific structural error and leaves storage unchanged.
+#[test]
+fn set_mapping_validator_contract_address_rejects_bad_prefix_without_storing() {
+	new_test_ext().execute_with(|| {
+		let before = MainChainMappingValidatorAddress::<Test>::get();
+
+		assert_noop!(
+			CNightObservation::set_mapping_validator_contract_address(
+				RawOrigin::Root.into(),
+				b"not_a_cardano_address".to_vec(),
+			),
+			Error::<Test>::InvalidMappingValidatorAddress
+		);
+
+		assert_eq!(
+			MainChainMappingValidatorAddress::<Test>::get(),
+			before,
+			"rejected address must not be stored"
+		);
+	});
+}
+
+// A valid-prefix address over the bounded length is rejected with the length
+// error rather than the structural error, and is not stored.
+#[test]
+fn set_mapping_validator_contract_address_rejects_over_length_without_storing() {
+	new_test_ext().execute_with(|| {
+		let before = MainChainMappingValidatorAddress::<Test>::get();
+		let over_length =
+			format!("addr_test1{}", "q".repeat(CARDANO_BECH32_ADDRESS_MAX_LENGTH as usize));
+
+		assert_noop!(
+			CNightObservation::set_mapping_validator_contract_address(
+				RawOrigin::Root.into(),
+				over_length.into_bytes(),
+			),
+			Error::<Test>::MaxCardanoAddrLengthExceeded
+		);
+
+		assert_eq!(
+			MainChainMappingValidatorAddress::<Test>::get(),
+			before,
+			"rejected address must not be stored"
+		);
+	});
+}
+
 type BoundedAssetName = BoundedVec<u8, ConstU32<CARDANO_ASSET_NAME_MAX_LENGTH>>;
 
 fn bounded_asset_name(bytes: &[u8]) -> BoundedAssetName {