From f8207587d357bd6c7bbd872e4611b90aed42ada2 Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 09:12:34 +0100 Subject: [PATCH 1/7] chore(cnight-observation): scaffold CIP-19 reward address validation work package Seed commit for issue #498 work package. Implementation to follow. Signed-off-by: Mike Clay From 767e07aa3152c4ca61273c7447378cafef07d6f1 Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 10:20:43 +0100 Subject: [PATCH 2/7] fix(cnight-observation): complete CIP-19 validation for Cardano reward addresses Reward addresses were accepted with only a length check; the four follower construction sites bypassed even that via to_bytes().try_into().unwrap(), the genesis path was length-only, and nothing validated the CIP-19 address-type or network-id nibble. A wrong-length, wrong-type, wrong-network, or malformed reward address could be accepted and stored. Add a single validated constructor CardanoRewardAddressBytes::try_new(bytes, expected_network) on the no_std primitive, with CardanoRewardAddressError carrying a distinct variant per rejection case (WrongLength, WrongAddressType, WrongNetwork). Checks run length-first, then the type nibble (14/15), then the network nibble, using plain bit arithmetic. Route every production path through it: a shared decode_reward_address helper wraps try_new at all four follower sites (log-and-skip on rejection, matching the existing resilience pattern) and the dead CardanoNetworkError variant is replaced by InvalidRewardAddress. Genesis validates each mappings key against type + network, deriving the expected network from the mapping-validator Bech32 prefix. The mapping-validator address (a Shelley script/enterprise address, not a reward address) gets a sibling structural validator returning a specific InvalidMappingValidatorAddress error rather than the generic length error, used by both the extrinsic and the genesis build. Add six unit tests on try_new covering wrong length, wrong type, wrong network, a malformed header, and valid type-14/type-15 round-trips. Issue: https://github.com/shieldedtech/shielded-security-engineering/issues/498 Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Mike Clay --- pallets/cnight-observation/src/config.rs | 11 +- pallets/cnight-observation/src/lib.rs | 100 ++++++++-- primitives/cnight-observation/src/lib.rs | 174 ++++++++++++++++++ .../src/data_source/cnight_observation.rs | 73 ++++++-- 4 files changed, 326 insertions(+), 32 deletions(-) diff --git a/pallets/cnight-observation/src/config.rs b/pallets/cnight-observation/src/config.rs index 3220becc5..61d1c5dfe 100644 --- a/pallets/cnight-observation/src/config.rs +++ b/pallets/cnight-observation/src/config.rs @@ -70,8 +70,17 @@ mod mappings_serde { access.next_entry::>()? { let bytes: Vec = hex::decode(&key).map_err(serde::de::Error::custom)?; + // Length-only check here: serde deserialization has no access to the + // expected network id, so the CIP-19 type + network nibbles are + // validated later in the pallet's genesis `build`, where the network + // is derivable from the mapping-validator address. The message names + // the length so a length failure is not misattributed to a header + // problem. + 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); } diff --git a/pallets/cnight-observation/src/lib.rs b/pallets/cnight-observation/src/lib.rs index 7e40033f8..c91cac7b6 100644 --- a/pallets/cnight-observation/src/lib.rs +++ b/pallets/cnight-observation/src/lib.rs @@ -113,6 +113,27 @@ pub mod pallet { pub type BoundedCardanoAddress = BoundedVec>; + /// 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 { + 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,9 @@ pub mod pallet { pub enum Error { /// A Cardano Wallet address was sent, but was longer than expected MaxCardanoAddrLengthExceeded, + /// The mapping-validator address is not a well-formed Cardano Bech32 + /// address (wrong human-readable prefix, empty, or over the length bound) + InvalidMappingValidatorAddress, /// A Cardano asset name contained non-ASCII bytes NonAsciiAssetName, /// Only one inherent is allowed per block @@ -310,20 +334,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::::set( - self.config - .addresses - .mapping_validator_address - .as_bytes() - .to_vec() - .try_into() - .unwrap_or_else(|v: Vec| { - panic!( - "genesis: cNightObservation.config.addresses.mapping_validator_address \ - length {} bytes exceeds maximum {}", - v.len(), - BoundedCardanoAddress::bound(), - ) - }), + Self::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::::set(( @@ -370,6 +390,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::::insert( @@ -431,6 +472,28 @@ pub mod pallet { } impl Pallet { + /// Sibling of the reward-address `try_new` for the mapping-validator address. + /// + /// The mapping-validator address is a Shelley script/enterprise (payment) + /// address, not a reward address — CIP-19 gives it a different address-type + /// category, so it must NOT go through the reward `try_new` (which would + /// reject it on the type nibble). Full Bech32/CIP-19 decoding belongs in the + /// follower, not the pallet, so this validates structurally: the bytes must + /// be a non-empty, valid UTF-8 Cardano Bech32 address (`addr` / `addr_test` + /// prefix) within the bounded length. It returns a specific error rather than + /// the generic length error so a malformed address is distinguishable from a + /// merely over-long one. + fn validate_mapping_validator_address( + address: Vec, + ) -> Result> { + let text = core::str::from_utf8(&address) + .map_err(|_| Error::::InvalidMappingValidatorAddress)?; + if expected_network_from_bech32_hrp(text).is_none() { + return Err(Error::::InvalidMappingValidatorAddress); + } + address.try_into().map_err(|_| Error::::MaxCardanoAddrLengthExceeded) + } + fn get_data_from_inherent_data( data: &InherentData, ) -> Result, InherentError> { @@ -765,12 +828,9 @@ pub mod pallet { address: Vec, ) -> DispatchResult { ensure_root(origin)?; - MainChainMappingValidatorAddress::::set( - address - .clone() - .try_into() - .map_err(|_| Error::::MaxCardanoAddrLengthExceeded)?, - ); + MainChainMappingValidatorAddress::::set(Self::validate_mapping_validator_address( + address, + )?); Ok(()) } diff --git a/primitives/cnight-observation/src/lib.rs b/primitives/cnight-observation/src/lib.rs index ce1b02511..ba349f538 100644 --- a/primitives/cnight-observation/src/lib.rs +++ b/primitives/cnight-observation/src/lib.rs @@ -61,6 +61,88 @@ pub struct CardanoRewardAddressBytes( #[serde(with = "hex")] pub [u8; CARDANO_REWARD_ADDRESS_LENGTH], ); +/// The two CIP-19 address-type nibbles that denote a reward (stake) account: +/// 14 = reward account keyed by a stake key hash, 15 = reward account keyed by a +/// script hash. Any other upper nibble is a different address category (Shelley +/// base/pointer/enterprise or Byron) and is not a reward address. +const CARDANO_REWARD_ADDRESS_TYPE_KEY_HASH: u8 = 14; +const CARDANO_REWARD_ADDRESS_TYPE_SCRIPT_HASH: u8 = 15; + +/// Why a byte string was rejected as a CIP-19 Cardano reward address. +/// +/// Each variant maps to exactly one of the checks [`CardanoRewardAddressBytes::try_new`] +/// runs, so a rejection points at the specific header property that failed rather +/// than collapsing every failure into a single opaque error. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CardanoRewardAddressError { + /// The input was not the fixed CIP-19 reward-address length of 29 bytes + /// (1 header byte + 28 credential bytes). + WrongLength { found: usize }, + /// The header's upper nibble is not a reward-address type (14 or 15), so the + /// bytes describe a different Cardano address category. + WrongAddressType { found: u8 }, + /// The header's lower nibble does not match the network the chain expects + /// (testnet = 0, mainnet = 1). + WrongNetwork { expected: u8, found: u8 }, +} + +impl core::fmt::Display for CardanoRewardAddressError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + CardanoRewardAddressError::WrongLength { found } => write!( + f, + "invalid Cardano reward address length: expected {CARDANO_REWARD_ADDRESS_LENGTH} bytes, found {found}" + ), + CardanoRewardAddressError::WrongAddressType { found } => write!( + f, + "invalid Cardano reward address type: header nibble {found} is not a reward account (expected 14 or 15)" + ), + CardanoRewardAddressError::WrongNetwork { expected, found } => write!( + f, + "invalid Cardano reward address network: expected network id {expected}, found {found}" + ), + } + } +} + +impl CardanoRewardAddressBytes { + /// Validated CIP-19 constructor: the single trust-boundary entry point that + /// asserts a byte string is a well-formed reward address for `expected_network`. + /// + /// Checks run length-first so that reading the header byte is only ever done on + /// a 29-byte input, then the header's two nibbles: the upper nibble must be a + /// reward-account type (14 or 15) and the lower nibble must equal + /// `expected_network`. Uses plain slice/bit arithmetic so it compiles under + /// `no_std`. + pub fn try_new( + bytes: Vec, + expected_network: u8, + ) -> Result { + if bytes.len() != CARDANO_REWARD_ADDRESS_LENGTH { + return Err(CardanoRewardAddressError::WrongLength { found: bytes.len() }); + } + + let header = bytes[0]; + let address_type = header >> 4; + if address_type != CARDANO_REWARD_ADDRESS_TYPE_KEY_HASH + && address_type != CARDANO_REWARD_ADDRESS_TYPE_SCRIPT_HASH + { + return Err(CardanoRewardAddressError::WrongAddressType { found: address_type }); + } + + let network = header & 0x0F; + if network != expected_network { + return Err(CardanoRewardAddressError::WrongNetwork { + expected: expected_network, + found: network, + }); + } + + // Length is confirmed to be exactly 29, so this conversion cannot fail. + Ok(Self(bytes.try_into().expect("length checked to be 29 above"))) + } +} + impl TryFrom> for CardanoRewardAddressBytes { type Error = <[u8; CARDANO_REWARD_ADDRESS_LENGTH] as TryFrom>>::Error; @@ -445,3 +527,95 @@ decl_runtime_apis! { fn get_utxo_capacity_per_block() -> u32; } } + +#[cfg(test)] +mod tests { + use super::*; + + const MAINNET: u8 = 1; + const TESTNET: u8 = 0; + + /// Build a 29-byte reward address with the given header byte followed by a + /// deterministic 28-byte credential. + fn reward_address_with_header(header: u8) -> Vec { + let mut bytes = Vec::with_capacity(CARDANO_REWARD_ADDRESS_LENGTH); + bytes.push(header); + bytes.extend((0..28u8).map(|i| i.wrapping_add(1))); + bytes + } + + #[test] + fn try_new_rejects_wrong_length() { + // 28 bytes (one short) and 30 bytes (one long) both fail on length before + // the header is ever inspected. + let short = vec![0u8; CARDANO_REWARD_ADDRESS_LENGTH - 1]; + assert_eq!( + CardanoRewardAddressBytes::try_new(short, MAINNET), + Err(CardanoRewardAddressError::WrongLength { found: 28 }) + ); + + let long = vec![0u8; CARDANO_REWARD_ADDRESS_LENGTH + 1]; + assert_eq!( + CardanoRewardAddressBytes::try_new(long, MAINNET), + Err(CardanoRewardAddressError::WrongLength { found: 30 }) + ); + } + + #[test] + fn try_new_rejects_wrong_address_type() { + // Type nibble 0 (Shelley base) with a mainnet network nibble: correct + // length and network, but not a reward-account type. + let header = (0 << 4) | MAINNET; + let bytes = reward_address_with_header(header); + assert_eq!( + CardanoRewardAddressBytes::try_new(bytes, MAINNET), + Err(CardanoRewardAddressError::WrongAddressType { found: 0 }) + ); + } + + #[test] + fn try_new_rejects_wrong_network() { + // Valid reward type (14) but the network nibble is mainnet while the chain + // expects testnet. + let header = (CARDANO_REWARD_ADDRESS_TYPE_KEY_HASH << 4) | MAINNET; + let bytes = reward_address_with_header(header); + assert_eq!( + CardanoRewardAddressBytes::try_new(bytes, TESTNET), + Err(CardanoRewardAddressError::WrongNetwork { expected: TESTNET, found: MAINNET }) + ); + } + + #[test] + fn try_new_rejects_malformed_header() { + // A 29-byte blob whose header is neither a reward type nor the expected + // network. The type check runs first, so the specific variant is + // WrongAddressType (type nibble 8 = Byron), matching PL-2: "malformed" is + // surfaced as whichever nibble check fails first. + let header = (8 << 4) | 0x0F; + let bytes = reward_address_with_header(header); + assert_eq!( + CardanoRewardAddressBytes::try_new(bytes, MAINNET), + Err(CardanoRewardAddressError::WrongAddressType { found: 8 }) + ); + } + + #[test] + fn try_new_accepts_valid_key_hash_reward_address() { + // Type 14 (stake key hash) for mainnet round-trips to the exact bytes. + let header = (CARDANO_REWARD_ADDRESS_TYPE_KEY_HASH << 4) | MAINNET; + let bytes = reward_address_with_header(header); + let addr = CardanoRewardAddressBytes::try_new(bytes.clone(), MAINNET) + .expect("valid type-14 mainnet reward address"); + assert_eq!(addr.0.to_vec(), bytes); + } + + #[test] + fn try_new_accepts_valid_script_hash_reward_address() { + // Type 15 (script hash) for testnet round-trips to the exact bytes. + let header = (CARDANO_REWARD_ADDRESS_TYPE_SCRIPT_HASH << 4) | TESTNET; + let bytes = reward_address_with_header(header); + let addr = CardanoRewardAddressBytes::try_new(bytes.clone(), TESTNET) + .expect("valid type-15 testnet reward address"); + assert_eq!(addr.0.to_vec(), bytes); + } +} diff --git a/primitives/mainchain-follower/src/data_source/cnight_observation.rs b/primitives/mainchain-follower/src/data_source/cnight_observation.rs index fce856690..7c8f5db6a 100644 --- a/primitives/mainchain-follower/src/data_source/cnight_observation.rs +++ b/primitives/mainchain-follower/src/data_source/cnight_observation.rs @@ -24,7 +24,8 @@ use cardano_serialization_lib::{ ScriptHash, }; use midnight_primitives_cnight_observation::{ - CNightAddresses, CardanoPosition, CardanoRewardAddressBytes, DustPublicKeyBytes, ObservedUtxos, + CNightAddresses, CardanoPosition, CardanoRewardAddressBytes, CardanoRewardAddressError, + DustPublicKeyBytes, ObservedUtxos, }; use sidechain_domain::{McBlockHash, McBlockNumber, McTxHash, McTxIndexInBlock, TX_HASH_SIZE}; pub use sqlx::PgPool; @@ -62,8 +63,8 @@ pub enum MidnightCNightObservationDataSourceError { MissingBlockReference(McBlockHash), #[error("Error querying database: {0}")] DBQueryError(#[from] sqlx::error::Error), - #[error("Error extracting network id from Cardano address")] - CardanoNetworkError(String), + #[error("Invalid Cardano reward address: {0}")] + InvalidRewardAddress(CardanoRewardAddressError), #[error("Invalid value for mapping validator address")] MappingValidatorInvalidAddress(String), } @@ -84,6 +85,22 @@ pub enum RegistrationDatumDecodeError { DustAddressInvalidLength(usize), } +/// Validate the bytes emitted by `cardano-serialization-lib`'s `RewardAddress` +/// against CIP-19 before wrapping them, replacing the previous +/// `to_bytes().try_into().unwrap()` shortcut at every follower construction site. +/// +/// `cardano_network` is the network the follower is syncing against (derived once +/// in `bulk_pull`); it is the value the reward address's network nibble must match. +/// Routing all four sites through this one helper means every reward address the +/// follower stores has passed the same length + type + network checks. +fn decode_reward_address( + reward_address_bytes: Vec, + cardano_network: u8, +) -> Result { + CardanoRewardAddressBytes::try_new(reward_address_bytes, cardano_network) + .map_err(MidnightCNightObservationDataSourceError::InvalidRewardAddress) +} + pub struct MidnightCNightObservationDataSourceImpl { pub pool: PgPool, pub metrics_opt: Option, @@ -219,13 +236,21 @@ impl MidnightCNightObservationDataSourceImpl { }; let reward_address = RewardAddress::new(cardano_network, &credential); - // Unwrap here is OK - we know the reward_address is always 29 bytes - let cardano_address = reward_address.to_address().to_bytes().try_into().unwrap(); + let cardano_reward_address = match decode_reward_address( + reward_address.to_address().to_bytes(), + cardano_network, + ) { + Ok(addr) => addr, + Err(e) => { + log::error!("Rejected registration reward address: {e} ({header:?})"); + continue; + }, + }; let utxo = ObservedUtxo { header, data: ObservedUtxoData::Registration(RegistrationData { - cardano_reward_address: CardanoRewardAddressBytes(cardano_address), + cardano_reward_address, dust_public_key, }), }; @@ -272,13 +297,21 @@ impl MidnightCNightObservationDataSourceImpl { }; let reward_address = RewardAddress::new(cardano_network, &credential); - // Unwrap here is OK - we know the reward_address is always 29 bytes - let cardano_address = reward_address.to_address().to_bytes().try_into().unwrap(); + let cardano_reward_address = match decode_reward_address( + reward_address.to_address().to_bytes(), + cardano_network, + ) { + Ok(addr) => addr, + Err(e) => { + log::error!("Rejected deregistration reward address: {e} ({header:?})"); + continue; + }, + }; let utxo = ObservedUtxo { header, data: ObservedUtxoData::Deregistration(DeregistrationData { - cardano_reward_address: CardanoRewardAddressBytes(cardano_address), + cardano_reward_address, dust_public_key, }), }; @@ -328,7 +361,16 @@ impl MidnightCNightObservationDataSourceImpl { continue; }; let reward_address = RewardAddress::new(cardano_network, &base_address.stake_cred()); - let owner = reward_address.to_address().to_bytes().try_into().unwrap(); + let owner = match decode_reward_address( + reward_address.to_address().to_bytes(), + cardano_network, + ) { + Ok(addr) => addr, + Err(e) => { + log::error!("Rejected asset-create owner reward address: {e} ({header:?})"); + continue; + }, + }; let utxo = ObservedUtxo { header, @@ -385,7 +427,16 @@ impl MidnightCNightObservationDataSourceImpl { continue; }; let reward_address = RewardAddress::new(cardano_network, &base_address.stake_cred()); - let owner = reward_address.to_address().to_bytes().try_into().unwrap(); + let owner = match decode_reward_address( + reward_address.to_address().to_bytes(), + cardano_network, + ) { + Ok(addr) => addr, + Err(e) => { + log::error!("Rejected asset-spend owner reward address: {e} ({header:?})"); + continue; + }, + }; let utxo = ObservedUtxo { header, From ade5386ea3bddc9d7789ca9155875b4ea3c17b91 Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 10:39:17 +0100 Subject: [PATCH 3/7] docs(cnight-observation): trim CIP-19 validation comments to proportional verbosity Manual review of the CIP-19 reward-address validation change flagged that several new comments were more verbose than the surrounding existing code. Reduce them so comment density matches the neighbourhood, comments-only with no logic or behaviour change: - config.rs: drop the serde deserializer's length-only rationale block. - pallets/lib.rs: condense the InvalidMappingValidatorAddress variant doc and the validate_mapping_validator_address docblock. - primitives/cnight-observation/lib.rs: drop the CardanoRewardAddressError enum docblock and condense the try_new docblock. - mainchain-follower/cnight_observation.rs: condense the decode_reward_address doc. Issue: https://github.com/shieldedtech/shielded-security-engineering/issues/498 Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Mike Clay --- pallets/cnight-observation/src/config.rs | 6 ------ pallets/cnight-observation/src/lib.rs | 19 ++++++------------- primitives/cnight-observation/src/lib.rs | 16 +++------------- .../src/data_source/cnight_observation.rs | 8 +------- 4 files changed, 10 insertions(+), 39 deletions(-) diff --git a/pallets/cnight-observation/src/config.rs b/pallets/cnight-observation/src/config.rs index 61d1c5dfe..e1c02aef3 100644 --- a/pallets/cnight-observation/src/config.rs +++ b/pallets/cnight-observation/src/config.rs @@ -70,12 +70,6 @@ mod mappings_serde { access.next_entry::>()? { let bytes: Vec = hex::decode(&key).map_err(serde::de::Error::custom)?; - // Length-only check here: serde deserialization has no access to the - // expected network id, so the CIP-19 type + network nibbles are - // validated later in the pallet's genesis `build`, where the network - // is derivable from the mapping-validator address. The message names - // the length so a length failure is not misattributed to a header - // problem. let byte_len = bytes.len(); let addr = CardanoRewardAddressBytes::try_from(bytes).map_err(|_| { serde::de::Error::custom(alloc::format!( diff --git a/pallets/cnight-observation/src/lib.rs b/pallets/cnight-observation/src/lib.rs index c91cac7b6..0f5b90325 100644 --- a/pallets/cnight-observation/src/lib.rs +++ b/pallets/cnight-observation/src/lib.rs @@ -198,8 +198,7 @@ pub mod pallet { pub enum Error { /// A Cardano Wallet address was sent, but was longer than expected MaxCardanoAddrLengthExceeded, - /// The mapping-validator address is not a well-formed Cardano Bech32 - /// address (wrong human-readable prefix, empty, or over the length bound) + /// The mapping-validator address is not a well-formed Cardano Bech32 address InvalidMappingValidatorAddress, /// A Cardano asset name contained non-ASCII bytes NonAsciiAssetName, @@ -472,17 +471,11 @@ pub mod pallet { } impl Pallet { - /// Sibling of the reward-address `try_new` for the mapping-validator address. - /// - /// The mapping-validator address is a Shelley script/enterprise (payment) - /// address, not a reward address — CIP-19 gives it a different address-type - /// category, so it must NOT go through the reward `try_new` (which would - /// reject it on the type nibble). Full Bech32/CIP-19 decoding belongs in the - /// follower, not the pallet, so this validates structurally: the bytes must - /// be a non-empty, valid UTF-8 Cardano Bech32 address (`addr` / `addr_test` - /// prefix) within the bounded length. It returns a specific error rather than - /// the generic length error so a malformed address is distinguishable from a - /// merely over-long one. + /// 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, ) -> Result> { diff --git a/primitives/cnight-observation/src/lib.rs b/primitives/cnight-observation/src/lib.rs index ba349f538..87536719d 100644 --- a/primitives/cnight-observation/src/lib.rs +++ b/primitives/cnight-observation/src/lib.rs @@ -68,11 +68,6 @@ pub struct CardanoRewardAddressBytes( const CARDANO_REWARD_ADDRESS_TYPE_KEY_HASH: u8 = 14; const CARDANO_REWARD_ADDRESS_TYPE_SCRIPT_HASH: u8 = 15; -/// Why a byte string was rejected as a CIP-19 Cardano reward address. -/// -/// Each variant maps to exactly one of the checks [`CardanoRewardAddressBytes::try_new`] -/// runs, so a rejection points at the specific header property that failed rather -/// than collapsing every failure into a single opaque error. #[derive(Debug, Clone, PartialEq, Eq)] pub enum CardanoRewardAddressError { /// The input was not the fixed CIP-19 reward-address length of 29 bytes @@ -106,14 +101,9 @@ impl core::fmt::Display for CardanoRewardAddressError { } impl CardanoRewardAddressBytes { - /// Validated CIP-19 constructor: the single trust-boundary entry point that - /// asserts a byte string is a well-formed reward address for `expected_network`. - /// - /// Checks run length-first so that reading the header byte is only ever done on - /// a 29-byte input, then the header's two nibbles: the upper nibble must be a - /// reward-account type (14 or 15) and the lower nibble must equal - /// `expected_network`. Uses plain slice/bit arithmetic so it compiles under - /// `no_std`. + /// Validated CIP-19 constructor for a reward address on `expected_network`. + /// Checks length first, then the header's type nibble (14 or 15) and network + /// nibble, so the header byte is only read on a 29-byte input. pub fn try_new( bytes: Vec, expected_network: u8, diff --git a/primitives/mainchain-follower/src/data_source/cnight_observation.rs b/primitives/mainchain-follower/src/data_source/cnight_observation.rs index 7c8f5db6a..fc6d2f5f0 100644 --- a/primitives/mainchain-follower/src/data_source/cnight_observation.rs +++ b/primitives/mainchain-follower/src/data_source/cnight_observation.rs @@ -86,13 +86,7 @@ pub enum RegistrationDatumDecodeError { } /// Validate the bytes emitted by `cardano-serialization-lib`'s `RewardAddress` -/// against CIP-19 before wrapping them, replacing the previous -/// `to_bytes().try_into().unwrap()` shortcut at every follower construction site. -/// -/// `cardano_network` is the network the follower is syncing against (derived once -/// in `bulk_pull`); it is the value the reward address's network nibble must match. -/// Routing all four sites through this one helper means every reward address the -/// follower stores has passed the same length + type + network checks. +/// against CIP-19 before wrapping them. fn decode_reward_address( reward_address_bytes: Vec, cardano_network: u8, From fcdd96c6d5a10fe784af526b7c4666e6d1814cab Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 10:47:16 +0100 Subject: [PATCH 4/7] test(cnight-observation): repair and extend genesis/extrinsic CIP-19 tests Post-implementation test-suite review found that routing the genesis mapping-validator address through validate_mapping_validator_address broke the existing over-length panic test: it fed a bad-prefix address that now fails the HRP check before the length branch, and it asserted the old "maximum N" wording that the new message replaced with "max length N". Repair the over-length test to use a valid addr_test-prefixed address so it reaches the length branch, and assert the current wording. Add coverage for the paths that carry real CIP-19 entropy and previously had no negative test: - genesis panics with InvalidMappingValidatorAddress on a bad-prefix validator address (structural reject before the length branch) - genesis panics on a mappings key whose network nibble does not match the network derived from the validator address prefix - set_mapping_validator_contract_address rejects a bad-prefix address with InvalidMappingValidatorAddress and an over-length valid-prefix address with MaxCardanoAddrLengthExceeded, storing neither Verification of compilation and the test run is deferred to the user (cargo is not run in this environment). Issue: https://github.com/shieldedtech/shielded-security-engineering/issues/498 Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Mike Clay --- pallets/cnight-observation/tests/tests.rs | 134 +++++++++++++++++++--- 1 file changed, 119 insertions(+), 15 deletions(-) diff --git a/pallets/cnight-observation/tests/tests.rs b/pallets/cnight-observation/tests/tests.rs index 329421e7d..b0c56564d 100644 --- a/pallets/cnight-observation/tests/tests.rs +++ b/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:: { +// 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>, +) -> pallet_cnight_observation::GenesisConfig { + pallet_cnight_observation::GenesisConfig:: { 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) -> 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("genesis build must panic"); - let msg = payload + payload .downcast_ref::() .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::::get(); + + assert_noop!( + CNightObservation::set_mapping_validator_contract_address( + RawOrigin::Root.into(), + b"not_a_cardano_address".to_vec(), + ), + Error::::InvalidMappingValidatorAddress + ); + + assert_eq!( + MainChainMappingValidatorAddress::::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::::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::::MaxCardanoAddrLengthExceeded + ); + + assert_eq!( + MainChainMappingValidatorAddress::::get(), + before, + "rejected address must not be stored" + ); + }); +} + type BoundedAssetName = BoundedVec>; fn bounded_asset_name(bytes: &[u8]) -> BoundedAssetName { From 2e02d55e3bfcb7741749b02d59549874d523c0a2 Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 11:04:08 +0100 Subject: [PATCH 5/7] chore(cnight-observation): add CIP-19 reward-address validation change file Add the changes/ fragment for the CIP-19 reward-address validation work so the change is traceable to issue #498 in release notes and satisfies the CI changes-check. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Mike Clay --- ...rvation-cip19-reward-address-validation.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 changes/node/changed/cnight-observation-cip19-reward-address-validation.md diff --git a/changes/node/changed/cnight-observation-cip19-reward-address-validation.md b/changes/node/changed/cnight-observation-cip19-reward-address-validation.md new file mode 100644 index 000000000..0fbc4b3b3 --- /dev/null +++ b/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 From ee39eb4807ad51748430b3308be0acf3fa499400 Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 11:34:45 +0100 Subject: [PATCH 6/7] fix(cnight-observation): call validate_mapping_validator_address via Pallet in genesis build The genesis build runs in impl BuildGenesisConfig for GenesisConfig, where Self is GenesisConfig; the helper is an associated fn of Pallet, so the Self:: path failed to resolve (E0599). Qualify the call as Pallet::::. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Mike Clay --- pallets/cnight-observation/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/cnight-observation/src/lib.rs b/pallets/cnight-observation/src/lib.rs index 0f5b90325..b60c8b6a5 100644 --- a/pallets/cnight-observation/src/lib.rs +++ b/pallets/cnight-observation/src/lib.rs @@ -333,7 +333,7 @@ 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::::set( - Self::validate_mapping_validator_address( + Pallet::::validate_mapping_validator_address( self.config.addresses.mapping_validator_address.as_bytes().to_vec(), ) .unwrap_or_else(|e| { From b78009fdcfba0b9fe4e430a26ed60f2af1c8e62e Mon Sep 17 00:00:00 2001 From: Mike Clay Date: Fri, 3 Jul 2026 16:06:55 +0100 Subject: [PATCH 7/7] test(cnight-observation): use expect_err to capture genesis build panic payload catch_unwind returns a Result whose Ok arm is the unit type; calling expect on it yielded that unit value, so downcast_ref was invoked on unit (E0599). Use expect_err to take the panic payload box instead. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Mike Clay --- pallets/cnight-observation/tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/cnight-observation/tests/tests.rs b/pallets/cnight-observation/tests/tests.rs index b0c56564d..eec510566 100644 --- a/pallets/cnight-observation/tests/tests.rs +++ b/pallets/cnight-observation/tests/tests.rs @@ -1726,7 +1726,7 @@ fn build_panic_message(genesis: pallet_cnight_observation::GenesisConfig) genesis.build(); }); })) - .expect("genesis build must panic"); + .expect_err("genesis build must panic"); payload .downcast_ref::()