diff --git a/contracts/asset_path_payment/src/lib.rs b/contracts/asset_path_payment/src/lib.rs index 590e46fb..1936f5f6 100644 --- a/contracts/asset_path_payment/src/lib.rs +++ b/contracts/asset_path_payment/src/lib.rs @@ -24,6 +24,7 @@ pub enum PathPaymentError { PriceImpactTooHigh = 12, TransferFailed = 13, ContractPaused = 14, + SelfPayment = 15, } /// Storage keys @@ -213,6 +214,10 @@ impl AssetPathPaymentContract { Self::require_not_paused(&env)?; from.require_auth(); + if from == to { + return Err(PathPaymentError::SelfPayment); + } + // Validate amounts if source_amount <= 0 { return Err(PathPaymentError::InvalidAmount); diff --git a/contracts/asset_path_payment/src/test.rs b/contracts/asset_path_payment/src/test.rs index ca2b1a8b..1998cfbb 100644 --- a/contracts/asset_path_payment/src/test.rs +++ b/contracts/asset_path_payment/src/test.rs @@ -157,6 +157,28 @@ fn test_initiate_path_payment_rejects_invalid_amounts() { assert_eq!(result, Err(Ok(PathPaymentError::SlippageExceeded))); } +#[test] +fn test_initiate_path_payment_rejects_self_payment() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let from = Address::generate(&env); + let source = create_token_contract(&env, &Address::generate(&env)); + let dest = create_token_contract(&env, &Address::generate(&env)); + let stellar = token::StellarAssetClient::new(&env, &source); + stellar.mint(&from, &5000); + + let contract_id = env.register(AssetPathPaymentContract, ()); + let client = AssetPathPaymentContractClient::new(&env, &contract_id); + client.init(&admin); + + let path = Vec::new(&env); + + let result = client.try_initiate_path_payment(&from, &from, &source, &dest, &100, &90, &100, &path); + assert_eq!(result, Err(Ok(PathPaymentError::SelfPayment))); +} + #[test] fn test_initiate_path_payment_escrows_tokens() { let env = Env::default(); diff --git a/contracts/bulk_payment/src/lib.rs b/contracts/bulk_payment/src/lib.rs index 0d9bf6e3..6bc1cdbe 100644 --- a/contracts/bulk_payment/src/lib.rs +++ b/contracts/bulk_payment/src/lib.rs @@ -1091,7 +1091,7 @@ impl BulkPaymentContract { let status = if fail_count == 0 { symbol_short!("completed") } else if actual_success == 0 { - symbol_short!("rollbck") + symbol_short!("rollback") } else { symbol_short!("partial") }; @@ -1722,7 +1722,7 @@ impl BulkPaymentContract { let status = if fail_count == 0 { symbol_short!("completed") } else if success_count == 0 { - symbol_short!("rollbck") + symbol_short!("rollback") } else { symbol_short!("partial") }; diff --git a/contracts/bulk_payment/src/test.rs b/contracts/bulk_payment/src/test.rs index f21409b7..9cb73eb1 100644 --- a/contracts/bulk_payment/src/test.rs +++ b/contracts/bulk_payment/src/test.rs @@ -251,7 +251,7 @@ fn test_partial_batch_skips_insufficient_funds() { } #[test] -fn test_partial_batch_all_fail_status_is_rollbck() { +fn test_partial_batch_all_fail_status_is_rollback() { let (env, sender, token, client) = setup(); let mut payments: Vec = Vec::new(&env); payments.push_back(PaymentOp { @@ -269,6 +269,26 @@ fn test_partial_batch_all_fail_status_is_rollbck() { assert_eq!(result.failures.get(0).unwrap().index, 0); } +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_partial_batch_overflow_returns_error() { + let (env, sender, token, client) = setup(); + + let mut payments: Vec = Vec::new(&env); + payments.push_back(PaymentOp { + recipient: Address::generate(&env), + amount: i128::MAX, + category: soroban_sdk::symbol_short!("payroll"), + }); + payments.push_back(PaymentOp { + recipient: Address::generate(&env), + amount: 1, + category: soroban_sdk::symbol_short!("payroll"), + }); + + client.execute_batch_partial(&sender, &token, &payments, &0); +} + #[test] #[should_panic(expected = "Error(Contract, #4)")] fn test_partial_batch_empty_panics() { @@ -1047,9 +1067,9 @@ fn test_v2_partial_invalid_recorded_as_failed() { } /// When ALL payments in a partial batch are invalid, the batch status is -/// "rollbck" (no funds were pulled or held). +/// "rollback" (no funds were pulled or held). #[test] -fn test_v2_partial_all_fail_status_rollbck() { +fn test_v2_partial_all_fail_status_rollback() { let (env, sender, token, client) = setup(); let mut payments: Vec = Vec::new(&env); @@ -1070,7 +1090,7 @@ fn test_v2_partial_all_fail_status_rollbck() { let record = client.get_batch(&batch_id); assert_eq!(record.success_count, 0); assert_eq!(record.fail_count, 2); - assert_eq!(record.status, soroban_sdk::symbol_short!("rollbck")); + assert_eq!(record.status, soroban_sdk::symbol_short!("rollback")); // Sender balance is unchanged — nothing was pulled. let tc = TokenClient::new(&env, &token); diff --git a/contracts/milestone_escrow/src/lib.rs b/contracts/milestone_escrow/src/lib.rs index 16dc9cc7..cc3da6ee 100644 --- a/contracts/milestone_escrow/src/lib.rs +++ b/contracts/milestone_escrow/src/lib.rs @@ -342,6 +342,13 @@ impl MilestoneEscrowContract { Ok(()) } + /// Release funds for an approved milestone. + /// + /// Soroban executes each transaction atomically within a single thread, so + /// two concurrent releases for the *same* escrow cannot interleave within a + /// single execution. However, we defensively re-derive the remaining + /// balance from milestone state before transferring so that future changes + /// (e.g. batched or cross-contract calls) cannot cause a double-spend. pub fn release_milestone( e: Env, escrow_id: u64, @@ -378,6 +385,16 @@ impl MilestoneEscrowContract { } } + // Invariant: re-derive remaining balance from milestone state so that + // concurrent or cross-contract callers cannot double-spend. + let remaining_from_state = record + .total_amount + .checked_sub(record.released_amount) + .ok_or(ContractError::InvalidAmount)?; + if remaining_from_state < milestone.amount { + return Err(ContractError::InsufficientEscrowBalance); + } + let contract_balance = token::Client::new(&e, &record.token).balance(&e.current_contract_address()); if contract_balance < milestone.amount { diff --git a/contracts/milestone_escrow/src/test.rs b/contracts/milestone_escrow/src/test.rs index 19fc23d5..ae32afa5 100644 --- a/contracts/milestone_escrow/src/test.rs +++ b/contracts/milestone_escrow/src/test.rs @@ -773,3 +773,55 @@ fn test_cancel_with_approved_milestones_recovers_full_unreleased() { assert_eq!(token_client.balance(&sender), 1_000_000); assert_eq!(token_client.balance(&beneficiary), 0); } + +// ============================================================================== +// -- Issue #883: sequential release invariant ---------------------------------- +// ============================================================================== + +/// Sequential releases across milestones must never allow balances to go +/// negative or double-spend. The invariant check re-derives remaining balance +/// from milestone state before each transfer. +#[test] +fn test_sequential_release_balances_never_negative() { + let (e, sender, beneficiary, verifier, token, token_client, _, client) = setup(); + // Three milestones: [1000, 2000, 3000], total = 6000. + let milestones = make_milestones(&e, &[1000, 2000, 3000]); + let escrow_id = client.create_escrow(&sender, &beneficiary, &verifier, &token, &milestones); + + // Approve all milestones on separate ledgers. + e.ledger().set_sequence_number(1); + client.approve_milestone(&escrow_id, &0); + e.ledger().set_sequence_number(2); + client.approve_milestone(&escrow_id, &1); + e.ledger().set_sequence_number(3); + client.approve_milestone(&escrow_id, &2); + + // Release milestone 0. + e.ledger().set_sequence_number(4); + client.release_milestone(&escrow_id, &0); + let record = client.get_escrow(&escrow_id); + assert_eq!(record.released_amount, 1000); + assert!(record.is_active); + + // Release milestone 1. + e.ledger().set_sequence_number(5); + client.release_milestone(&escrow_id, &1); + let record = client.get_escrow(&escrow_id); + assert_eq!(record.released_amount, 3000); + assert!(record.is_active); + + // Release milestone 2. + e.ledger().set_sequence_number(6); + client.release_milestone(&escrow_id, &2); + let record = client.get_escrow(&escrow_id); + assert_eq!(record.released_amount, 6000); + assert!(!record.is_active); + + // Beneficiary received exactly the sum of all milestones. + assert_eq!(token_client.balance(&beneficiary), 6000); + // Contract holds zero. + assert_eq!( + token_client.balance(&e.current_contract_address()), + 0 + ); +}