From 9e0b0adf02e180438760f6acf65dd2e75abef98c Mon Sep 17 00:00:00 2001 From: Adefokun Adeoluwa Israel Date: Mon, 17 Aug 2026 15:56:49 +0100 Subject: [PATCH] fix: prevent negative calculate split amounts --- settlement_contract/src/admin.rs | 3 + settlement_contract/src/errors.rs | 4 + settlement_contract/src/payments.rs | 24 ++- settlement_contract/src/settlement.rs | 3 + .../src/tests/conformity_tests.rs | 17 +- settlement_contract/src/tests/mod.rs | 1 + .../src/tests/payment_tests.rs | 145 ++++++++++++++++++ 7 files changed, 182 insertions(+), 15 deletions(-) create mode 100644 settlement_contract/src/tests/payment_tests.rs diff --git a/settlement_contract/src/admin.rs b/settlement_contract/src/admin.rs index 94d2ca9..fe06ddd 100644 --- a/settlement_contract/src/admin.rs +++ b/settlement_contract/src/admin.rs @@ -505,6 +505,9 @@ impl SettlementContract { if new_rule.platform_fee_bps < MIN_FEE_BPS || new_rule.network_fee_bps < MIN_FEE_BPS { panic_with_error!(env, SettlementError::InvalidFeeBps); } + if new_rule.platform_fee_bps + new_rule.network_fee_bps > BPS_DENOMINATOR { + panic_with_error!(env, SettlementError::InvalidFeeBps); + } if new_rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER { panic_with_error!(env, SettlementError::InvalidSettlementDelay); } diff --git a/settlement_contract/src/errors.rs b/settlement_contract/src/errors.rs index f40e680..64d97ab 100644 --- a/settlement_contract/src/errors.rs +++ b/settlement_contract/src/errors.rs @@ -73,6 +73,9 @@ pub enum SettlementError { AmountTooSmall = 313, AmountZero = 314, AmountNegative = 315, + /// The rounded platform and network fees exceed the gross payment amount. + /// Raised by `calculate_split` before a `FeeSplit` or `PaymentRecord` is produced. + SplitExceedsAmount = 316, } const _: () = { @@ -121,4 +124,5 @@ const _: () = { assert!(SettlementError::AmountTooSmall as u32 >= error_codes::SETTLEMENT_RANGE_START); assert!(SettlementError::AmountZero as u32 >= error_codes::SETTLEMENT_RANGE_START); assert!(SettlementError::AmountNegative as u32 >= error_codes::SETTLEMENT_RANGE_START); + assert!(SettlementError::SplitExceedsAmount as u32 >= error_codes::SETTLEMENT_RANGE_START); }; diff --git a/settlement_contract/src/payments.rs b/settlement_contract/src/payments.rs index 66fa5fd..e901baf 100644 --- a/settlement_contract/src/payments.rs +++ b/settlement_contract/src/payments.rs @@ -11,15 +11,6 @@ use crate::{ }; /// Computes the platform, network, and merchant fee amounts for an amount using ceil-based rounding. -/// -/// # Known edge case: negative merchant amount -/// -/// Ceiling rounding of both fees independently can make -/// `platform_fee_amount + network_fee_amount > amount` for small gross amounts -/// (e.g. `amount = 1`, `platform_fee_bps = 5000`, `network_fee_bps = 5000`), -/// which yields a **negative** `merchant_amount`. This is intentional with the -/// current rounding policy (fees are never under-collected); callers must treat -/// a negative merchant payout as a known, documented outcome rather than a bug. fn calculate_split(env: &Env, amount: i128, rule: &SettlementRule) -> FeeSplit { let denom = BPS_DENOMINATOR as i128; let platform_bps = rule.platform_bps(); @@ -42,11 +33,16 @@ fn calculate_split(env: &Env, amount: i128, rule: &SettlementRule) -> FeeSplit { let platform_fee_amount = platform_bps.calculate_fee_ceil(amount); let network_fee_amount = network_bps.calculate_fee_ceil(amount); - // The merchant amount is calculated as the subtraction remainder of the gross amount minus all rounded-up fees. - // This ensures the sum of the split amounts (platform fee + network fee + merchant share) always equals the gross amount. - // Consequence: The merchant absorbs all rounding dust. For very small gross amounts with high/extreme fee percentages, - // the sum of rounded-up fees can exceed the gross amount, resulting in a negative merchant payout. - let merchant_amount = amount - platform_fee_amount - network_fee_amount; + let total_fee_amount = platform_fee_amount + .checked_add(network_fee_amount) + .unwrap_or_else(|| panic_with_error!(env, SettlementError::AmountOverflow)); + if total_fee_amount > amount { + panic_with_error!(env, SettlementError::SplitExceedsAmount); + } + + let merchant_amount = amount + .checked_sub(total_fee_amount) + .unwrap_or_else(|| panic_with_error!(env, SettlementError::AmountOverflow)); FeeSplit { gross_amount: amount, platform_fee_amount, diff --git a/settlement_contract/src/settlement.rs b/settlement_contract/src/settlement.rs index 009367b..57c767d 100644 --- a/settlement_contract/src/settlement.rs +++ b/settlement_contract/src/settlement.rs @@ -98,6 +98,9 @@ impl SettlementContract { if new_rule.platform_fee_bps < MIN_FEE_BPS || new_rule.network_fee_bps < MIN_FEE_BPS { panic_with_error!(&env, SettlementError::InvalidFeeBps); } + if new_rule.platform_fee_bps + new_rule.network_fee_bps > BPS_DENOMINATOR { + panic_with_error!(&env, SettlementError::InvalidFeeBps); + } if new_rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER { panic_with_error!(&env, SettlementError::InvalidSettlementDelay); } diff --git a/settlement_contract/src/tests/conformity_tests.rs b/settlement_contract/src/tests/conformity_tests.rs index e85768c..f06f408 100644 --- a/settlement_contract/src/tests/conformity_tests.rs +++ b/settlement_contract/src/tests/conformity_tests.rs @@ -60,7 +60,7 @@ fn governance_codes() -> [(&'static str, u32); 19] { ] } -fn settlement_codes() -> [(&'static str, u32); 24] { +fn settlement_codes() -> [(&'static str, u32); 30] { [ ( "AlreadyInitialized", @@ -122,6 +122,21 @@ fn settlement_codes() -> [(&'static str, u32); 24] { ), ("InvalidGovernance", SettlementError::InvalidGovernance as u32), ("AmountOverflow", SettlementError::AmountOverflow as u32), + ( + "GovernanceCallFailed", + SettlementError::GovernanceCallFailed as u32, + ), + ( + "FeeExceedsGovernanceConfig", + SettlementError::FeeExceedsGovernanceConfig as u32, + ), + ("AmountTooSmall", SettlementError::AmountTooSmall as u32), + ("AmountZero", SettlementError::AmountZero as u32), + ("AmountNegative", SettlementError::AmountNegative as u32), + ( + "SplitExceedsAmount", + SettlementError::SplitExceedsAmount as u32, + ), ] } diff --git a/settlement_contract/src/tests/mod.rs b/settlement_contract/src/tests/mod.rs index 5d9c572..4c4b5ee 100644 --- a/settlement_contract/src/tests/mod.rs +++ b/settlement_contract/src/tests/mod.rs @@ -6,6 +6,7 @@ pub mod admin_tests; pub mod conformity_tests; +pub mod payment_tests; use crate::*; use soroban_sdk::testutils::Address as _; diff --git a/settlement_contract/src/tests/payment_tests.rs b/settlement_contract/src/tests/payment_tests.rs new file mode 100644 index 0000000..ad280d4 --- /dev/null +++ b/settlement_contract/src/tests/payment_tests.rs @@ -0,0 +1,145 @@ +//! Tests for payment reference storage and fee-split calculation. + +extern crate std; + +use crate::*; +use soroban_sdk::testutils::Ledger; +use soroban_sdk::{Address, BytesN, Env}; + +use super::setup; + +fn rule(platform_fee_bps: u32, network_fee_bps: u32) -> SettlementRule { + SettlementRule { + platform_fee_bps, + network_fee_bps, + settlement_delay_ledger: 0, + auto_settle: false, + } +} + +fn payment_reference(env: &Env, byte: u8) -> BytesN<32> { + BytesN::from_array(env, &[byte; 32]) +} + +fn registered_merchant_with_rule( + client: &SettlementContractClient<'static>, + admins: &soroban_sdk::Vec
, + merchant: &Address, + settlement_rule: SettlementRule, +) { + client.register_merchant(admins, merchant); + client.set_settlement_rule(admins, merchant, &settlement_rule); +} + +#[test] +fn calculate_fee_split_preserves_valid_combined_fees() { + let (_env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(250, 50)); + + let split = client.calculate_fee_split(&merchant, &10_000); + + assert_eq!(split.gross_amount, 10_000); + assert_eq!(split.platform_fee_amount, 250); + assert_eq!(split.network_fee_amount, 50); + assert_eq!(split.merchant_amount, 9_700); +} + +#[test] +fn calculate_fee_split_allows_exact_100_percent_when_rounding_fits() { + let (_env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(5_000, 5_000)); + + let split = client.calculate_fee_split(&merchant, &100); + + assert_eq!(split.gross_amount, 100); + assert_eq!(split.platform_fee_amount, 50); + assert_eq!(split.network_fee_amount, 50); + assert_eq!(split.merchant_amount, 0); +} + +#[test] +#[should_panic(expected = "Error(Contract, #316)")] +fn calculate_fee_split_rejects_ceil_rounded_fees_above_amount() { + let (_env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(5_000, 5_000)); + + client.calculate_fee_split(&merchant, &101); +} + +#[test] +#[should_panic(expected = "Error(Contract, #316)")] +fn calculate_fee_split_rejects_minimal_amount_when_rounding_exceeds_gross() { + let (_env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(5_000, 5_000)); + + client.calculate_fee_split(&merchant, &1); +} + +#[test] +#[should_panic(expected = "Error(Contract, #314)")] +fn calculate_fee_split_rejects_zero_amount_before_split_calculation() { + let (_env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(250, 50)); + + client.calculate_fee_split(&merchant, &0); +} + +#[test] +fn store_payment_reference_rejects_ceil_rounded_split_without_storing_record() { + let (env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(5_000, 5_000)); + let reference = payment_reference(&env, 1); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.store_payment_reference(&merchant, &reference, &101); + })); + + assert!(result.is_err()); + assert!(client.get_payment_reference(&reference).is_none()); +} + +#[test] +#[should_panic(expected = "Error(Contract, #4)")] +fn set_default_rule_rejects_fee_sum_over_100_percent() { + let (_env, client, admins, _merchant) = setup(); + + client.set_default_rule(&admins, &rule(6_000, 6_000)); +} + +#[test] +fn scheduled_set_default_rule_rejects_fee_sum_over_100_percent() { + let (env, client, admins, _merchant) = setup(); + let admin = admins.get(0).unwrap(); + let invalid_rule = rule(6_000, 6_000); + let operation = Operation::SetDefaultRule(invalid_rule); + + client.schedule(&admin, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); + env.ledger() + .with_mut(|ledger| ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.execute(&operation); + })); + + assert!(result.is_err()); + assert!(client.get_default_rule().is_none()); +} + +#[test] +fn store_payment_reference_keeps_valid_payment_storage_unchanged() { + let (env, client, admins, merchant) = setup(); + registered_merchant_with_rule(&client, &admins, &merchant, rule(250, 50)); + let reference = payment_reference(&env, 2); + + let split = client.store_payment_reference(&merchant, &reference, &10_000); + let record = client.get_payment_reference(&reference).unwrap(); + + assert_eq!(split.gross_amount, 10_000); + assert_eq!(split.platform_fee_amount, 250); + assert_eq!(split.network_fee_amount, 50); + assert_eq!(split.merchant_amount, 9_700); + assert_eq!(record.amount, 10_000); + assert_eq!(record.platform_fee_amount, 250); + assert_eq!(record.network_fee_amount, 50); + assert_eq!(record.merchant_amount, 9_700); +}