diff --git a/packages/rs-dpp/src/address_funds/fee_strategy/deduct_fee_from_inputs_and_outputs/v0/mod.rs b/packages/rs-dpp/src/address_funds/fee_strategy/deduct_fee_from_inputs_and_outputs/v0/mod.rs index e52f2623a06..ed975814d07 100644 --- a/packages/rs-dpp/src/address_funds/fee_strategy/deduct_fee_from_inputs_and_outputs/v0/mod.rs +++ b/packages/rs-dpp/src/address_funds/fee_strategy/deduct_fee_from_inputs_and_outputs/v0/mod.rs @@ -24,6 +24,11 @@ pub fn deduct_fee_from_outputs_or_remaining_balance_of_inputs_v0( ) -> Result { let mut remaining_fee = fee; + // Snapshot addresses before any mutations so indices remain stable. + // Without this, removing a drained entry shifts all subsequent indices. + let input_addresses: Vec = inputs.keys().copied().collect(); + let output_addresses: Vec = outputs.keys().copied().collect(); + for step in fee_strategy { if remaining_fee == 0 { break; @@ -31,30 +36,34 @@ pub fn deduct_fee_from_outputs_or_remaining_balance_of_inputs_v0( match step { AddressFundsFeeStrategyStep::DeductFromInput(index) => { - // Reduce the remaining balance of the input at the specified index - if let Some((&address, &(nonce, amount))) = inputs.iter().nth(*index as usize) { - let reduction = remaining_fee.min(amount); - let new_amount = amount - reduction; - remaining_fee -= reduction; + // Resolve the index via the original snapshot, then look up by key + if let Some(&address) = input_addresses.get(*index as usize) { + if let Some(&(nonce, amount)) = inputs.get(&address) { + let reduction = remaining_fee.min(amount); + let new_amount = amount - reduction; + remaining_fee -= reduction; - if new_amount == 0 { - inputs.remove(&address); - } else { - inputs.insert(address, (nonce, new_amount)); + if new_amount == 0 { + inputs.remove(&address); + } else { + inputs.insert(address, (nonce, new_amount)); + } } } } AddressFundsFeeStrategyStep::ReduceOutput(index) => { - // Reduce the output at the specified index - if let Some((&address, &amount)) = outputs.iter().nth(*index as usize) { - let reduction = remaining_fee.min(amount); - let new_amount = amount - reduction; - remaining_fee -= reduction; + // Resolve the index via the original snapshot, then look up by key + if let Some(&address) = output_addresses.get(*index as usize) { + if let Some(&amount) = outputs.get(&address) { + let reduction = remaining_fee.min(amount); + let new_amount = amount - reduction; + remaining_fee -= reduction; - if new_amount == 0 { - outputs.remove(&address); - } else { - outputs.insert(address, new_amount); + if new_amount == 0 { + outputs.remove(&address); + } else { + outputs.insert(address, new_amount); + } } } } diff --git a/packages/rs-dpp/src/address_funds/witness.rs b/packages/rs-dpp/src/address_funds/witness.rs index c8d6aa2fc69..317a3c7603d 100644 --- a/packages/rs-dpp/src/address_funds/witness.rs +++ b/packages/rs-dpp/src/address_funds/witness.rs @@ -6,6 +6,10 @@ use platform_value::BinaryData; #[cfg(feature = "state-transition-serde-conversion")] use serde::{Deserialize, Serialize}; +/// Maximum number of entries in a P2SH signatures vector. +/// This is 16 (max keys from OP_PUSHNUM_16) + 1 (CHECKMULTISIG dummy byte). +pub const MAX_P2SH_SIGNATURES: usize = 17; + /// The input witness data required to spend from a PlatformAddress. /// /// This enum captures the different spending patterns for P2PKH and P2SH addresses. @@ -63,6 +67,13 @@ impl Decode for AddressWitness { } 1 => { let signatures = Vec::::decode(decoder)?; + if signatures.len() > MAX_P2SH_SIGNATURES { + return Err(DecodeError::OtherString(format!( + "P2SH signatures count {} exceeds maximum {}", + signatures.len(), + MAX_P2SH_SIGNATURES, + ))); + } let redeem_script = BinaryData::decode(decoder)?; Ok(AddressWitness::P2sh { signatures, @@ -89,6 +100,13 @@ impl<'de, C> bincode::BorrowDecode<'de, C> for AddressWitness { } 1 => { let signatures = Vec::::borrow_decode(decoder)?; + if signatures.len() > MAX_P2SH_SIGNATURES { + return Err(DecodeError::OtherString(format!( + "P2SH signatures count {} exceeds maximum {}", + signatures.len(), + MAX_P2SH_SIGNATURES, + ))); + } let redeem_script = BinaryData::borrow_decode(decoder)?; Ok(AddressWitness::P2sh { signatures, @@ -190,6 +208,13 @@ impl<'de> Deserialize<'de> for AddressWitness { "p2sh" => { let signatures = signatures.ok_or_else(|| de::Error::missing_field("signatures"))?; + if signatures.len() > MAX_P2SH_SIGNATURES { + return Err(de::Error::custom(format!( + "P2SH signatures count {} exceeds maximum {}", + signatures.len(), + MAX_P2SH_SIGNATURES, + ))); + } let redeem_script = redeem_script .ok_or_else(|| de::Error::missing_field("redeemScript"))?; Ok(AddressWitness::P2sh { @@ -380,4 +405,109 @@ mod tests { assert_eq!(witness, deserialized); } + + /// AUDIT L1: Unbounded P2SH witness size during deserialization. + /// + /// The `Decode` impl for `AddressWitness::P2sh` now enforces + /// `MAX_P2SH_SIGNATURES` during deserialization. A payload with more + /// signatures than the limit is rejected with a decode error. + /// + /// Location: rs-dpp/src/address_funds/witness.rs + #[test] + fn test_p2sh_witness_rejects_excessive_signatures() { + // Create a P2SH witness with 1000 signatures — far above MAX_P2SH_SIGNATURES + let num_signatures = 1000; + let signatures: Vec = (0..num_signatures) + .map(|i| BinaryData::new(vec![0x30, 0x44, i as u8])) + .collect(); + + let witness = AddressWitness::P2sh { + signatures, + redeem_script: BinaryData::new(vec![0x52, 0xae]), + }; + + // Encode succeeds (encoding has no limit), but decode must reject + let encoded = bincode::encode_to_vec(&witness, config::standard()).unwrap(); + let result: Result<(AddressWitness, usize), _> = + bincode::decode_from_slice(&encoded, config::standard()); + + assert!( + result.is_err(), + "AUDIT L1: P2SH witness with {} signatures should be rejected during \ + deserialization. MAX_P2SH_SIGNATURES = {}.", + num_signatures, + MAX_P2SH_SIGNATURES, + ); + } + + /// AUDIT L3: No maximum length check on P2SH signatures vector. + /// + /// The deserialization now enforces `MAX_P2SH_SIGNATURES` (17). Signature + /// counts above this limit are rejected during decode. The boundary value + /// (17) is accepted, and 18+ is rejected. + /// + /// Location: rs-dpp/src/address_funds/witness.rs + #[test] + fn test_p2sh_witness_max_signatures_boundary() { + // Counts above MAX_P2SH_SIGNATURES should be rejected during decode + for count in [50, 100, 500] { + let signatures: Vec = (0..count) + .map(|_| BinaryData::new(vec![0x30, 0x44, 0x02, 0x20])) + .collect(); + + let witness = AddressWitness::P2sh { + signatures, + redeem_script: BinaryData::new(vec![0x52, 0xae]), + }; + + let encoded = bincode::encode_to_vec(&witness, config::standard()).unwrap(); + let result: Result<(AddressWitness, usize), _> = + bincode::decode_from_slice(&encoded, config::standard()); + + assert!( + result.is_err(), + "AUDIT L3: P2SH witness with {} signatures should be rejected during \ + deserialization. MAX_P2SH_SIGNATURES = {}.", + count, + MAX_P2SH_SIGNATURES, + ); + } + + // MAX_P2SH_SIGNATURES (17) should be accepted + let signatures: Vec = (0..MAX_P2SH_SIGNATURES) + .map(|_| BinaryData::new(vec![0x30, 0x44, 0x02, 0x20])) + .collect(); + + let witness = AddressWitness::P2sh { + signatures, + redeem_script: BinaryData::new(vec![0x52, 0xae]), + }; + + let encoded = bincode::encode_to_vec(&witness, config::standard()).unwrap(); + let decoded: AddressWitness = bincode::decode_from_slice(&encoded, config::standard()) + .unwrap() + .0; + + assert_eq!(witness, decoded); + + // MAX_P2SH_SIGNATURES + 1 should be rejected + let signatures: Vec = (0..MAX_P2SH_SIGNATURES + 1) + .map(|_| BinaryData::new(vec![0x30, 0x44, 0x02, 0x20])) + .collect(); + + let witness = AddressWitness::P2sh { + signatures, + redeem_script: BinaryData::new(vec![0x52, 0xae]), + }; + + let encoded = bincode::encode_to_vec(&witness, config::standard()).unwrap(); + let result: Result<(AddressWitness, usize), _> = + bincode::decode_from_slice(&encoded, config::standard()); + + assert!( + result.is_err(), + "P2SH witness with {} signatures (MAX + 1) should be rejected", + MAX_P2SH_SIGNATURES + 1, + ); + } } diff --git a/packages/rs-dpp/src/errors/consensus/basic/basic_error.rs b/packages/rs-dpp/src/errors/consensus/basic/basic_error.rs index 08f6a5aa874..db5d518eb93 100644 --- a/packages/rs-dpp/src/errors/consensus/basic/basic_error.rs +++ b/packages/rs-dpp/src/errors/consensus/basic/basic_error.rs @@ -80,6 +80,7 @@ use crate::consensus::basic::state_transition::{ OutputsNotGreaterThanInputsError, StateTransitionMaxSizeExceededError, StateTransitionNotActiveError, TransitionNoInputsError, TransitionNoOutputsError, TransitionOverMaxInputsError, TransitionOverMaxOutputsError, WithdrawalBalanceMismatchError, + WithdrawalBelowMinAmountError, }; use crate::consensus::basic::{ IncompatibleProtocolVersionError, UnsupportedFeatureError, UnsupportedProtocolVersionError, @@ -645,6 +646,9 @@ pub enum BasicError { #[error(transparent)] WithdrawalBalanceMismatchError(WithdrawalBalanceMismatchError), + #[error(transparent)] + WithdrawalBelowMinAmountError(WithdrawalBelowMinAmountError), + #[error(transparent)] InsufficientFundingAmountError(InsufficientFundingAmountError), diff --git a/packages/rs-dpp/src/errors/consensus/basic/state_transition/mod.rs b/packages/rs-dpp/src/errors/consensus/basic/state_transition/mod.rs index 7753bafef39..a7c02e2db76 100644 --- a/packages/rs-dpp/src/errors/consensus/basic/state_transition/mod.rs +++ b/packages/rs-dpp/src/errors/consensus/basic/state_transition/mod.rs @@ -20,6 +20,7 @@ mod transition_no_outputs_error; mod transition_over_max_inputs_error; mod transition_over_max_outputs_error; mod withdrawal_balance_mismatch_error; +mod withdrawal_below_min_amount_error; pub use fee_strategy_duplicate_error::*; pub use fee_strategy_empty_error::*; @@ -43,3 +44,4 @@ pub use transition_no_outputs_error::*; pub use transition_over_max_inputs_error::*; pub use transition_over_max_outputs_error::*; pub use withdrawal_balance_mismatch_error::*; +pub use withdrawal_below_min_amount_error::*; diff --git a/packages/rs-dpp/src/errors/consensus/basic/state_transition/withdrawal_below_min_amount_error.rs b/packages/rs-dpp/src/errors/consensus/basic/state_transition/withdrawal_below_min_amount_error.rs new file mode 100644 index 00000000000..7372a9f184d --- /dev/null +++ b/packages/rs-dpp/src/errors/consensus/basic/state_transition/withdrawal_below_min_amount_error.rs @@ -0,0 +1,52 @@ +use crate::consensus::basic::BasicError; +use crate::consensus::ConsensusError; +use crate::errors::ProtocolError; +use bincode::{Decode, Encode}; +use platform_serialization_derive::{PlatformDeserialize, PlatformSerialize}; +use thiserror::Error; + +#[derive( + Error, Debug, Clone, PartialEq, Eq, Encode, Decode, PlatformSerialize, PlatformDeserialize, +)] +#[error( + "Withdrawal amount {withdrawal_amount} must be at least {min_amount} and at most {max_amount}" +)] +#[platform_serialize(unversioned)] +pub struct WithdrawalBelowMinAmountError { + /* + + DO NOT CHANGE ORDER OF FIELDS WITHOUT INTRODUCING OF NEW VERSION + + */ + withdrawal_amount: u64, + min_amount: u64, + max_amount: u64, +} + +impl WithdrawalBelowMinAmountError { + pub fn new(withdrawal_amount: u64, min_amount: u64, max_amount: u64) -> Self { + Self { + withdrawal_amount, + min_amount, + max_amount, + } + } + + pub fn withdrawal_amount(&self) -> u64 { + self.withdrawal_amount + } + + pub fn min_amount(&self) -> u64 { + self.min_amount + } + + pub fn max_amount(&self) -> u64 { + self.max_amount + } +} + +impl From for ConsensusError { + fn from(err: WithdrawalBelowMinAmountError) -> Self { + Self::BasicError(BasicError::WithdrawalBelowMinAmountError(err)) + } +} diff --git a/packages/rs-dpp/src/errors/consensus/codes.rs b/packages/rs-dpp/src/errors/consensus/codes.rs index b25a99a6909..1758f022ac8 100644 --- a/packages/rs-dpp/src/errors/consensus/codes.rs +++ b/packages/rs-dpp/src/errors/consensus/codes.rs @@ -231,6 +231,7 @@ impl ErrorWithCode for BasicError { Self::InputsNotLessThanOutputsError(_) => 10815, Self::OutputAddressAlsoInputError(_) => 10816, Self::InvalidRemainderOutputCountError(_) => 10817, + Self::WithdrawalBelowMinAmountError(_) => 10818, } } } diff --git a/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs b/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs index 1944600870b..9dcbc061782 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs @@ -9,13 +9,15 @@ use crate::consensus::basic::state_transition::{ FeeStrategyDuplicateError, FeeStrategyEmptyError, FeeStrategyIndexOutOfBoundsError, FeeStrategyTooManyStepsError, InputBelowMinimumError, InputWitnessCountMismatchError, OutputAddressAlsoInputError, OutputBelowMinimumError, TransitionNoInputsError, - TransitionOverMaxInputsError, + TransitionOverMaxInputsError, WithdrawalBalanceMismatchError, WithdrawalBelowMinAmountError, }; use crate::consensus::basic::BasicError; use crate::state_transition::address_credit_withdrawal_transition::v0::AddressCreditWithdrawalTransitionV0; -use crate::state_transition::address_credit_withdrawal_transition::MIN_CORE_FEE_PER_BYTE; +use crate::state_transition::address_credit_withdrawal_transition::{ + MIN_CORE_FEE_PER_BYTE, MIN_WITHDRAWAL_AMOUNT, +}; use crate::state_transition::StateTransitionStructureValidation; -use crate::util::is_fibonacci_number::is_fibonacci_number; +use crate::util::is_non_zero_fibonacci_number::is_non_zero_fibonacci_number; use crate::validation::SimpleConsensusValidationResult; use crate::withdrawal::Pooling; use platform_version::version::PlatformVersion; @@ -171,9 +173,6 @@ impl StateTransitionStructureValidation for AddressCreditWithdrawalTransitionV0 } } - // Note: The withdrawal amount is implicitly input_sum - output_sum - // No explicit balance check needed here as the withdrawal amount is computed, not specified - // Validate pooling - currently we do not support pooling, so we must validate that pooling is `Never` if self.pooling != Pooling::Never { return SimpleConsensusValidationResult::new_with_error( @@ -183,7 +182,7 @@ impl StateTransitionStructureValidation for AddressCreditWithdrawalTransitionV0 } // Validate core_fee_per_byte is a Fibonacci number - if !is_fibonacci_number(self.core_fee_per_byte as u64) { + if !is_non_zero_fibonacci_number(self.core_fee_per_byte as u64) { return SimpleConsensusValidationResult::new_with_error( InvalidCreditWithdrawalTransitionCoreFeeError::new( self.core_fee_per_byte, @@ -212,6 +211,35 @@ impl StateTransitionStructureValidation for AddressCreditWithdrawalTransitionV0 ); } + // Validate that input_sum > output_amount (withdrawal amount must be positive) + let input_sum = input_sum.unwrap(); // Safe: checked above + let output_amount = self.output.as_ref().map_or(0, |(_, amount)| *amount); + if input_sum <= output_amount { + return SimpleConsensusValidationResult::new_with_error( + BasicError::WithdrawalBalanceMismatchError(WithdrawalBalanceMismatchError::new( + input_sum, + output_amount, + input_sum.saturating_sub(output_amount), + )) + .into(), + ); + } + + // Validate withdrawal amount meets minimum and maximum + let withdrawal_amount = input_sum - output_amount; // Safe: checked input_sum > output_amount above + if withdrawal_amount < MIN_WITHDRAWAL_AMOUNT + || withdrawal_amount > platform_version.system_limits.max_withdrawal_amount + { + return SimpleConsensusValidationResult::new_with_error( + BasicError::WithdrawalBelowMinAmountError(WithdrawalBelowMinAmountError::new( + withdrawal_amount, + MIN_WITHDRAWAL_AMOUNT, + platform_version.system_limits.max_withdrawal_amount, + )) + .into(), + ); + } + SimpleConsensusValidationResult::new() } } @@ -221,7 +249,9 @@ mod tests { use super::*; use crate::address_funds::AddressWitness; use crate::address_funds::PlatformAddress; + use crate::identity::core_script::CoreScript; use assert_matches::assert_matches; + use rand::SeedableRng; use std::collections::BTreeMap; #[test] @@ -243,6 +273,8 @@ mod tests { }, ], fee_strategy: vec![AddressFundsFeeStrategyStep::DeductFromInput(0)], + core_fee_per_byte: 1, // Valid Fibonacci number — ensures we reach the overflow check + output_script: CoreScript::random_p2pkh(&mut rand::rngs::StdRng::seed_from_u64(1)), ..Default::default() }; @@ -251,9 +283,7 @@ mod tests { assert_matches!( result.errors.as_slice(), [crate::consensus::ConsensusError::BasicError( - BasicError::InvalidCreditWithdrawalTransitionCoreFeeError( - InvalidCreditWithdrawalTransitionCoreFeeError { .. } - ) + BasicError::OverflowError(_) )] ); } diff --git a/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs b/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs index 481c74a843d..ec9d56ce610 100644 --- a/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs +++ b/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs @@ -50,15 +50,25 @@ pub trait StateTransitionWitnessValidation: StateTransitionWitnessSigned + Signa /// # Returns /// * `WitnessValidationResult` - Contains validation result and operations performed fn validate_witnesses(&self, signable_bytes: &[u8]) -> WitnessValidationResult { + let inputs = self.inputs(); + let witnesses = self.witnesses(); + + // Verify witness count matches input count before zipping + if inputs.len() != witnesses.len() { + return WitnessValidationResult::new_with_error( + InvalidStateTransitionSignatureError::new(format!( + "Number of witnesses ({}) does not match number of inputs ({})", + witnesses.len(), + inputs.len() + )) + .into(), + ); + } + let mut total_operations = AddressWitnessVerificationOperations::new(); // Validate each witness against its corresponding input address - for (i, (address, witness)) in self - .inputs() - .keys() - .zip(self.witnesses().iter()) - .enumerate() - { + for (i, (address, witness)) in inputs.keys().zip(witnesses.iter()).enumerate() { match address.verify_bytes_against_witness(witness, signable_bytes) { Ok(operations) => { total_operations.combine(&operations); diff --git a/packages/rs-dpp/src/util/is_fibonacci_number.rs b/packages/rs-dpp/src/util/is_fibonacci_number.rs deleted file mode 100644 index 4c736e0a9a0..00000000000 --- a/packages/rs-dpp/src/util/is_fibonacci_number.rs +++ /dev/null @@ -1,20 +0,0 @@ -fn is_perfect_square(number: u64) -> bool { - (number as f64).sqrt().fract() == 0.0 -} - -pub fn is_fibonacci_number(number: u64) -> bool { - let square_check_up = 5u64 - .checked_mul(number) - .and_then(|n| n.checked_mul(number)) - .and_then(|n| n.checked_add(4)); - - let square_check_down = 5u64 - .checked_mul(number) - .and_then(|n| n.checked_mul(number)) - .and_then(|n| n.checked_sub(4)); - - match (square_check_up, square_check_down) { - (Some(n1), Some(n2)) => is_perfect_square(n1) || is_perfect_square(n2), - _ => false, // Return false if either calculation overflows - } -} diff --git a/packages/rs-dpp/src/util/is_non_zero_fibonacci_number.rs b/packages/rs-dpp/src/util/is_non_zero_fibonacci_number.rs new file mode 100644 index 00000000000..8474a09f84d --- /dev/null +++ b/packages/rs-dpp/src/util/is_non_zero_fibonacci_number.rs @@ -0,0 +1,129 @@ +fn is_perfect_square(number: u64) -> bool { + if number < 2 { + return true; + } + // Integer square root via Newton's method + let mut x = number; + let mut y = x.div_ceil(2); + while y < x { + x = y; + y = (x + number / x) / 2; + } + x * x == number +} + +pub fn is_non_zero_fibonacci_number(number: u64) -> bool { + if number == 0 { + return false; + } + + let square_check_up = 5u64 + .checked_mul(number) + .and_then(|n| n.checked_mul(number)) + .and_then(|n| n.checked_add(4)); + + let square_check_down = 5u64 + .checked_mul(number) + .and_then(|n| n.checked_mul(number)) + .and_then(|n| n.checked_sub(4)); + + match (square_check_up, square_check_down) { + (Some(n1), Some(n2)) => is_perfect_square(n1) || is_perfect_square(n2), + (Some(n1), None) => is_perfect_square(n1), + (None, Some(n2)) => is_perfect_square(n2), + (None, None) => false, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// AUDIT M6: Floating-point imprecision in is_perfect_square for large values. + /// + /// `is_perfect_square` casts u64 to f64 and checks `sqrt().fract() == 0.0`. + /// f64 has only 52 bits of mantissa, so for values above 2^52, precision is lost. + /// This means `is_perfect_square` can return incorrect results for large numbers. + /// + /// Currently not exploitable for `core_fee_per_byte` (u32), but the function + /// accepts u64 and is technically unsound for large inputs. + /// + /// Location: rs-dpp/src/util/is_non_zero_fibonacci_number.rs:1-3 + #[test] + fn test_is_perfect_square_large_values() { + // For small values, is_perfect_square works correctly + assert!(is_perfect_square(0)); + assert!(is_perfect_square(1)); + assert!(is_perfect_square(4)); + assert!(is_perfect_square(9)); + assert!(is_perfect_square(16)); + assert!(!is_perfect_square(2)); + assert!(!is_perfect_square(3)); + + // Find a value where f64 imprecision causes is_perfect_square to give wrong answer. + // The number (2^26 + 1)^2 = 2^52 + 2^27 + 1 = 4503599761588225 + // When cast to f64, this may lose the +1 and appear as (2^26 + 1)^2 exactly, + // or nearby non-squares may appear as perfect squares. + // + // Strategy: find a non-square near a large perfect square where f64 rounds + // the sqrt to an exact integer. + let base: u64 = (1u64 << 26) + 1; // 67108865 + let perfect_square = base * base; // 4503599761588225 + + // Verify perfect_square is correctly identified + assert!( + is_perfect_square(perfect_square), + "AUDIT M6: {} should be a perfect square ({}^2)", + perfect_square, + base + ); + + // Check non-squares adjacent to large perfect squares. + // Due to f64 imprecision, some of these may incorrectly return true. + let false_positives: Vec = (1..=10u64) + .filter(|&offset| { + let candidate = perfect_square + offset; + // This should NOT be a perfect square, but f64 may say it is + is_perfect_square(candidate) + }) + .collect(); + + // If is_perfect_square is sound, there should be no false positives. + // With f64 imprecision, some non-squares will be falsely identified as perfect squares. + assert!( + false_positives.is_empty(), + "AUDIT M6: is_perfect_square returned true for non-square values near {}^2: \ + offsets {:?}. This is caused by f64 imprecision — (number as f64).sqrt() \ + rounds to an exact integer for these non-square values. \ + Fix: use integer square root instead of floating-point.", + base, + false_positives + ); + } + + /// AUDIT M6 (supplementary): Verify is_non_zero_fibonacci_number works for known values + #[test] + fn test_known_non_zero_fibonacci_numbers() { + // Known non-zero Fibonacci numbers + let fibs = [ + 1, 1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144, 233, 377, 610, 987, 1597, + ]; + for &n in &fibs { + assert!( + is_non_zero_fibonacci_number(n), + "{} should be recognized as a Fibonacci number", + n + ); + } + + // Known non-Fibonacci numbers + let non_fibs = [0, 4, 6, 7, 9, 10, 11, 12, 14, 15, 16, 22, 100]; + for &n in &non_fibs { + assert!( + !is_non_zero_fibonacci_number(n), + "{} should NOT be recognized as a Fibonacci number", + n + ); + } + } +} diff --git a/packages/rs-dpp/src/util/mod.rs b/packages/rs-dpp/src/util/mod.rs index 2dee13be293..a95aa51923f 100644 --- a/packages/rs-dpp/src/util/mod.rs +++ b/packages/rs-dpp/src/util/mod.rs @@ -6,7 +6,7 @@ pub mod cbor_value; pub mod deserializer; pub mod entropy_generator; pub mod hash; -pub mod is_fibonacci_number; +pub mod is_non_zero_fibonacci_number; pub mod json_path; pub mod json_schema; pub mod json_value; diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_credit_withdrawal/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_credit_withdrawal/tests.rs index 68706b62a87..4fb2b771b80 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_credit_withdrawal/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_credit_withdrawal/tests.rs @@ -3921,7 +3921,7 @@ mod tests { #[test] fn test_large_amount_withdrawal() { - // Test withdrawal with very large amounts + // Test withdrawal at the maximum allowed amount (500 Dash) let platform_version = PlatformVersion::latest(); let platform_config = PlatformConfig { testing_configs: PlatformTestConfig { @@ -3939,18 +3939,16 @@ mod tests { let mut signer = TestAddressSigner::new(); let input_address = signer.add_p2pkh([1u8; 32]); - // Very large balance (1 billion credits) - let large_balance = 1_000_000_000_000_000_000u64; + // Balance large enough to cover max withdrawal + fees + let large_balance = dash_to_credits!(600.0); setup_address_with_balance(&mut platform, input_address, 0, large_balance); let mut rng = StdRng::seed_from_u64(567); let mut inputs = BTreeMap::new(); - // Withdraw most of it - inputs.insert( - input_address, - (1 as AddressNonce, large_balance - dash_to_credits!(1.0)), - ); + // Withdraw the max allowed amount (500 Dash = 50_000_000_000_000 credits) + let max_withdrawal = platform_version.system_limits.max_withdrawal_amount; + inputs.insert(input_address, (1 as AddressNonce, max_withdrawal)); let transition = create_signed_address_credit_withdrawal_transition( &signer, @@ -5830,4 +5828,444 @@ mod tests { ); } } + + mod security { + use super::*; + use dpp::state_transition::StateTransitionStructureValidation; + + /// AUDIT C1 (Structure): Output amount exceeds input sum — should be rejected. + /// + /// The withdrawal amount is computed as `total_inputs - output_amount`. + /// If `output_amount > total_inputs`, this subtraction underflows (panic in + /// debug, wrap in release). Structure validation currently does NOT check + /// that `output_amount <= input_sum`. + /// + /// Location: rs-drive/.../address_credit_withdrawal/v0/transformer.rs:40 + #[test] + fn test_output_exceeds_input_rejected_by_structure() { + let platform_version = PlatformVersion::latest(); + let mut rng = StdRng::seed_from_u64(999); + + let mut inputs = BTreeMap::new(); + inputs.insert( + create_platform_address(1), + (1 as AddressNonce, dash_to_credits!(0.01)), + ); + + // Output (change) is 100x larger than input sum + let output = Some((create_platform_address(2), dash_to_credits!(1.0))); + + let transition = AddressCreditWithdrawalTransitionV0 { + inputs, + output, + fee_strategy: AddressFundsFeeStrategy::from(vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + ]), + core_fee_per_byte: 1, + pooling: Pooling::Never, + output_script: create_random_output_script(&mut rng), + user_fee_increase: 0, + input_witnesses: vec![create_dummy_witness()], + }; + + let result = transition.validate_structure(platform_version); + + // SHOULD be invalid: output (1.0 Dash) > input (0.01 Dash) would + // cause underflow in withdrawal amount calculation. + // Currently FAILS: structure validation does not check this invariant. + assert!( + !result.is_valid(), + "AUDIT C1: Structure validation should reject when output amount ({}) exceeds \ + input sum ({}). The withdrawal amount would underflow to a huge u64 value.", + dash_to_credits!(1.0), + dash_to_credits!(0.01), + ); + } + + /// AUDIT C1 (Processing): When output > input reaches the transformer, + /// the unchecked subtraction panics in debug mode or wraps in release mode. + /// + /// This test submits a signed transition where output > input. Since + /// structure validation does not catch this, processing reaches the + /// transformer where `total_inputs - output_amount` underflows. + /// + /// Expected: the transition is rejected with an error. + /// Actual: panics (debug) or produces a wrapped withdrawal amount (release). + #[test] + fn test_output_exceeds_input_rejected_by_processing() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let input_address = signer.add_p2pkh([1u8; 32]); + // Address has 1.0 Dash balance + setup_address_with_balance(&mut platform, input_address, 0, dash_to_credits!(1.0)); + + let mut rng = StdRng::seed_from_u64(567); + + // Input takes 0.01 Dash from the address + let mut inputs = BTreeMap::new(); + inputs.insert(input_address, (1 as AddressNonce, dash_to_credits!(0.01))); + + // Output (change) requests 0.5 Dash — more than input sum of 0.01 Dash + // withdrawal_amount = 0.01 - 0.5 = UNDERFLOW + let output_address = create_platform_address(2); + + let transition = create_signed_address_credit_withdrawal_transition( + &signer, + inputs, + Some((output_address, dash_to_credits!(0.5))), + vec![AddressFundsFeeStrategyStep::DeductFromInput(0)], + create_random_output_script(&mut rng), + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + // In debug mode, this panics at `total_inputs - output_amount`. + // In release mode, the withdrawal amount wraps to u64::MAX - delta. + // Either way, the transition should be REJECTED, not succeed. + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition without panic"); + + // Should NOT be a successful execution — output > input must be rejected + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::WithdrawalBalanceMismatchError(_)) + )] + ); + } + + /// AUDIT M1: Fee deduction BTreeMap index shifting after entry removal. + /// + /// When fee strategy step DeductFromInput(0) drains input A to zero, + /// A is removed from the BTreeMap. The next step DeductFromInput(1) + /// now targets what was originally at index 2 (C) instead of index 1 (B), + /// because all indices shifted down after the removal. + /// + /// Location: rs-dpp/.../deduct_fee_from_inputs_and_outputs/v0/mod.rs:35-45 + #[test] + fn test_fee_deduction_stable_after_entry_removal() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let addr_a = signer.add_p2pkh([10u8; 32]); + let addr_b = signer.add_p2pkh([20u8; 32]); + let addr_c = signer.add_p2pkh([30u8; 32]); + + // Determine BTreeMap sort order + let mut sorted_addrs = vec![addr_a, addr_b, addr_c]; + sorted_addrs.sort(); + let first = sorted_addrs[0]; + let second = sorted_addrs[1]; + let third = sorted_addrs[2]; + + let first_balance = dash_to_credits!(0.1); + let second_balance = dash_to_credits!(1.0); + let third_balance = dash_to_credits!(1.0); + + // Input amount leaves only 1000 credits remaining for first + let first_input = first_balance - 1000; + let second_input = dash_to_credits!(0.01); + let third_input = dash_to_credits!(0.01); + + setup_address_with_balance(&mut platform, first, 0, first_balance); + setup_address_with_balance(&mut platform, second, 0, second_balance); + setup_address_with_balance(&mut platform, third, 0, third_balance); + + let mut rng = StdRng::seed_from_u64(567); + + let mut inputs = BTreeMap::new(); + inputs.insert(first, (1 as AddressNonce, first_input)); + inputs.insert(second, (1 as AddressNonce, second_input)); + inputs.insert(third, (1 as AddressNonce, third_input)); + + // Fee strategy: deduct from index 0 (first), then index 1 (should be second). + let transition = create_signed_address_credit_withdrawal_transition( + &signer, + inputs, + None, + vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + AddressFundsFeeStrategyStep::DeductFromInput(1), + ], + create_random_output_script(&mut rng), + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &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 { .. }], + "Transaction should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + let second_remaining_before_fee = second_balance - second_input; + + let (_, second_final) = platform + .drive + .fetch_balance_and_nonce(&second, None, platform_version) + .expect("should fetch") + .expect("second address should exist"); + + assert!( + second_final < second_remaining_before_fee, + "AUDIT M1: Fee should have been deducted from second address (original \ + BTreeMap index 1), but it was deducted from third address instead. \ + After first was drained (1000 credits) and removed from BTreeMap, \ + DeductFromInput(1) shifted to target the third address. \ + second's balance: {} (expected < {})", + second_final, + second_remaining_before_fee + ); + } + + /// AUDIT H2: Missing MIN_WITHDRAWAL_AMOUNT validation for address withdrawals. + /// + /// The identity credit withdrawal transition checks MIN_WITHDRAWAL_AMOUNT at + /// `identity_credit_withdrawal/structure/v1/mod.rs:33`, but the address credit + /// withdrawal transition has no such check. An attacker can create a withdrawal + /// with a tiny amount (e.g., 1 credit) which is economically irrational and + /// wastes chain resources (creates a Core transaction for dust). + /// + /// This test creates a withdrawal where `withdrawal_amount = input_sum - output_amount` + /// is very small (1000 credits = dust) and verifies that structure validation rejects it. + /// + /// Location: rs-dpp/.../address_credit_withdrawal/v0/state_transition_validation.rs + #[test] + fn test_withdrawal_below_min_amount_rejected_by_structure() { + let platform_version = PlatformVersion::latest(); + let mut rng = StdRng::seed_from_u64(999); + + // Input has 0.1 Dash, output takes back 0.1 Dash minus 1000 credits + // This leaves withdrawal_amount = 1000 credits (dust) + let input_amount = dash_to_credits!(0.1); + let output_amount = input_amount - 1000; // Leave only 1000 credits for withdrawal + + let mut inputs = BTreeMap::new(); + inputs.insert( + create_platform_address(1), + (1 as AddressNonce, input_amount), + ); + + let output = Some((create_platform_address(2), output_amount)); + + let transition = AddressCreditWithdrawalTransitionV0 { + inputs, + output, + fee_strategy: AddressFundsFeeStrategy::from(vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + ]), + core_fee_per_byte: 1, + pooling: Pooling::Never, + output_script: create_random_output_script(&mut rng), + user_fee_increase: 0, + input_witnesses: vec![create_dummy_witness()], + }; + + let result = transition.validate_structure(platform_version); + + // SHOULD be invalid: 1000 credits is below any reasonable MIN_WITHDRAWAL_AMOUNT. + // The identity credit withdrawal enforces this, but address withdrawal does not. + assert!( + !result.is_valid(), + "AUDIT H2: Dust withdrawal of 1000 credits (input {} - output {} = {}) \ + should be rejected by structure validation. Identity credit withdrawal \ + enforces MIN_WITHDRAWAL_AMOUNT but address credit withdrawal does not. \ + This allows economically irrational withdrawals that waste Core chain \ + resources creating dust transactions.", + input_amount, + output_amount, + input_amount - output_amount, + ); + } + + /// AUDIT H2 (Processing): Dust withdrawal reaches processing without rejection. + /// + /// Signed version of the H2 test that goes through the full processing pipeline. + #[test] + fn test_withdrawal_below_min_amount_rejected_by_processing() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let input_address = signer.add_p2pkh([1u8; 32]); + setup_address_with_balance(&mut platform, input_address, 0, dash_to_credits!(1.0)); + + let mut rng = StdRng::seed_from_u64(567); + + // Input takes 0.1 Dash, output returns all but 1000 credits + let input_amount = dash_to_credits!(0.1); + let output_amount = input_amount - 1000; + + let mut inputs = BTreeMap::new(); + inputs.insert(input_address, (1 as AddressNonce, input_amount)); + + let output_address = create_platform_address(2); + + let transition = create_signed_address_credit_withdrawal_transition( + &signer, + inputs, + Some((output_address, output_amount)), + vec![AddressFundsFeeStrategyStep::DeductFromInput(0)], + create_random_output_script(&mut rng), + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + + // Dust withdrawal should NOT succeed + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::WithdrawalBelowMinAmountError(_)) + )] + ); + } + + /// AUDIT H3: Unchecked `.sum()` in withdrawal transformer uses wrapping arithmetic. + /// + /// At `transformer.rs:36`, the withdrawal transformer computes input sum using + /// `.sum()` which wraps on overflow in release mode. Structure validation has + /// an upstream overflow check, but if bypassed, the transformer would produce + /// a wrapped (incorrect) sum. This is a defense-in-depth violation. + /// + /// This test verifies that structure validation correctly catches the overflow + /// (complementing M7). The transformer code should also use `checked_add`. + /// + /// Location: rs-drive/.../address_credit_withdrawal/v0/transformer.rs:36 + #[test] + fn test_transformer_input_sum_uses_checked_add() { + let platform_version = PlatformVersion::latest(); + let mut rng = StdRng::seed_from_u64(999); + + // Two inputs that sum to > u64::MAX + let mut inputs = BTreeMap::new(); + inputs.insert(create_platform_address(1), (0 as AddressNonce, u64::MAX)); + inputs.insert(create_platform_address(2), (0 as AddressNonce, u64::MAX)); + + let transition = AddressCreditWithdrawalTransitionV0 { + inputs, + output: None, + fee_strategy: AddressFundsFeeStrategy::from(vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + ]), + core_fee_per_byte: 1, // Valid Fibonacci number + pooling: Pooling::Never, + output_script: create_random_output_script(&mut rng), + user_fee_increase: 0, + input_witnesses: vec![create_dummy_witness(), create_dummy_witness()], + }; + + let result = transition.validate_structure(platform_version); + + // Structure validation should catch the overflow + assert!( + !result.is_valid(), + "AUDIT H3: Two inputs of u64::MAX should cause overflow. Structure validation \ + catches this, but the transformer at transformer.rs:36 uses .sum() which would \ + silently wrap in release mode if structure validation were bypassed. \ + The transformer should use checked_add for defense-in-depth." + ); + + // Verify it's specifically an overflow error (not some other error) + let has_overflow_error = result + .errors + .iter() + .any(|e| matches!(e, ConsensusError::BasicError(BasicError::OverflowError(_)))); + assert!( + has_overflow_error, + "AUDIT H3: Expected OverflowError for input sum overflow, got {:?}", + result.errors + ); + } + } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs index 0d524484384..090e49840ea 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs @@ -8920,4 +8920,671 @@ mod tests { } } } + + mod security { + use super::*; + + /// AUDIT M1: Fee deduction BTreeMap index shifting after entry removal. + /// + /// When fee strategy step DeductFromInput(0) drains input A to zero, + /// A is removed from the BTreeMap. The next step DeductFromInput(1) + /// now targets what was originally at index 2 (C) instead of index 1 (B), + /// because all indices shifted down after the removal. + /// + /// Location: rs-dpp/.../deduct_fee_from_inputs_and_outputs/v0/mod.rs:35-45 + #[test] + fn test_fee_deduction_stable_after_entry_removal() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let addr_a = signer.add_p2pkh([10u8; 32]); + let addr_b = signer.add_p2pkh([20u8; 32]); + let addr_c = signer.add_p2pkh([30u8; 32]); + + // Determine BTreeMap sort order + let mut sorted_addrs = vec![addr_a, addr_b, addr_c]; + sorted_addrs.sort(); + let first = sorted_addrs[0]; + let second = sorted_addrs[1]; + let third = sorted_addrs[2]; + + let first_balance = dash_to_credits!(0.1); + let second_balance = dash_to_credits!(1.0); + let third_balance = dash_to_credits!(1.0); + + // Input amount leaves only 1000 credits remaining for first + let first_input = first_balance - 1000; + let second_input = dash_to_credits!(0.01); + let third_input = dash_to_credits!(0.01); + + setup_address_with_balance(&mut platform, first, 0, first_balance); + setup_address_with_balance(&mut platform, second, 0, second_balance); + setup_address_with_balance(&mut platform, third, 0, third_balance); + + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + // Asset lock provides ~1.0 Dash of credits + + let mut inputs = BTreeMap::new(); + inputs.insert(first, (1 as AddressNonce, first_input)); + inputs.insert(second, (1 as AddressNonce, second_input)); + inputs.insert(third, (1 as AddressNonce, third_input)); + + let remainder_address = create_platform_address(100); + let mut outputs = BTreeMap::new(); + outputs.insert(remainder_address, None); // remainder output + + // Fee strategy: deduct from index 0 (first), then index 1 (should be second). + let transition = create_signed_address_funding_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + &signer, + inputs, + outputs, + vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + AddressFundsFeeStrategyStep::DeductFromInput(1), + ], + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &[result], + &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 { .. }], + "Transaction should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + let second_remaining_before_fee = second_balance - second_input; + + let (_, second_final) = platform + .drive + .fetch_balance_and_nonce(&second, None, platform_version) + .expect("should fetch") + .expect("second address should exist"); + + assert!( + second_final < second_remaining_before_fee, + "AUDIT M1: Fee should have been deducted from second address (original \ + BTreeMap index 1), but it was deducted from third address instead. \ + After first was drained (1000 credits) and removed from BTreeMap, \ + DeductFromInput(1) shifted to target the third address. \ + second's balance: {} (expected < {})", + second_final, + second_remaining_before_fee + ); + } + + /// AUDIT M2: ReduceOutput index invalidated after remainder removal. + /// + /// When `total_available == explicit_outputs_sum`, the remainder output is + /// removed at `advanced_structure/v0/mod.rs:84-86`. If the fee strategy + /// includes `ReduceOutput(i)` targeting the remainder position, the index + /// becomes out-of-bounds after removal. Fee deduction silently skips the + /// step, meaning the attacker gets a free (zero-fee) transaction. + /// + /// This test creates a transition where total available equals explicit outputs + /// (so remainder is 0 and removed), with a fee strategy targeting the removed output. + /// + /// Location: rs-dpp/.../address_funding_from_asset_lock/advanced_structure/v0/mod.rs:84-86 + #[test] + fn test_reduce_output_index_after_remainder_removal() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let signer = TestAddressSigner::new(); + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + // Asset lock provides ~1.0 Dash of credits (100_000_000 duffs * 1000) + let asset_lock_credits = dash_to_credits!(1.0); + + // Create two explicit outputs that sum to exactly the asset lock amount. + // This means remainder = 0, which triggers remainder removal. + let output_addr_1 = create_platform_address(1); + let output_addr_2 = create_platform_address(2); + + let mut outputs = BTreeMap::new(); + outputs.insert(output_addr_1, Some(asset_lock_credits / 2)); + outputs.insert(output_addr_2, Some(asset_lock_credits / 2)); + + // Fee strategy targets ReduceOutput(2) — originally the remainder position. + // After remainder is removed (outputs shrink from 3 to 2), index 2 is OOB. + // Fee deduction may silently skip, giving a free transaction. + let transition = create_signed_address_funding_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + &signer, + BTreeMap::new(), // No address inputs + outputs, + vec![AddressFundsFeeStrategyStep::ReduceOutput(2)], + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &[result], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + + // The transition should either: + // 1. Be rejected because the fee strategy references an invalid index, OR + // 2. Have fees properly deducted from a different output. + // It should NOT succeed with zero fees paid. + match processing_result.execution_results().as_slice() { + [StateTransitionExecutionResult::SuccessfulExecution { .. }] => { + // If it succeeded, verify fees were actually deducted + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + let bal_1 = platform + .drive + .fetch_balance_and_nonce(&output_addr_1, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + let bal_2 = platform + .drive + .fetch_balance_and_nonce(&output_addr_2, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + let total_output = bal_1 + bal_2; + + // If fees were properly deducted, total_output < asset_lock_credits + assert!( + total_output < asset_lock_credits, + "AUDIT M2: Transaction succeeded but total output ({}) equals \ + asset lock credits ({}). Fees were not deducted because \ + ReduceOutput(2) targeted the removed remainder position. \ + The fee deduction was silently skipped, giving a free transaction.", + total_output, + asset_lock_credits, + ); + } + _ => { + // Rejected — this is acceptable behavior (fee strategy validation caught it) + } + } + } + + /// AUDIT M5: No explicit conservation-of-credits check (asset lock only). + /// + /// This test verifies credit conservation when using only an asset lock + /// (no address inputs). All asset lock credits should end up in outputs + fees. + /// This is a standalone conservation test complementing C2. + /// + /// Location: rs-drive/.../address_funding_from_asset_lock_transition.rs + #[test] + fn test_credits_conservation_with_asset_lock_only() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let signer = TestAddressSigner::new(); + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + let asset_lock_credits = dash_to_credits!(1.0); + + // Pure asset lock funding — no address inputs + let output_addr = create_platform_address(1); + let remainder_addr = create_platform_address(2); + + let explicit_amount = dash_to_credits!(0.3); + let mut outputs = BTreeMap::new(); + outputs.insert(output_addr, Some(explicit_amount)); + outputs.insert(remainder_addr, None); // remainder + + let transition = create_signed_address_funding_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + &signer, + BTreeMap::new(), // No address inputs + outputs, + vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &[result], + &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 { .. }], + "Pure asset lock funding should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + // Read final balances + let output_final = platform + .drive + .fetch_balance_and_nonce(&output_addr, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + let remainder_final = platform + .drive + .fetch_balance_and_nonce(&remainder_addr, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + let total_in_addresses = output_final + remainder_final; + + // Conservation: all asset lock credits should be in outputs + fees + // total_in_addresses + fees_paid = asset_lock_credits + // So: total_in_addresses <= asset_lock_credits + // total_in_addresses >= asset_lock_credits - max_reasonable_fee + let max_reasonable_fee = dash_to_credits!(0.01); + + assert!( + total_in_addresses >= asset_lock_credits - max_reasonable_fee, + "AUDIT M5: Credits not conserved. Asset lock provided {} credits, \ + but output addresses only received {} credits total. \ + Expected at least {} (asset_lock - max_fee {}). \ + There is no explicit conservation-of-credits assertion in the processing code.", + asset_lock_credits, + total_in_addresses, + asset_lock_credits - max_reasonable_fee, + max_reasonable_fee, + ); + + assert!( + total_in_addresses <= asset_lock_credits, + "AUDIT M5: Output total {} exceeds asset lock credits {}. \ + Credits were created from nothing!", + total_in_addresses, + asset_lock_credits, + ); + } + + /// AUDIT M3: Remainder arithmetic uses unchecked operations. + /// + /// At `address_funding_from_asset_lock/mod.rs:61,64`, the transformer uses + /// `.sum()` and unchecked subtraction for remainder computation. If structure + /// validation is bypassed, these operations could wrap/underflow. + /// + /// This test verifies that structure validation catches the overflow at the + /// correct level, but notes the transformer lacks defense-in-depth. + /// + /// Location: rs-drive/.../address_funding_from_asset_lock/mod.rs:61,64 + #[test] + fn test_remainder_arithmetic_uses_checked_operations() { + use crate::execution::validation::state_transition::processor::traits::basic_structure::StateTransitionBasicStructureValidationV0; + + let platform_version = PlatformVersion::latest(); + + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, _asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + + // Create outputs that sum to > u64::MAX, with a remainder output + let output_addr_1 = create_platform_address(1); + let output_addr_2 = create_platform_address(2); + let remainder_addr = create_platform_address(3); + + let mut outputs = BTreeMap::new(); + outputs.insert(output_addr_1, Some(u64::MAX)); + outputs.insert(output_addr_2, Some(u64::MAX)); + outputs.insert(remainder_addr, None); // Remainder output + + let transition = create_raw_transition_with_dummy_witnesses( + asset_lock_proof, + BTreeMap::new(), + outputs, + AddressFundsFeeStrategy::from(vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]), + 0, + ); + + let result = transition + .validate_basic_structure(dpp::dashcore::Network::Testnet, platform_version) + .expect("validation should not return Err"); + + assert!(!result.is_valid()); + assert_matches!( + result.first_error().unwrap(), + ConsensusError::BasicError(BasicError::OverflowError(_)) + ); + } + + /// AUDIT C2: Remainder calculation ignores input contributions — funds destroyed. + /// + /// When a transition combines an asset lock with existing address inputs, + /// the remainder is computed as `asset_lock_balance - explicit_outputs_sum`, + /// completely ignoring the input contributions. The input funds are deducted + /// from the source address but never routed to any output — credits are + /// permanently destroyed. + /// + /// Locations: + /// - rs-drive/.../address_funding_from_asset_lock/mod.rs:64 (resolved_outputs) + /// - rs-drive/.../address_funding_from_asset_lock_transition.rs:61 (operations) + #[test] + fn test_remainder_includes_input_contributions() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let input_address = signer.add_p2pkh([1u8; 32]); + let input_balance = dash_to_credits!(0.5); + let input_amount = dash_to_credits!(0.3); + setup_address_with_balance(&mut platform, input_address, 0, input_balance); + + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + // Asset lock provides ~1.0 Dash of credits + + // Combine asset lock + existing address input, all going to remainder + let mut inputs = BTreeMap::new(); + inputs.insert(input_address, (1 as AddressNonce, input_amount)); + + let remainder_address = create_platform_address(2); + let mut outputs = BTreeMap::new(); + outputs.insert(remainder_address, None); // Single remainder output + + let transition = create_signed_address_funding_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + &signer, + inputs, + outputs, + vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &[result], + &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 { .. }], + "Transaction should succeed" + ); + + // Commit so we can read final balances + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + // Check remainder balance + let remainder_result = platform + .drive + .fetch_balance_and_nonce(&remainder_address, None, platform_version) + .expect("should fetch"); + + let remainder_balance = remainder_result.map(|(_, balance)| balance).unwrap_or(0); + + // Expected: asset_lock (~1.0 Dash) + input (0.3 Dash) - fees + // = ~1.3 Dash - small_fee > 1.2 Dash + // Actual (BUG): asset_lock (~1.0 Dash) - fees < 1.0 Dash + // The 0.3 Dash from the input address is deducted but never added + // to any output — those credits are permanently destroyed. + assert!( + remainder_balance > dash_to_credits!(1.2), + "AUDIT C2: Remainder should include input contributions. \ + Expected > {} credits (~1.3 Dash - fees), got {} credits. \ + The input contribution of {} credits ({} Dash) was deducted from \ + the source address but never routed to any output — destroyed.", + dash_to_credits!(1.2), + remainder_balance, + input_amount, + "0.3" + ); + } + + /// AUDIT C2 (conservation): Verify total credits are conserved. + /// + /// After processing an asset-lock-with-inputs transition, the total + /// credits across all affected addresses plus the withdrawal document + /// should equal the initial credits. Currently, input contributions + /// vanish, violating conservation. + #[test] + fn test_credits_conservation_with_inputs() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let input_address = signer.add_p2pkh([1u8; 32]); + let input_balance = dash_to_credits!(1.0); + let input_amount = dash_to_credits!(0.5); + setup_address_with_balance(&mut platform, input_address, 0, input_balance); + + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + let asset_lock_credits = dash_to_credits!(1.0); // ~1 Dash from fixture + + let mut inputs = BTreeMap::new(); + inputs.insert(input_address, (1 as AddressNonce, input_amount)); + + let explicit_address = create_platform_address(2); + let remainder_address = create_platform_address(3); + let explicit_amount = dash_to_credits!(0.2); + + let mut outputs = BTreeMap::new(); + outputs.insert(explicit_address, Some(explicit_amount)); + outputs.insert(remainder_address, None); + + let transition = create_signed_address_funding_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + &signer, + inputs, + outputs, + vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &[result], + &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 { .. }], + "Transaction should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + // Read all final balances + let _input_final = platform + .drive + .fetch_balance_and_nonce(&input_address, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + let explicit_final = platform + .drive + .fetch_balance_and_nonce(&explicit_address, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + let remainder_final = platform + .drive + .fetch_balance_and_nonce(&remainder_address, None, platform_version) + .expect("should fetch") + .map(|(_, b)| b) + .unwrap_or(0); + + // Total credits in = asset_lock + input_amount + let total_credits_in = asset_lock_credits + input_amount; + + // Total credits out = input_remaining + explicit + remainder + fees + // input_remaining = input_balance - input_amount + let _input_remaining = input_balance - input_amount; + let total_credits_in_addresses = explicit_final + remainder_final; + + // The output addresses should have received total_credits_in minus fees. + // input_final should equal input_remaining minus any fee deducted from it. + // Conservation: explicit_final + remainder_final >= total_credits_in - max_reasonable_fee + let max_reasonable_fee = dash_to_credits!(0.01); // Fees should be small + + assert!( + total_credits_in_addresses >= total_credits_in - max_reasonable_fee, + "AUDIT C2: Credits not conserved. Input contributed {} credits but \ + output addresses only received {} credits total. Expected at least {} \ + (total_in {} - max_fee {}). The input contribution was destroyed.", + input_amount, + total_credits_in_addresses, + total_credits_in - max_reasonable_fee, + total_credits_in, + max_reasonable_fee, + ); + } + } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs index ac3a14621aa..034a7992d53 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs @@ -14,6 +14,7 @@ mod tests { }; use dpp::block::block_info::BlockInfo; use dpp::consensus::basic::BasicError; + use dpp::consensus::signature::SignatureError; use dpp::consensus::state::state_error::StateError; use dpp::consensus::ConsensusError; use dpp::dash_to_credits; @@ -382,7 +383,9 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::BasicError(BasicError::InputWitnessCountMismatchError(_)) + ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ) )] ); } @@ -5237,10 +5240,12 @@ mod tests { // Should fail with some error (not panic) assert!(!processing_result.execution_results().is_empty()); - assert!(!matches!( - processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - )); + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] + ); } } @@ -6100,12 +6105,11 @@ mod tests { .expect("expected to process"); // Should fail - timelock scripts should not be accepted - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Timelock (CLTV) script should not be accepted" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -6176,12 +6180,11 @@ mod tests { .expect("expected to process"); // Should fail - either invalid script format or signature verification fails - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "OP_RETURN script should not be accepted" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -6565,12 +6568,11 @@ mod tests { .expect("expected to process"); // MUST fail - OP_TRUE script without proper multisig structure is not valid - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "OP_TRUE script should NOT be accepted - this would be a critical vulnerability!" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -6639,12 +6641,11 @@ mod tests { ) .expect("expected to process"); - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "OP_1 script should NOT be accepted" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -6831,12 +6832,11 @@ mod tests { ) .expect("expected to process"); - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Disabled opcode OP_CAT should not be accepted" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -6905,12 +6905,11 @@ mod tests { ) .expect("expected to process"); - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Disabled opcode OP_VER should not be accepted" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -6986,12 +6985,11 @@ mod tests { .expect("expected to process"); // Large scripts should be rejected (or at least the OP_1 should fail validation) - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Very large redeem script should be rejected" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -7187,12 +7185,11 @@ mod tests { .expect("expected to process"); // Non-canonical DER should be rejected - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Non-canonical DER signature should be rejected" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -7262,12 +7259,11 @@ mod tests { .expect("expected to process"); // Empty script should fail (leaves empty stack) - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Empty script should be rejected" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -7339,12 +7335,11 @@ mod tests { .expect("expected to process"); // OP_CODESEPARATOR in non-standard script should be rejected - assert!( - !matches!( - &processing_result.execution_results()[0], - StateTransitionExecutionResult::SuccessfulExecution { .. } - ), - "Script with OP_CODESEPARATOR should be rejected" + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::BasicError(BasicError::SerializedObjectParsingError(_)) + )] ); } @@ -8291,4 +8286,397 @@ mod tests { ); } } + + mod security_edge_cases { + use super::*; + use dpp::consensus::signature::SignatureError; + + /// AUDIT H1: Witness validation uses zip() without count check. + /// + /// `validate_witnesses` pairs inputs with witnesses using `zip()`, which + /// silently stops at the shorter iterator. With 0 witnesses on 2 inputs, + /// zip produces 0 iterations and validation "passes" (no errors). + /// The witness count mismatch is only caught later by structure validation. + /// + /// This test verifies that the PROCESSING pipeline rejects a transition + /// with fewer witnesses than inputs as an UNPAID error (meaning the + /// witness validation stage caught it, not the structure validation stage). + /// + /// Location: rs-dpp/.../state_transition_witness_validation.rs:56-60 + #[test] + fn test_zero_witnesses_rejected_by_witness_validation() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let addr_a = signer.add_p2pkh([1u8; 32]); + let addr_b = signer.add_p2pkh([2u8; 32]); + + setup_address_with_balance(&mut platform, addr_a, 0, dash_to_credits!(1.0)); + setup_address_with_balance(&mut platform, addr_b, 0, dash_to_credits!(1.0)); + + // Create a transition with 2 inputs but 0 witnesses. + // The validate_witnesses zip() produces 0 iterations → "passes". + // Only structure validation catches the mismatch later. + let mut inputs = BTreeMap::new(); + inputs.insert(addr_a, (1 as AddressNonce, dash_to_credits!(0.1))); + inputs.insert(addr_b, (1 as AddressNonce, dash_to_credits!(0.1))); + + let mut outputs = BTreeMap::new(); + outputs.insert(create_platform_address(10), dash_to_credits!(0.2)); + + // Construct transition with 0 witnesses (not matching 2 inputs) + let transition = create_raw_transition_with_dummy_witnesses( + inputs, + outputs, + AddressFundsFeeStrategy::from(vec![AddressFundsFeeStrategyStep::DeductFromInput( + 0, + )]), + 0, // 0 witnesses for 2 inputs + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + + // The transition is correctly rejected. But the question is: by WHICH + // validation stage? If witness validation caught it (correct), it would + // be an InvalidStateTransitionSignatureError (unpaid). If structure + // validation caught it (current buggy behavior), it would be + // InputWitnessCountMismatchError (also unpaid). + // + // We assert it should be a signature error (from witness validation), + // not a count mismatch error (from structure validation that runs later). + // Currently FAILS: witness validation passes with 0 iterations, + // and structure validation catches it as InputWitnessCountMismatchError. + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ) + )], + "AUDIT H1: With 0 witnesses on 2 inputs, witness validation should catch \ + the mismatch and return InvalidStateTransitionSignatureError. Currently, \ + zip() produces 0 iterations so witness validation passes, and the error \ + is caught later by structure validation as InputWitnessCountMismatchError. \ + Got: {:?}", + processing_result.execution_results() + ); + } + + /// AUDIT M4: Credits (u64) cast to SumValue (i64) truncation destroys funds. + /// + /// When setting an address balance, `set_balance_to_address_operations_v0` + /// casts `balance as SumValue` where SumValue is i64. For balance values + /// > i64::MAX, this would produce a negative value. The Drive layer now + /// guards against this with an overflow check, returning an error instead + /// of silently truncating. + /// + /// In practice this can never happen because MAX_CREDITS == i64::MAX and + /// all input validation enforces this, but the Drive layer should still + /// reject such values defensively. + /// + /// Location: rs-drive/src/drive/address_funds/set_balance_to_address/v0/mod.rs + #[test] + fn test_balance_exceeding_i64_max_returns_overflow_error() { + let platform_version = PlatformVersion::latest(); + + let platform = TestPlatformBuilder::new() + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let address = PlatformAddress::P2pkh([1u8; 20]); + let large_balance: u64 = i64::MAX as u64 + 1; // 9_223_372_036_854_775_808 + + // Attempting to set a balance > i64::MAX should return an overflow error + let mut drive_operations = Vec::new(); + let result = platform.drive.set_balance_to_address( + address, + 0, + large_balance, + &mut None, + &mut drive_operations, + platform_version, + ); + + assert!( + result.is_err(), + "AUDIT M4: set_balance_to_address should reject balance {} (> i64::MAX = {}) \ + with an overflow error to prevent u64→i64 truncation in sum tree storage.", + large_balance, + i64::MAX, + ); + + // Verify that i64::MAX itself is accepted (boundary value) + let mut drive_operations = Vec::new(); + let result = platform.drive.set_balance_to_address( + address, + 0, + i64::MAX as u64, + &mut None, + &mut drive_operations, + platform_version, + ); + + assert!( + result.is_ok(), + "set_balance_to_address should accept balance == i64::MAX" + ); + } + + /// AUDIT L2: Nonce overflow at u32::MAX locks address permanently. + /// + /// `AddressNonce` is defined as `u32` (rs-dpp/src/lib.rs:117), meaning an + /// address can be used at most ~4.3 billion times. After nonce reaches u32::MAX, + /// no further transactions are possible — the address is permanently locked. + /// + /// This test sets up an address with nonce at u32::MAX - 1, performs one + /// transaction (nonce becomes u32::MAX), then attempts another transaction + /// to verify the address cannot transact further. + /// + /// Location: rs-dpp/src/lib.rs:117 + #[test] + fn test_nonce_at_u32_max_locks_address() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let input_address = signer.add_p2pkh([1u8; 32]); + + // Set up address with nonce at u32::MAX - 1 and large balance + let nonce_near_max: AddressNonce = u32::MAX - 1; + setup_address_with_balance( + &mut platform, + input_address, + nonce_near_max, + dash_to_credits!(10.0), + ); + + let recipient = create_platform_address(2); + + // First transaction: nonce = u32::MAX (should succeed) + let transition1 = create_signed_address_funds_transfer_transition( + &signer, + input_address, + u32::MAX, // nonce = u32::MAX + dash_to_credits!(0.01), + recipient, + dash_to_credits!(0.01), + ); + + let result1 = transition1.serialize_to_bytes().expect("should serialize"); + let platform_state = platform.state.load(); + let transaction1 = platform.drive.grove.start_transaction(); + + let processing_result1 = platform + .platform + .process_raw_state_transitions( + &vec![result1], + &platform_state, + &BlockInfo::default(), + &transaction1, + platform_version, + false, + None, + ) + .expect("expected to process"); + + // This transaction should succeed (nonce u32::MAX is valid) + assert_matches!( + processing_result1.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "Transaction with nonce u32::MAX should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction1) + .unwrap() + .expect("should commit"); + + // After this, the address nonce is at u32::MAX. + // Any further transaction would need nonce = u32::MAX + 1 = overflow. + // The address is effectively locked forever. + // This test documents the behavior — it's a low-severity issue since + // 4.3 billion transactions per address is practically unreachable, + // but there's no graceful handling or error message. + // + // Note: We can't easily test the "next" transaction because we'd need + // nonce u32::MAX + 1 which wraps to 0 and would be rejected as a + // nonce reuse. This is documented as a design limitation. + } + + /// AUDIT M1: Fee deduction BTreeMap index shifting after entry removal. + /// + /// When fee strategy step DeductFromInput(0) drains input A to zero, + /// A is removed from the BTreeMap. The next step DeductFromInput(1) + /// now targets what was originally at index 2 (C) instead of index 1 (B), + /// because all indices shifted down after the removal. + /// + /// Location: rs-dpp/.../deduct_fee_from_inputs_and_outputs/v0/mod.rs:35-45 + #[test] + fn test_fee_deduction_stable_after_entry_removal() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let addr_a = signer.add_p2pkh([10u8; 32]); + let addr_b = signer.add_p2pkh([20u8; 32]); + let addr_c = signer.add_p2pkh([30u8; 32]); + + // Determine BTreeMap sort order (PlatformAddress sorts by hash) + let mut sorted_addrs = vec![addr_a, addr_b, addr_c]; + sorted_addrs.sort(); + let first = sorted_addrs[0]; // BTreeMap index 0 + let second = sorted_addrs[1]; // BTreeMap index 1 + let third = sorted_addrs[2]; // BTreeMap index 2 + + // Set up balances so that "first" has tiny remaining balance after transfer + let first_balance = dash_to_credits!(0.1); + let second_balance = dash_to_credits!(1.0); + let third_balance = dash_to_credits!(1.0); + + // Input amount leaves only 1000 credits remaining for first + let first_input = first_balance - 1000; + let second_input = dash_to_credits!(0.01); + let third_input = dash_to_credits!(0.01); + + setup_address_with_balance(&mut platform, first, 0, first_balance); + setup_address_with_balance(&mut platform, second, 0, second_balance); + setup_address_with_balance(&mut platform, third, 0, third_balance); + + let total_transfer = first_input + second_input + third_input; + + let mut inputs = BTreeMap::new(); + inputs.insert(first, (1 as AddressNonce, first_input)); + inputs.insert(second, (1 as AddressNonce, second_input)); + inputs.insert(third, (1 as AddressNonce, third_input)); + + let mut outputs = BTreeMap::new(); + outputs.insert(create_platform_address(100), total_transfer); + + // Fee strategy: deduct from index 0 (first), then index 1 (should be second). + // Bug: after first is drained and removed, index 1 becomes third. + let fee_strategy = vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + AddressFundsFeeStrategyStep::DeductFromInput(1), + ]; + + let transition = create_signed_transition_with_custom_outputs( + &signer, + inputs, + outputs, + fee_strategy, + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &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 { .. }], + "Transaction should succeed" + ); + + // Commit to read final balances + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + // Check that fees were deducted from SECOND (index 1), not THIRD. + // remaining_before_fee for second = second_balance - second_input + let second_remaining_before_fee = second_balance - second_input; + + let (_, second_final) = platform + .drive + .fetch_balance_and_nonce(&second, None, platform_version) + .expect("should fetch") + .expect("second address should exist"); + + // If fee was correctly deducted from second (BTreeMap index 1 before mutation), + // second_final should be LESS than second_remaining_before_fee. + // With the bug, second is untouched and third gets the fee instead. + assert!( + second_final < second_remaining_before_fee, + "AUDIT M1: Fee should have been deducted from second address (original \ + BTreeMap index 1), but it was deducted from third address instead. \ + After first was drained (1000 credits) and removed from BTreeMap, \ + DeductFromInput(1) shifted to target the third address. \ + second's balance: {} (expected < {})", + second_final, + second_remaining_before_fee + ); + } + } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/mod.rs index 57e7db27fe7..90d7f68499d 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/mod.rs @@ -2,6 +2,8 @@ mod advanced_structure; mod basic_structure; pub(crate) mod public_key_signatures; mod state; +#[cfg(test)] +mod tests; use crate::error::execution::ExecutionError; use crate::error::Error; diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/tests.rs index 76d15b21ac4..7da09b71e4d 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/tests.rs @@ -603,10 +603,9 @@ mod tests { // and fails when trying to validate the missing second witness assert_matches!( check_result.errors.as_slice(), - [ConsensusError::SignatureError(_)] - | [ConsensusError::BasicError( - BasicError::InputWitnessCountMismatchError(_) - )] + [ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + )] ); } @@ -1036,7 +1035,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); } @@ -1135,7 +1134,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); } @@ -1240,7 +1239,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); } } @@ -1323,7 +1322,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); // Commit the transaction @@ -1441,7 +1440,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); // Commit the transaction @@ -1538,7 +1537,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); // Commit the transaction @@ -1898,7 +1897,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); // Commit the first transaction @@ -2035,7 +2034,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); // Commit the first transaction @@ -2294,7 +2293,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }] + [StateTransitionExecutionResult::SuccessfulExecution { .. }] ); // Verify the identity was actually created and funded @@ -2799,7 +2798,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }], + [StateTransitionExecutionResult::SuccessfulExecution { .. }], "Expected valid structure, got {:?}", processing_result.execution_results() ); @@ -2997,7 +2996,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }], + [StateTransitionExecutionResult::SuccessfulExecution { .. }], "Expected valid structure with output, got {:?}", processing_result.execution_results() ); @@ -3595,7 +3594,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }], + [StateTransitionExecutionResult::SuccessfulExecution { .. }], "Expected valid structure with single master key, got {:?}", processing_result.execution_results() ); @@ -3670,7 +3669,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }], + [StateTransitionExecutionResult::SuccessfulExecution { .. }], "Expected valid structure with multiple keys, got {:?}", processing_result.execution_results() ); @@ -3745,7 +3744,7 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution{ .. }], + [StateTransitionExecutionResult::SuccessfulExecution { .. }], "Expected valid structure with max keys, got {:?}", processing_result.execution_results() ); @@ -8840,8 +8839,13 @@ mod tests { dash_to_credits!(10.0), ); - // Set up output address with near-max balance - setup_address_with_balance(&mut platform, output_address.clone(), 0, u64::MAX - 1000); + // Set up output address with near-max balance (leave room for other balances in sum tree) + setup_address_with_balance( + &mut platform, + output_address.clone(), + 0, + i64::MAX as u64 - dash_to_credits!(1000.0), + ); let mut inputs = BTreeMap::new(); inputs.insert( @@ -11316,4 +11320,400 @@ mod tests { ); } } + + mod security { + use super::*; + use dpp::state_transition::StateTransitionStructureValidation; + + /// AUDIT M1: Fee deduction BTreeMap index shifting after entry removal. + /// + /// When fee strategy step DeductFromInput(0) drains input A to zero, + /// A is removed from the BTreeMap. The next step DeductFromInput(1) + /// now targets what was originally at index 2 (C) instead of index 1 (B), + /// because all indices shifted down after the removal. + /// + /// Location: rs-dpp/.../deduct_fee_from_inputs_and_outputs/v0/mod.rs:35-45 + #[test] + fn test_fee_deduction_stable_after_entry_removal() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut rng = StdRng::seed_from_u64(567); + + // Create identity + let (identity, identity_signer) = + create_identity_with_keys([1u8; 32], &mut rng, platform_version); + + let mut address_signer = TestAddressSigner::new(); + let addr_a = address_signer.add_p2pkh([10u8; 32]); + let addr_b = address_signer.add_p2pkh([20u8; 32]); + let addr_c = address_signer.add_p2pkh([30u8; 32]); + + // Determine BTreeMap sort order + let mut sorted_addrs = vec![addr_a, addr_b, addr_c]; + sorted_addrs.sort(); + let first = sorted_addrs[0]; + let second = sorted_addrs[1]; + let third = sorted_addrs[2]; + + let first_balance = dash_to_credits!(0.1); + let second_balance = dash_to_credits!(1.0); + let third_balance = dash_to_credits!(1.0); + + // Input amount leaves only 1000 credits remaining for first + let first_input = first_balance - 1000; + let second_input = dash_to_credits!(0.01); + let third_input = dash_to_credits!(0.01); + + setup_address_with_balance(&mut platform, first, 0, first_balance); + setup_address_with_balance(&mut platform, second, 0, second_balance); + setup_address_with_balance(&mut platform, third, 0, third_balance); + + let mut inputs = BTreeMap::new(); + inputs.insert(first, (1 as AddressNonce, first_input)); + inputs.insert(second, (1 as AddressNonce, second_input)); + inputs.insert(third, (1 as AddressNonce, third_input)); + + // Fee strategy: deduct from index 0 (first), then index 1 (should be second). + // Bug: after first is drained and removed, index 1 becomes third. + let fee_strategy = AddressFundsFeeStrategy::from(vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + AddressFundsFeeStrategyStep::DeductFromInput(1), + ]); + + let transition = create_signed_identity_create_from_addresses_transition_full( + &identity, + &address_signer, + &identity_signer, + inputs, + None, // No output + fee_strategy, + platform_version, + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &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 { .. }], + "Transaction should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + // Verify first address was fully drained (exercising the index-shift scenario) + let first_result = platform + .drive + .fetch_balance_and_nonce(&first, None, platform_version) + .expect("should fetch"); + assert!( + first_result.is_none() || first_result.map(|(_, b)| b) == Some(0), + "First address should be fully drained to exercise index-shift scenario" + ); + + let second_remaining_before_fee = second_balance - second_input; + + let (_, second_final) = platform + .drive + .fetch_balance_and_nonce(&second, None, platform_version) + .expect("should fetch") + .expect("second address should exist"); + + assert!( + second_final < second_remaining_before_fee, + "AUDIT M1: Fee should have been deducted from second address (original \ + BTreeMap index 1), but it was deducted from third address instead. \ + After first was drained (1000 credits) and removed from BTreeMap, \ + DeductFromInput(1) shifted to target the third address. \ + second's balance: {} (expected < {})", + second_final, + second_remaining_before_fee + ); + } + + /// AUDIT M3: Unchecked subtraction in identity_create_from_addresses transformer. + /// + /// At `transformer.rs:39`, the transformer uses `.sum()` (wrapping) and at + /// line 43 uses unchecked subtraction for computing the amount to fund the + /// new identity. If structure validation is bypassed, these operations could + /// wrap/underflow silently. + /// + /// This test verifies that structure validation catches overflow at the + /// correct level, but notes the transformer lacks defense-in-depth. + /// + /// Location: rs-drive/.../identity_create_from_addresses/v0/transformer.rs:39,43 + #[test] + fn test_transformer_subtraction_uses_checked_arithmetic() { + use crate::execution::validation::state_transition::processor::traits::basic_structure::StateTransitionBasicStructureValidationV0; + + let platform_version = PlatformVersion::latest(); + let mut rng = StdRng::seed_from_u64(999); + + let public_keys = create_default_public_keys(&mut rng, platform_version); + + // Two inputs that sum to > u64::MAX + let mut inputs = BTreeMap::new(); + inputs.insert(create_platform_address(1), (0 as AddressNonce, u64::MAX)); + inputs.insert(create_platform_address(2), (0 as AddressNonce, u64::MAX)); + + let transition = create_raw_transition_with_dummy_witnesses( + public_keys, + inputs, + None, + AddressFundsFeeStrategy::from(vec![AddressFundsFeeStrategyStep::DeductFromInput( + 0, + )]), + 2, + ); + + let result = transition + .validate_basic_structure(dpp::dashcore::Network::Testnet, platform_version) + .expect("validation should not return Err"); + + assert!(!result.is_valid()); + assert_matches!( + result.first_error().unwrap(), + ConsensusError::BasicError(BasicError::OverflowError(_)) + ); + } + + /// AUDIT M8: Fee deduction doesn't check fee_fully_covered. + /// + /// When processing IdentityCreateFromAddresses, the execution path deducts + /// fees but never checks whether `fee_fully_covered` is true. If the actual + /// fee exceeds the estimated fee, partial payment occurs — the validator + /// subsidizes the difference. + /// + /// This test creates a transition where address balances are just barely + /// enough for the transfer amount but insufficient to cover fees. + /// + /// Location: rs-drive-abci/.../identity_create_from_addresses (fee deduction logic) + #[test] + fn test_partial_fee_payment_rejected() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut rng = StdRng::seed_from_u64(567); + + let (identity, identity_signer) = + create_identity_with_keys([1u8; 32], &mut rng, platform_version); + + let mut address_signer = TestAddressSigner::new(); + let input_address = address_signer.add_p2pkh([1u8; 32]); + + // Set up address with a very small balance — just enough for a tiny transfer + // but not enough to also cover processing fees + let min_output = platform_version + .dpp + .state_transitions + .address_funds + .min_output_amount; + let tiny_balance = min_output + 1; // Just barely above minimum + + setup_address_with_balance(&mut platform, input_address, 0, tiny_balance); + + let mut inputs = BTreeMap::new(); + inputs.insert(input_address, (1 as AddressNonce, tiny_balance)); + + // All input goes to identity creation, nothing left for fees + let transition = create_signed_identity_create_from_addresses_transition( + &identity, + &address_signer, + &identity_signer, + inputs, + None, + Some(AddressFundsFeeStrategy::from(vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + ])), + platform_version, + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + + // The transition should either: + // 1. Be rejected because insufficient funds for fees, OR + // 2. Succeed with fees fully deducted from the input + // + // What it should NOT do is succeed with partial fee payment. + // The fee_fully_covered flag should be checked. + match processing_result.execution_results().as_slice() { + [StateTransitionExecutionResult::SuccessfulExecution { .. }] => { + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + // If it succeeded, verify the input was fully consumed + let (_, input_final) = platform + .drive + .fetch_balance_and_nonce(&input_address, None, platform_version) + .expect("should fetch") + .unwrap_or((0, 0)); + + // The input address should have 0 balance (all consumed) + // If fee_fully_covered was not checked, the input might still + // have some balance because fees were only partially deducted. + assert!( + input_final == 0, + "AUDIT M8: Identity creation succeeded but input address still has \ + {} credits remaining. This suggests fees were not fully deducted — \ + fee_fully_covered was not checked. The validator subsidized the \ + remaining fee cost.", + input_final, + ); + } + _ => { + // Rejected — acceptable behavior (insufficient funds for fees) + } + } + } + + /// AUDIT L4: Identity ID derivation lacks domain separator. + /// + /// `identity_id_from_inputs()` derives identity ID from input addresses and nonces + /// using double hashing, but does NOT include the transition type in the hash. + /// This means different transition types with identical inputs would produce + /// the same identity ID, creating potential cross-transition collisions. + /// + /// Location: rs-dpp/.../state_transition_identity_id_from_inputs.rs + #[test] + fn test_identity_id_has_no_domain_separator() { + use dpp::state_transition::StateTransitionIdentityIdFromInputs; + + let platform_version = PlatformVersion::latest(); + let mut rng = StdRng::seed_from_u64(567); + + // Create an identity and address signer + let (identity, identity_signer) = + create_identity_with_keys([1u8; 32], &mut rng, platform_version); + + let mut address_signer = TestAddressSigner::new(); + let addr = address_signer.add_p2pkh([1u8; 32]); + + let mut inputs = BTreeMap::new(); + inputs.insert(addr, (1 as AddressNonce, dash_to_credits!(1.0))); + + // Create an IdentityCreateFromAddresses transition + let transition = create_signed_identity_create_from_addresses_transition( + &identity, + &address_signer, + &identity_signer, + inputs.clone(), + None, + None, + platform_version, + ); + + // Get the identity ID from the transition + let create_id = match &transition { + StateTransition::IdentityCreateFromAddresses(t) => t + .identity_id_from_inputs() + .expect("should derive identity ID"), + _ => panic!("Expected IdentityCreateFromAddresses"), + }; + + // The identity ID is derived purely from input addresses and nonces. + // If a different transition type (e.g., IdentityTopUpFromAddresses) used + // the same inputs, it would produce the same ID because no transition + // type discriminator is included in the hash. + // + // We verify the ID is deterministic (same inputs → same ID) + let transition2 = create_signed_identity_create_from_addresses_transition( + &identity, + &address_signer, + &identity_signer, + inputs, + None, + None, + platform_version, + ); + + let create_id_2 = match &transition2 { + StateTransition::IdentityCreateFromAddresses(t) => t + .identity_id_from_inputs() + .expect("should derive identity ID"), + _ => panic!("Expected IdentityCreateFromAddresses"), + }; + + // Same inputs → same ID (deterministic) + assert_eq!( + create_id, create_id_2, + "Identity ID should be deterministic for same inputs" + ); + + // The vulnerability: the hash does NOT include the transition type. + // If someone creates a different transition type with the same inputs, + // they get the same identity ID. This is documented as a low-severity + // issue since the practical impact is limited (different transition types + // are processed differently and the collision doesn't lead to exploits + // in the current codebase). + // + // To fix: include a domain separator (transition type byte) in the hash input. + // e.g., hash(0x01 || address_nonce_data) for creates + // hash(0x02 || address_nonce_data) for topups + } + } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_transfer_to_addresses/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_transfer_to_addresses/tests.rs index 5d099279aac..f98f0d1e10e 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_transfer_to_addresses/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_transfer_to_addresses/tests.rs @@ -234,6 +234,198 @@ mod tests { } } + mod security { + use super::*; + use dpp::state_transition::StateTransitionStructureValidation; + + /// AUDIT M9: Unchecked `.sum()` wrapping in drive operations. + /// + /// At `identity_credit_transfer_to_addresses_transition.rs:38`, + /// `recipient_addresses.values().sum()` uses wrapping arithmetic. + /// If the sum overflows, `RemoveFromIdentityBalance` removes a tiny + /// amount while each recipient gets credited fully — credits from nothing. + /// + /// Structure validation at the DPP layer uses `checked_add` and catches + /// this, but the drive operation lacks its own defense-in-depth check. + /// + /// This test verifies structure validation catches the overflow. + /// + /// Location: rs-drive/.../identity_credit_transfer_to_addresses_transition.rs:38 + #[test] + fn test_recipient_sum_overflow_returns_error() { + let platform_version = PlatformVersion::latest(); + + // Create two recipients whose amounts sum to > u64::MAX + let mut recipient_addresses = BTreeMap::new(); + recipient_addresses.insert(create_platform_address(1), u64::MAX); + recipient_addresses.insert(create_platform_address(2), u64::MAX); + + let transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { + identity_id: [1u8; 32].into(), + recipient_addresses, + nonce: 1, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: BinaryData::new(vec![0; 65]), + }; + + // Structure validation should catch the overflow + let result = transition_v0.validate_structure(platform_version); + + assert!( + !result.is_valid(), + "AUDIT M9: Two recipients of u64::MAX should cause overflow in structure \ + validation. The DPP layer uses checked_add and catches this. However, the \ + drive operation at identity_credit_transfer_to_addresses_transition.rs:38 \ + uses .sum() which would silently wrap, allowing credit creation from nothing." + ); + + let has_overflow = result + .errors + .iter() + .any(|e| matches!(e, ConsensusError::BasicError(BasicError::OverflowError(_)))); + assert!( + has_overflow, + "AUDIT M9: Expected OverflowError, got {:?}", + result.errors + ); + } + + /// AUDIT L6: transform_into_action performs zero validation. + /// + /// `transform_into_action_v0` at `transform_into_action/v0/mod.rs:16-23` + /// simply wraps the transition into an action with zero checks. No balance + /// validation, no nonce validation, no recipient validation occurs in the + /// transformer — it relies entirely on upstream and downstream checks. + /// + /// This test demonstrates that any data passes through unchecked. + /// + /// Location: rs-drive-abci/.../identity_credit_transfer_to_addresses/transform_into_action/v0/mod.rs:16-23 + #[test] + fn test_transform_into_action_passes_without_validation() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let mut rng = StdRng::seed_from_u64(567); + + // Create identity with zero balance + let (identity, signer) = create_identity_with_transfer_key( + [1u8; 32], + 0, // Zero balance + &mut rng, + platform_version, + ); + + add_identity_to_drive(&mut platform, &identity); + + // Create transfer that exceeds balance (identity has 0) + let mut recipient_addresses = BTreeMap::new(); + recipient_addresses.insert(create_platform_address(1), dash_to_credits!(1.0)); + + let transition = create_signed_transition(&identity, &signer, recipient_addresses, 1); + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process"); + + // The transition should be rejected due to insufficient balance. + // This rejection happens at the drive operation level, NOT in transform_into_action. + // transform_into_action passes it through with zero validation. + // This test documents that the transformer is a pure pass-through. + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::StateError(StateError::IdentityInsufficientBalanceError(_)) + )] + ); + } + + /// AUDIT L8: Fee estimation uses saturating_add while structure uses checked_add. + /// + /// Fee estimation at `state_transition_estimated_fee_validation.rs:19` uses + /// `saturating_add` when computing the total. Structure validation uses + /// `checked_add`. With very large values, fee estimation silently saturates + /// (to u64::MAX) instead of erroring, while structure validation properly + /// errors with OverflowError. This inconsistency could allow a malformed + /// transition to pass fee estimation but fail structure validation. + /// + /// Location: rs-drive-abci/.../state_transition_estimated_fee_validation.rs:19 + #[test] + fn test_fee_estimation_saturating_add_matches_structure_validation() { + let platform_version = PlatformVersion::latest(); + + // Create recipients whose sum overflows u64 + let mut recipient_addresses = BTreeMap::new(); + recipient_addresses.insert(create_platform_address(1), u64::MAX - 1); + recipient_addresses.insert(create_platform_address(2), u64::MAX - 1); + + let transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { + identity_id: [1u8; 32].into(), + recipient_addresses: recipient_addresses.clone(), + nonce: 1, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: BinaryData::new(vec![0; 65]), + }; + + // Structure validation should reject with OverflowError (uses checked_add) + let structure_result = transition_v0.validate_structure(platform_version); + assert!( + !structure_result.is_valid(), + "Structure validation should reject overflow" + ); + + let has_overflow = structure_result + .errors + .iter() + .any(|e| matches!(e, ConsensusError::BasicError(BasicError::OverflowError(_)))); + + // The key insight: structure validation uses checked_add (correct), + // but fee estimation uses saturating_add (inconsistent). Fee estimation + // would compute a saturated sum = u64::MAX and proceed without error, + // while structure validation correctly rejects. + // + // This inconsistency means the fee estimation path and structure validation + // path have different behavior for the same input — a code smell that could + // mask issues if the order of validation changes. + assert!( + has_overflow, + "AUDIT L8: Structure validation correctly rejects with OverflowError \ + (uses checked_add). However, fee estimation at \ + state_transition_estimated_fee_validation.rs:19 uses saturating_add, \ + which would silently compute u64::MAX instead of erroring. \ + This inconsistency means fee estimation and structure validation \ + disagree on overflow handling. Got errors: {:?}", + structure_result.errors + ); + } + } + // ========================================== // SUCCESSFUL TRANSITION TESTS // ========================================== diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs index 7d0d9b1a7f7..ba5c693a780 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs @@ -11,7 +11,7 @@ use dpp::state_transition::identity_credit_withdrawal_transition::accessors::Ide use dpp::state_transition::identity_credit_withdrawal_transition::{ IdentityCreditWithdrawalTransition, MIN_CORE_FEE_PER_BYTE, MIN_WITHDRAWAL_AMOUNT, }; -use dpp::util::is_fibonacci_number::is_fibonacci_number; +use dpp::util::is_non_zero_fibonacci_number::is_non_zero_fibonacci_number; use dpp::validation::SimpleConsensusValidationResult; use dpp::version::PlatformVersion; use dpp::withdrawal::Pooling; @@ -53,7 +53,7 @@ impl IdentityCreditWithdrawalStateTransitionStructureValidationV1 } // validate core_fee is in fibonacci sequence - if !is_fibonacci_number(self.core_fee_per_byte() as u64) { + if !is_non_zero_fibonacci_number(self.core_fee_per_byte() as u64) { result.add_error(InvalidCreditWithdrawalTransitionCoreFeeError::new( self.core_fee_per_byte(), MIN_CORE_FEE_PER_BYTE, diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up_from_addresses/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up_from_addresses/tests.rs index a3c8342abe1..fbf1d17bf99 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up_from_addresses/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up_from_addresses/tests.rs @@ -2,8 +2,8 @@ mod tests { use crate::config::{PlatformConfig, PlatformTestConfig}; use crate::execution::validation::state_transition::state_transitions::test_helpers::{ - create_dummy_witness, setup_address_with_balance, TestAddressSigner, - TestProtocolError as ProtocolError, + create_dummy_witness, create_platform_address, setup_address_with_balance, + TestAddressSigner, TestProtocolError as ProtocolError, }; use crate::platform_types::state_transitions_processing_result::StateTransitionExecutionResult; use crate::test::helpers::setup::TestPlatformBuilder; @@ -13,6 +13,7 @@ mod tests { }; use dpp::block::block_info::BlockInfo; use dpp::consensus::basic::BasicError; + use dpp::consensus::signature::SignatureError; use dpp::consensus::state::state_error::StateError; use dpp::consensus::ConsensusError; use dpp::dash_to_credits; @@ -1045,7 +1046,9 @@ mod tests { assert_matches!( processing_result.execution_results().as_slice(), [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::BasicError(BasicError::InputWitnessCountMismatchError(_)) + ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ) )] ); } @@ -3280,4 +3283,174 @@ mod tests { )); } } + + mod security { + use super::*; + + /// AUDIT M1: Fee deduction BTreeMap index shifting after entry removal. + /// + /// When fee strategy step DeductFromInput(0) drains input A to zero, + /// A is removed from the BTreeMap. The next step DeductFromInput(1) + /// now targets what was originally at index 2 (C) instead of index 1 (B), + /// because all indices shifted down after the removal. + /// + /// Location: rs-dpp/.../deduct_fee_from_inputs_and_outputs/v0/mod.rs:35-45 + #[test] + fn test_fee_deduction_stable_after_entry_removal() { + let platform_version = PlatformVersion::latest(); + let platform_config = PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }; + + let mut platform = TestPlatformBuilder::new() + .with_config(platform_config) + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_genesis_state(); + + let identity = setup_identity(&mut platform, 1, dash_to_credits!(1.0)); + + let mut signer = TestAddressSigner::new(); + let addr_a = signer.add_p2pkh([10u8; 32]); + let addr_b = signer.add_p2pkh([20u8; 32]); + let addr_c = signer.add_p2pkh([30u8; 32]); + + // Determine BTreeMap sort order + let mut sorted_addrs = vec![addr_a, addr_b, addr_c]; + sorted_addrs.sort(); + let first = sorted_addrs[0]; + let second = sorted_addrs[1]; + let third = sorted_addrs[2]; + + let first_balance = dash_to_credits!(0.1); + let second_balance = dash_to_credits!(1.0); + let third_balance = dash_to_credits!(1.0); + + // Input amount leaves only 1000 credits remaining for first + let first_input = first_balance - 1000; + let second_input = dash_to_credits!(0.01); + let third_input = dash_to_credits!(0.01); + + setup_address_with_balance(&mut platform, first, 0, first_balance); + setup_address_with_balance(&mut platform, second, 0, second_balance); + setup_address_with_balance(&mut platform, third, 0, third_balance); + + let mut inputs = BTreeMap::new(); + inputs.insert(first, (1 as AddressNonce, first_input)); + inputs.insert(second, (1 as AddressNonce, second_input)); + inputs.insert(third, (1 as AddressNonce, third_input)); + + // Fee strategy: deduct from index 0 (first), then index 1 (should be second). + let fee_strategy = AddressFundsFeeStrategy::from(vec![ + AddressFundsFeeStrategyStep::DeductFromInput(0), + AddressFundsFeeStrategyStep::DeductFromInput(1), + ]); + + let transition = create_signed_transition_with_options( + &identity, + &signer, + inputs, + None, + fee_strategy, + 0, + platform_version, + ); + + let result = transition.serialize_to_bytes().expect("should serialize"); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + let processing_result = platform + .platform + .process_raw_state_transitions( + &vec![result], + &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 { .. }], + "Transaction should succeed" + ); + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit"); + + let second_remaining_before_fee = second_balance - second_input; + + let (_, second_final) = platform + .drive + .fetch_balance_and_nonce(&second, None, platform_version) + .expect("should fetch") + .expect("second address should exist"); + + assert!( + second_final < second_remaining_before_fee, + "AUDIT M1: Fee should have been deducted from second address (original \ + BTreeMap index 1), but it was deducted from third address instead. \ + After first was drained (1000 credits) and removed from BTreeMap, \ + DeductFromInput(1) shifted to target the third address. \ + second's balance: {} (expected < {})", + second_final, + second_remaining_before_fee + ); + } + + /// AUDIT M3: Unchecked subtraction in identity_top_up_from_addresses transformer. + /// + /// At `transformer.rs:24`, the transformer uses `.sum()` (wrapping) and at + /// line 28 uses unchecked subtraction. If structure validation is bypassed, + /// these operations could wrap/underflow silently. + /// + /// This test verifies structure validation catches overflow, but notes + /// the transformer lacks defense-in-depth. + /// + /// Location: rs-drive/.../identity_top_up_from_addresses/v0/transformer.rs:24,28 + #[test] + fn test_transformer_subtraction_uses_checked_arithmetic() { + use crate::execution::validation::state_transition::processor::traits::basic_structure::StateTransitionBasicStructureValidationV0; + + let platform_version = PlatformVersion::latest(); + + // Two inputs that sum to > u64::MAX + let mut inputs = BTreeMap::new(); + inputs.insert(create_platform_address(1), (0 as AddressNonce, u64::MAX)); + inputs.insert(create_platform_address(2), (0 as AddressNonce, u64::MAX)); + + let transition = create_raw_transition_with_dummy_witnesses( + Identifier::random(), + inputs, + None, + AddressFundsFeeStrategy::from(vec![AddressFundsFeeStrategyStep::DeductFromInput( + 0, + )]), + 2, + ); + + let result = transition + .validate_basic_structure(dpp::dashcore::Network::Testnet, platform_version) + .expect("validation should not return Err"); + + assert!(!result.is_valid()); + assert_matches!( + result.first_error().unwrap(), + ConsensusError::BasicError(BasicError::OverflowError(_)) + ); + } + } } diff --git a/packages/rs-drive/src/drive/address_funds/set_balance_to_address/v0/mod.rs b/packages/rs-drive/src/drive/address_funds/set_balance_to_address/v0/mod.rs index 16be97c5025..281a859fd62 100644 --- a/packages/rs-drive/src/drive/address_funds/set_balance_to_address/v0/mod.rs +++ b/packages/rs-drive/src/drive/address_funds/set_balance_to_address/v0/mod.rs @@ -4,6 +4,7 @@ use crate::fees::op::LowLevelDriveOperation; use dpp::address_funds::PlatformAddress; use dpp::fee::Credits; use dpp::prelude::AddressNonce; +use dpp::ProtocolError; use grovedb::batch::KeyInfoPath; use grovedb::element::SumValue; use grovedb::{Element, EstimatedLayerInformation}; @@ -44,6 +45,13 @@ impl Drive { )?; } + // Guard against u64 values that would overflow when cast to i64 (SumValue) + if balance > i64::MAX as u64 { + return Err( + ProtocolError::Overflow("balance exceeds maximum for sum tree storage").into(), + ); + } + // Simply insert/overwrite the balance as an ItemWithSumItem element // The nonce is stored as big-endian bytes, and the balance is the sum value Ok(vec![ diff --git a/packages/rs-drive/src/state_transition_action/action_convert_to_operations/address_funds/address_funding_from_asset_lock_transition.rs b/packages/rs-drive/src/state_transition_action/action_convert_to_operations/address_funds/address_funding_from_asset_lock_transition.rs index db5777a739f..30f85a501f4 100644 --- a/packages/rs-drive/src/state_transition_action/action_convert_to_operations/address_funds/address_funding_from_asset_lock_transition.rs +++ b/packages/rs-drive/src/state_transition_action/action_convert_to_operations/address_funds/address_funding_from_asset_lock_transition.rs @@ -25,7 +25,7 @@ impl DriveHighLevelOperationConverter for AddressFundingFromAssetLockTransitionA 0 => { let asset_lock_outpoint = self.asset_lock_outpoint(); - let (inputs, outputs, mut asset_lock_value) = + let (inputs, outputs, mut asset_lock_value, input_contributions_total) = self.inputs_with_remaining_balance_outputs_and_asset_lock_value_owned(); let initial_balance = asset_lock_value.remaining_credit_value(); @@ -56,9 +56,12 @@ impl DriveHighLevelOperationConverter for AddressFundingFromAssetLockTransitionA // Calculate the sum of explicit outputs let explicit_outputs_sum: u64 = outputs.values().flatten().sum(); - // Calculate remainder: initial_balance - explicit_outputs_sum - // Note: fees are handled separately during validation, inputs have been consumed above - let remainder_balance = initial_balance.saturating_sub(explicit_outputs_sum); + // Total available for outputs = asset lock + input contributions + // (input credits are already in the system — we're moving them, not creating them) + let total_available = initial_balance + input_contributions_total; + + // Calculate remainder: total_available - explicit_outputs_sum + let remainder_balance = total_available.saturating_sub(explicit_outputs_sum); // Add balance to outputs for (address, balance_option) in outputs { diff --git a/packages/rs-drive/src/state_transition_action/address_funds/address_credit_withdrawal/v0/transformer.rs b/packages/rs-drive/src/state_transition_action/address_funds/address_credit_withdrawal/v0/transformer.rs index 710239207b4..862b2d052c9 100644 --- a/packages/rs-drive/src/state_transition_action/address_funds/address_credit_withdrawal/v0/transformer.rs +++ b/packages/rs-drive/src/state_transition_action/address_funds/address_credit_withdrawal/v0/transformer.rs @@ -1,5 +1,6 @@ use crate::state_transition_action::address_funds::address_credit_withdrawal::v0::AddressCreditWithdrawalTransitionActionV0; use dpp::address_funds::PlatformAddress; +use dpp::consensus::basic::overflow_error::OverflowError; use dpp::data_contracts::withdrawals_contract; use dpp::data_contracts::withdrawals_contract::v1::document_types::withdrawal; use dpp::document::{Document, DocumentV0}; @@ -32,12 +33,33 @@ impl AddressCreditWithdrawalTransitionActionV0 { .. } = value; - // Sum all balances from inputs - let total_inputs: Credits = inputs.values().map(|(_, balance)| *balance).sum(); + // Sum all balances from inputs (checked to prevent overflow) + let total_inputs: Credits = match inputs + .values() + .try_fold(0u64, |acc, (_, balance)| acc.checked_add(*balance)) + { + Some(sum) => sum, + None => { + return ConsensusValidationResult::new_with_error( + OverflowError::new("Input sum overflow in withdrawal transformer".to_string()) + .into(), + ) + } + }; // Calculate the withdrawal amount: total remaining minus output (if any) let amount = match output { - Some((_, output_amount)) => total_inputs - output_amount, + Some((_, output_amount)) => match total_inputs.checked_sub(*output_amount) { + Some(diff) => diff, + None => { + return ConsensusValidationResult::new_with_error( + OverflowError::new( + "Output exceeds input sum in withdrawal transformer".to_string(), + ) + .into(), + ) + } + }, None => total_inputs, }; diff --git a/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/mod.rs b/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/mod.rs index eb036d54b82..f706035c265 100644 --- a/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/mod.rs +++ b/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/mod.rs @@ -47,6 +47,15 @@ impl AddressFundingFromAssetLockTransitionAction { } } + /// Get total credits contributed from address inputs + pub fn input_contributions_total(&self) -> Credits { + match self { + AddressFundingFromAssetLockTransitionAction::V0(transition) => { + transition.input_contributions_total + } + } + } + /// Get resolved outputs with remainder computed. /// Returns outputs where all Option are resolved to concrete Credits values. pub fn resolved_outputs(&self) -> BTreeMap { @@ -57,11 +66,14 @@ impl AddressFundingFromAssetLockTransitionAction { .asset_lock_value_to_be_consumed() .remaining_credit_value(); + // Total available = asset lock + input contributions + let total_available = asset_lock_balance + self.input_contributions_total(); + // Calculate the sum of explicit outputs let explicit_outputs_sum: Credits = outputs.values().flatten().sum(); // Calculate remainder - let remainder_balance = asset_lock_balance.saturating_sub(explicit_outputs_sum); + let remainder_balance = total_available.saturating_sub(explicit_outputs_sum); // Resolve all outputs outputs @@ -76,7 +88,7 @@ impl AddressFundingFromAssetLockTransitionAction { .collect() } - /// Returns owned copies of inputs and outputs. + /// Returns owned copies of inputs, outputs, asset lock value, and input contributions total. #[allow(clippy::type_complexity)] pub fn inputs_with_remaining_balance_outputs_and_asset_lock_value_owned( self, @@ -84,12 +96,14 @@ impl AddressFundingFromAssetLockTransitionAction { BTreeMap, BTreeMap>, AssetLockValue, + Credits, ) { match self { AddressFundingFromAssetLockTransitionAction::V0(transition) => ( transition.inputs_with_remaining_balance, transition.outputs, transition.asset_lock_value_to_be_consumed, + transition.input_contributions_total, ), } } diff --git a/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/mod.rs b/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/mod.rs index 69c65a1a931..d2746aa61c1 100644 --- a/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/mod.rs +++ b/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/mod.rs @@ -21,6 +21,8 @@ pub struct AddressFundingFromAssetLockTransitionActionV0 { pub inputs_with_remaining_balance: BTreeMap, /// outputs (Some = explicit amount, None = remainder recipient) pub outputs: BTreeMap>, + /// Total credits contributed from address inputs (sum of amounts being spent) + pub input_contributions_total: Credits, /// fee strategy pub fee_strategy: AddressFundsFeeStrategy, /// fee multiplier diff --git a/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/transformer.rs b/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/transformer.rs index e136ef8cfe2..67e1114c0a9 100644 --- a/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/transformer.rs +++ b/packages/rs-drive/src/state_transition_action/address_funds/address_funding_from_asset_lock/v0/transformer.rs @@ -38,11 +38,16 @@ impl AddressFundingFromAssetLockTransitionActionV0 { ) })?; + // Compute total credits contributed from address inputs + let input_contributions_total: Credits = + value.inputs.values().map(|(_, amount)| *amount).sum(); + Ok(AddressFundingFromAssetLockTransitionActionV0 { signable_bytes_hasher, asset_lock_value_to_be_consumed, asset_lock_outpoint: Bytes36::new(asset_lock_outpoint.into()), inputs_with_remaining_balance, + input_contributions_total, outputs: outputs.clone(), fee_strategy: fee_strategy.clone(), user_fee_increase: *user_fee_increase, diff --git a/packages/rs-drive/src/state_transition_action/identity/identity_create_from_addresses/v0/transformer.rs b/packages/rs-drive/src/state_transition_action/identity/identity_create_from_addresses/v0/transformer.rs index dae4226184e..c6d98a4a6a1 100644 --- a/packages/rs-drive/src/state_transition_action/identity/identity_create_from_addresses/v0/transformer.rs +++ b/packages/rs-drive/src/state_transition_action/identity/identity_create_from_addresses/v0/transformer.rs @@ -1,5 +1,6 @@ use crate::state_transition_action::identity::identity_create_from_addresses::v0::IdentityCreateFromAddressesTransitionActionV0; use dpp::address_funds::PlatformAddress; +use dpp::consensus::basic::overflow_error::OverflowError; use dpp::consensus::basic::value_error::ValueError; use dpp::consensus::ConsensusError; use dpp::fee::Credits; @@ -35,12 +36,35 @@ impl IdentityCreateFromAddressesTransitionActionV0 { .. } = value; - // Sum all balances from inputs - let total_inputs: Credits = inputs.values().map(|(_, balance)| *balance).sum(); + // Sum all balances from inputs (checked to prevent overflow) + let total_inputs: Credits = match inputs + .values() + .try_fold(0u64, |acc, (_, balance)| acc.checked_add(*balance)) + { + Some(sum) => sum, + None => { + return ConsensusValidationResult::new_with_error( + OverflowError::new( + "Input sum overflow in identity create transformer".to_string(), + ) + .into(), + ) + } + }; // Subtract the output amount if present let fund_identity_amount = match output { - Some((_, output_amount)) => total_inputs - output_amount, + Some((_, output_amount)) => match total_inputs.checked_sub(*output_amount) { + Some(diff) => diff, + None => { + return ConsensusValidationResult::new_with_error( + OverflowError::new( + "Output exceeds input sum in identity create transformer".to_string(), + ) + .into(), + ) + } + }, None => total_inputs, }; diff --git a/packages/rs-drive/src/state_transition_action/identity/identity_topup_from_addresses/v0/transformer.rs b/packages/rs-drive/src/state_transition_action/identity/identity_topup_from_addresses/v0/transformer.rs index fd4321b738e..1fa5743f8a1 100644 --- a/packages/rs-drive/src/state_transition_action/identity/identity_topup_from_addresses/v0/transformer.rs +++ b/packages/rs-drive/src/state_transition_action/identity/identity_topup_from_addresses/v0/transformer.rs @@ -1,5 +1,6 @@ use crate::state_transition_action::identity::identity_topup_from_addresses::v0::IdentityTopUpFromAddressesTransitionActionV0; use dpp::address_funds::PlatformAddress; +use dpp::consensus::basic::overflow_error::OverflowError; use dpp::fee::Credits; use dpp::prelude::{AddressNonce, ConsensusValidationResult}; use dpp::state_transition::state_transitions::identity::identity_topup_from_addresses_transition::v0::IdentityTopUpFromAddressesTransitionV0; @@ -20,12 +21,35 @@ impl IdentityTopUpFromAddressesTransitionActionV0 { .. } = value; - // Sum all balances from inputs - let total_inputs: Credits = inputs.values().map(|(_, balance)| *balance).sum(); + // Sum all balances from inputs (checked to prevent overflow) + let total_inputs: Credits = match inputs + .values() + .try_fold(0u64, |acc, (_, balance)| acc.checked_add(*balance)) + { + Some(sum) => sum, + None => { + return ConsensusValidationResult::new_with_error( + OverflowError::new( + "Input sum overflow in identity topup transformer".to_string(), + ) + .into(), + ) + } + }; // Subtract the output amount if present to get the topup amount let topup_amount = match output { - Some((_, output_amount)) => total_inputs - output_amount, + Some((_, output_amount)) => match total_inputs.checked_sub(*output_amount) { + Some(diff) => diff, + None => { + return ConsensusValidationResult::new_with_error( + OverflowError::new( + "Output exceeds input sum in identity topup transformer".to_string(), + ) + .into(), + ) + } + }, None => total_inputs, }; diff --git a/packages/wasm-dpp/src/errors/consensus/consensus_error.rs b/packages/wasm-dpp/src/errors/consensus/consensus_error.rs index 095a5b63a86..429a94b1a4f 100644 --- a/packages/wasm-dpp/src/errors/consensus/consensus_error.rs +++ b/packages/wasm-dpp/src/errors/consensus/consensus_error.rs @@ -88,7 +88,7 @@ use dpp::consensus::state::prefunded_specialized_balances::prefunded_specialized use dpp::consensus::state::prefunded_specialized_balances::prefunded_specialized_balance_not_found_error::PrefundedSpecializedBalanceNotFoundError; use dpp::consensus::state::token::{IdentityDoesNotHaveEnoughTokenBalanceError, IdentityTokenAccountNotFrozenError, IdentityTokenAccountFrozenError, TokenIsPausedError, IdentityTokenAccountAlreadyFrozenError, UnauthorizedTokenActionError, TokenSettingMaxSupplyToLessThanCurrentSupplyError, TokenMintPastMaxSupplyError, NewTokensDestinationIdentityDoesNotExistError, NewAuthorizedActionTakerIdentityDoesNotExistError, NewAuthorizedActionTakerGroupDoesNotExistError, NewAuthorizedActionTakerMainGroupNotSetError, InvalidGroupPositionError, TokenAlreadyPausedError, TokenNotPausedError, InvalidTokenClaimPropertyMismatch, InvalidTokenClaimNoCurrentRewards, InvalidTokenClaimWrongClaimant, TokenTransferRecipientIdentityNotExistError, PreProgrammedDistributionTimestampInPastError, IdentityHasNotAgreedToPayRequiredTokenAmountError, RequiredTokenPaymentInfoNotSetError, IdentityTryingToPayWithWrongTokenError, TokenDirectPurchaseUserPriceTooLow, TokenAmountUnderMinimumSaleAmount, TokenNotForDirectSale, InvalidTokenPositionStateError}; use dpp::consensus::state::address_funds::{AddressDoesNotExistError, AddressInvalidNonceError, AddressNotEnoughFundsError, AddressesNotEnoughFundsError}; -use dpp::consensus::basic::state_transition::{StateTransitionNotActiveError, TransitionOverMaxInputsError, TransitionOverMaxOutputsError, InputWitnessCountMismatchError, TransitionNoInputsError, TransitionNoOutputsError, FeeStrategyEmptyError, FeeStrategyDuplicateError, FeeStrategyIndexOutOfBoundsError, FeeStrategyTooManyStepsError, InputBelowMinimumError, OutputBelowMinimumError, InputOutputBalanceMismatchError, OutputsNotGreaterThanInputsError, WithdrawalBalanceMismatchError, InsufficientFundingAmountError, InputsNotLessThanOutputsError, OutputAddressAlsoInputError, InvalidRemainderOutputCountError}; +use dpp::consensus::basic::state_transition::{StateTransitionNotActiveError, TransitionOverMaxInputsError, TransitionOverMaxOutputsError, InputWitnessCountMismatchError, TransitionNoInputsError, TransitionNoOutputsError, FeeStrategyEmptyError, FeeStrategyDuplicateError, FeeStrategyIndexOutOfBoundsError, FeeStrategyTooManyStepsError, InputBelowMinimumError, OutputBelowMinimumError, InputOutputBalanceMismatchError, OutputsNotGreaterThanInputsError, WithdrawalBalanceMismatchError, InsufficientFundingAmountError, InputsNotLessThanOutputsError, OutputAddressAlsoInputError, InvalidRemainderOutputCountError, WithdrawalBelowMinAmountError}; use dpp::consensus::state::voting::masternode_incorrect_voter_identity_id_error::MasternodeIncorrectVoterIdentityIdError; use dpp::consensus::state::voting::masternode_incorrect_voting_address_error::MasternodeIncorrectVotingAddressError; use dpp::consensus::state::voting::masternode_not_found_error::MasternodeNotFoundError; @@ -920,6 +920,9 @@ fn from_basic_error(basic_error: &BasicError) -> JsValue { BasicError::InvalidRemainderOutputCountError(e) => { generic_consensus_error!(InvalidRemainderOutputCountError, e).into() } + BasicError::WithdrawalBelowMinAmountError(e) => { + generic_consensus_error!(WithdrawalBelowMinAmountError, e).into() + } } }