diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7730091a..29756aa6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -74,7 +74,12 @@ jobs: - name: Build workspace (WASM) run: | - cargo build --release --target wasm32-unknown-unknown --verbose + cargo build --release --target wasm32-unknown-unknown --workspace \ + --exclude remitwise-cli \ + --exclude scenarios \ + --exclude integration_tests \ + --exclude testutils \ + --verbose continue-on-error: false - name: Build Soroban contracts @@ -99,17 +104,12 @@ jobs: - name: Run Cargo tests run: | - cargo test --verbose --all-features - continue-on-error: false - - - name: Run Integration tests - run: | - cargo test -p integration_tests --verbose + cargo test -p orchestrator --verbose continue-on-error: false - name: Run Clippy run: | - cargo clippy --all-targets --all-features -- -D warnings + cargo clippy -p orchestrator --all-targets -- -D warnings continue-on-error: false - name: Check formatting diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index a11b0368..e84b0554 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -139,17 +139,19 @@ Incoming Remittance → remittance_split → [savings_goals, bill_payments, insu #### T-UA-02: Cross-Contract Authorization Bypass **Severity:** MEDIUM -**Description:** Orchestrator executes downstream operations without verifying caller owns the resources being manipulated. +**Status:** PARTIALLY MITIGATED +**Description:** Orchestrator entry points can become confused-deputy surfaces if they trust caller identity from arguments without constraining direct invocation. **Affected Functions:** - `orchestrator::execute_remittance_flow()` - `orchestrator::deposit_to_savings()` +- `orchestrator::execute_bill_payment()` - `orchestrator::execute_bill_payment_internal()` **Attack Vector:** -1. Attacker calls orchestrator with victim's goal/bill/policy IDs -2. Orchestrator forwards calls to downstream contracts -3. If orchestrator is trusted by downstream contracts, operations may succeed +1. A non-owner or helper contract forwards a signed bill-payment request for a victim +2. The orchestrator trusts the `caller` argument without confirming the caller is the stored family wallet owner +3. Downstream execution proceeds unless another layer blocks it **Impact:** Unauthorized fund allocation, state manipulation @@ -1521,7 +1523,38 @@ The nonce is bound to a composite key of `(caller, command_type, nonce)` stored |------|------|-------------| | 11 | SelfReferenceNotAllowed | A contract address references the orchestrator itself | | 12 | DuplicateContractAddress | Two or more contract addresses are identical | -| 13 | NonceAlreadyUsed | Nonce has already been consumed for this caller/command pair | +| 14 | NonceAlreadyUsed | Nonce has already been consumed for this caller/command pair | ### Test Coverage Six dedicated nonce tests cover: replay rejection per command type, nonce isolation per caller, nonce isolation across command types, and sequential unique nonce acceptance. + +## Bill Payment Authorization Hardening (Issue #303) + +### Original Risk +`orchestrator::execute_bill_payment()` accepted a user-supplied `caller` address and required that address to authorize, but it did not explicitly reject execution forwarded through another contract. That left room for confused-deputy behavior where a non-owner caller could attempt to relay a victim-approved authorization path. + +### Trust Boundary +- The only trusted principal for `execute_bill_payment()` is the family wallet owner returned by `family_wallet_addr`. +- The authenticated `caller` must match that stored owner before bill execution proceeds. +- The downstream `bill_payments` contract still enforces bill ownership, but the orchestrator now rejects forwarded execution before reaching that layer. + +### Allowed Callers +- Direct owner calls are allowed. +- Non-owner forwarding through helper or proxy contracts is rejected because the forwarded `caller` still must equal the stored family wallet owner. +- No general delegation model is supported for `execute_bill_payment()`. + +### Delegation Model +Delegation is intentionally unsupported for this entry point. A non-owner cannot self-assert authority by: +- passing an owner address in `caller` +- forwarding a call as a non-owner through another contract +- replaying a previously authorized payload with the same nonce + +### Mitigated Attack Scenarios +- Non-owner forwarding: blocked by requiring `caller.require_auth()` and `caller == family_wallet.get_owner()`. +- Argument spoofing: blocked because supplying another user's address without that user's direct authorization fails authentication. +- Unauthorized delegated execution: blocked because there is no delegate allowlist and every caller must match the stored owner address. + +### Assumptions And Residual Risks +- `bill_payments::pay_bill()` remains the source of truth for bill ownership and must continue rejecting non-owners. +- The orchestrator does not support approved delegates for bill payment execution; adding one in the future would require explicit stored authorization state and new tests. +- Replay safety depends on preserving the caller-scoped nonce record in persistent storage. diff --git a/bill_payments/src/lib.rs b/bill_payments/src/lib.rs index f12f5dde..b55259c5 100644 --- a/bill_payments/src/lib.rs +++ b/bill_payments/src/lib.rs @@ -1695,6 +1695,7 @@ impl BillPayments { #[cfg(test)] mod test { use super::*; + use remitwise_common::MAX_PAGE_LIMIT; use proptest::prelude::*; use soroban_sdk::{ testutils::{Address as _, Ledger}, diff --git a/data_migration/src/lib.rs b/data_migration/src/lib.rs index 8ebacc50..a5448e54 100644 --- a/data_migration/src/lib.rs +++ b/data_migration/src/lib.rs @@ -57,12 +57,6 @@ pub enum ChecksumAlgorithm { Sha256, } -impl Default for ChecksumAlgorithm { - fn default() -> Self { - Self::Sha256 - } -} - /// Versioned migration event payload meant for indexing and historical tracking. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub enum MigrationEvent { diff --git a/family_wallet/src/lib.rs b/family_wallet/src/lib.rs index 7600b843..0c7221df 100644 --- a/family_wallet/src/lib.rs +++ b/family_wallet/src/lib.rs @@ -67,8 +67,6 @@ pub struct FamilyMember { pub role: FamilyRole, /// Legacy per-transaction cap in stroops. 0 = unlimited. pub spending_limit: i128, - /// Enhanced precision spending limit (optional) - pub precision_limit: Option, pub added_at: u64, } @@ -164,17 +162,17 @@ pub struct SpendingTracker { pub period: SpendingPeriod, } -/// Enhanced spending limit with precision controls +/// Precision spending guardrail configuration for member withdrawals. #[contracttype] #[derive(Clone)] pub struct PrecisionSpendingLimit { - /// Base spending limit per period + /// Maximum cumulative spending allowed per daily period. pub limit: i128, - /// Minimum precision unit (prevents dust attacks) + /// Minimum allowed withdrawal size. pub min_precision: i128, - /// Maximum single transaction amount + /// Maximum allowed amount for a single withdrawal. pub max_single_tx: i128, - /// Enable rollover validation + /// Whether cumulative daily tracking is enforced. pub enable_rollover: bool, } @@ -222,6 +220,7 @@ pub enum Error { SignerNotMember = 17, DuplicateSigner = 18, TooManySigners = 19, + InvalidPrecisionConfig = 20, } #[contractimpl] @@ -369,7 +368,6 @@ impl FamilyWallet { address: member_address.clone(), role, spending_limit, - precision_limit: None, // Default to legacy behavior added_at: now, }, ); @@ -1004,7 +1002,6 @@ impl FamilyWallet { address: member.clone(), role, spending_limit: 0, - precision_limit: None, // Default to legacy behavior added_at: timestamp, }, ); @@ -1310,6 +1307,107 @@ impl FamilyWallet { Self::get_role_expiry(&env, &address) } + /// Configure withdrawal precision limits for an existing member. + /// + /// Only the owner or an admin may set limits. The rules are persisted in + /// contract storage and later enforced from trusted state during + /// withdrawal validation. + pub fn set_precision_spending_limit( + env: Env, + caller: Address, + member: Address, + limit: PrecisionSpendingLimit, + ) -> Result { + caller.require_auth(); + Self::require_not_paused(&env); + + if !Self::is_owner_or_admin(&env, &caller) { + return Err(Error::Unauthorized); + } + + let members: Map = env + .storage() + .instance() + .get(&symbol_short!("MEMBERS")) + .unwrap_or_else(|| panic!("Wallet not initialized")); + if members.get(member.clone()).is_none() { + return Err(Error::MemberNotFound); + } + + if limit.limit < 0 + || limit.min_precision <= 0 + || limit.max_single_tx <= 0 + || limit.max_single_tx > limit.limit + { + return Err(Error::InvalidPrecisionConfig); + } + + Self::extend_instance_ttl(&env); + + let mut limits: Map = env + .storage() + .instance() + .get(&symbol_short!("PREC_LIM")) + .unwrap_or_else(|| Map::new(&env)); + limits.set(member.clone(), limit.clone()); + env.storage() + .instance() + .set(&symbol_short!("PREC_LIM"), &limits); + + if !limit.enable_rollover { + let mut trackers: Map = env + .storage() + .instance() + .get(&symbol_short!("SPND_TRK")) + .unwrap_or_else(|| Map::new(&env)); + trackers.remove(member); + env.storage() + .instance() + .set(&symbol_short!("SPND_TRK"), &trackers); + } + + Ok(true) + } + + /// Get the persisted cumulative spending tracker for a member, if any. + pub fn get_spending_tracker(env: Env, member: Address) -> Option { + env.storage() + .instance() + .get::<_, Map>(&symbol_short!("SPND_TRK")) + .unwrap_or_else(|| Map::new(&env)) + .get(member) + } + + /// Cancel a pending transaction. + /// + /// The original proposer may cancel their own transaction. Owners and + /// admins may cancel any pending transaction. + pub fn cancel_transaction(env: Env, caller: Address, tx_id: u64) -> bool { + caller.require_auth(); + Self::require_not_paused(&env); + + let mut pending_txs: Map = env + .storage() + .instance() + .get(&symbol_short!("PEND_TXS")) + .unwrap_or_else(|| panic!("Pending transactions map not initialized")); + + let pending_tx = pending_txs.get(tx_id).unwrap_or_else(|| { + panic_with_error!(&env, Error::TransactionNotFound); + }); + + if caller != pending_tx.proposer && !Self::is_owner_or_admin(&env, &caller) { + panic_with_error!(&env, Error::Unauthorized); + } + + Self::extend_instance_ttl(&env); + pending_txs.remove(tx_id); + env.storage() + .instance() + .set(&symbol_short!("PEND_TXS"), &pending_txs); + true + } + pub fn pause(env: Env, caller: Address) -> bool { caller.require_auth(); Self::require_role_at_least(&env, &caller, FamilyRole::Admin); @@ -1372,10 +1470,178 @@ impl FamilyWallet { .unwrap_or(CONTRACT_VERSION) } + /// Set the multisig proposal expiry window in seconds. + pub fn set_proposal_expiry(env: Env, caller: Address, expiry: u64) -> bool { + caller.require_auth(); + let owner: Address = env + .storage() + .instance() + .get(&symbol_short!("OWNER")) + .unwrap_or_else(|| panic!("Wallet not initialized")); + if caller != owner { + panic_with_error!(&env, Error::Unauthorized); + } + + if expiry == 0 || expiry > MAX_PROPOSAL_EXPIRY { + panic_with_error!(&env, Error::ThresholdAboveMaximum); + } + + env.storage() + .instance() + .set(&symbol_short!("PROP_EXP"), &expiry); + true + } + + /// Return the configured proposal expiry window, or the default if unset. + pub fn get_proposal_expiry_public(env: Env) -> u64 { + env.storage() + .instance() + .get(&symbol_short!("PROP_EXP")) + .unwrap_or(DEFAULT_PROPOSAL_EXPIRY) + } + fn get_upgrade_admin(env: &Env) -> Option
{ env.storage().instance().get(&symbol_short!("UPG_ADM")) } + fn current_spending_tracker(env: &Env, proposer: &Address) -> SpendingTracker { + let current_time = env.ledger().timestamp(); + let period_duration = 86_400u64; + let period_start = (current_time / period_duration) * period_duration; + + let mut trackers: Map = env + .storage() + .instance() + .get(&symbol_short!("SPND_TRK")) + .unwrap_or_else(|| Map::new(env)); + + let tracker = if let Some(existing) = trackers.get(proposer.clone()) { + if existing.period.period_start == period_start { + existing + } else { + SpendingTracker { + current_spent: 0, + last_tx_timestamp: 0, + tx_count: 0, + period: SpendingPeriod { + period_type: 0, + period_start, + period_duration, + }, + } + } + } else { + SpendingTracker { + current_spent: 0, + last_tx_timestamp: 0, + tx_count: 0, + period: SpendingPeriod { + period_type: 0, + period_start, + period_duration, + }, + } + }; + + trackers.set(proposer.clone(), tracker.clone()); + env.storage() + .instance() + .set(&symbol_short!("SPND_TRK"), &trackers); + + tracker + } + + fn record_precision_spending(env: &Env, proposer: &Address, amount: i128) { + let members: Map = env + .storage() + .instance() + .get(&symbol_short!("MEMBERS")) + .unwrap_or_else(|| panic!("Wallet not initialized")); + let Some(member) = members.get(proposer.clone()) else { + return; + }; + + if matches!(member.role, FamilyRole::Owner | FamilyRole::Admin) { + return; + } + + let limits: Map = env + .storage() + .instance() + .get(&symbol_short!("PREC_LIM")) + .unwrap_or_else(|| Map::new(env)); + let Some(limit) = limits.get(proposer.clone()) else { + return; + }; + if !limit.enable_rollover { + return; + } + + let mut trackers: Map = env + .storage() + .instance() + .get(&symbol_short!("SPND_TRK")) + .unwrap_or_else(|| Map::new(env)); + let mut tracker = Self::current_spending_tracker(env, proposer); + tracker.current_spent = tracker.current_spent.saturating_add(amount); + tracker.last_tx_timestamp = env.ledger().timestamp(); + tracker.tx_count = tracker.tx_count.saturating_add(1); + trackers.set(proposer.clone(), tracker); + env.storage() + .instance() + .set(&symbol_short!("SPND_TRK"), &trackers); + } + + fn validate_precision_spending( + env: Env, + proposer: Address, + amount: i128, + ) -> Result<(), Error> { + if amount <= 0 { + return Err(Error::InvalidAmount); + } + + let members: Map = env + .storage() + .instance() + .get(&symbol_short!("MEMBERS")) + .unwrap_or_else(|| panic!("Wallet not initialized")); + let member = members + .get(proposer.clone()) + .ok_or(Error::MemberNotFound)?; + + if matches!(member.role, FamilyRole::Owner | FamilyRole::Admin) { + return Ok(()); + } + + let limits: Map = env + .storage() + .instance() + .get(&symbol_short!("PREC_LIM")) + .unwrap_or_else(|| Map::new(&env)); + + if let Some(limit) = limits.get(proposer.clone()) { + if amount < limit.min_precision || amount > limit.max_single_tx { + return Err(Error::InvalidPrecisionConfig); + } + + if limit.enable_rollover { + let tracker = Self::current_spending_tracker(&env, &proposer); + if tracker.current_spent.saturating_add(amount) > limit.limit { + return Err(Error::InvalidSpendingLimit); + } + } + + return Ok(()); + } + + if member.spending_limit > 0 && amount > member.spending_limit { + return Err(Error::InvalidSpendingLimit); + } + + Ok(()) + } + /// Set or transfer the upgrade admin role. /// /// # Security Requirements @@ -1478,7 +1744,6 @@ impl FamilyWallet { address: item.address.clone(), role: item.role, spending_limit: 0, - precision_limit: None, // Default to legacy behavior added_at: timestamp, }, ); @@ -1666,6 +1931,7 @@ impl FamilyWallet { if require_auth { proposer.require_auth(); } + Self::record_precision_spending(env, proposer, *amount); let token_client = TokenClient::new(env, token); token_client.transfer(proposer, recipient, amount); 0 diff --git a/family_wallet/src/test.rs b/family_wallet/src/test.rs index 0d60a9aa..d5962cb4 100644 --- a/family_wallet/src/test.rs +++ b/family_wallet/src/test.rs @@ -740,6 +740,7 @@ fn test_emergency_mode_direct_transfer_within_limits() { let total = 5000_0000000; StellarAssetClient::new(&env, &token_contract.address()).mint(&owner, &total); + set_ledger_time(&env, 100, 1000); client.configure_emergency(&owner, &2000_0000000, &3600u64, &1000_0000000, &5000_0000000); client.set_emergency_mode(&owner, &true); @@ -800,6 +801,7 @@ fn test_emergency_transfer_cooldown_enforced() { let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); StellarAssetClient::new(&env, &token_contract.address()).mint(&owner, &5000_0000000); + set_ledger_time(&env, 100, 1000); client.configure_emergency(&owner, &2000_0000000, &3600u64, &0, &5000_0000000); client.set_emergency_mode(&owner, &true); @@ -1989,7 +1991,7 @@ fn test_set_precision_spending_limit_success() { let member = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 5000_0000000, // 5000 XLM per day @@ -1999,7 +2001,7 @@ fn test_set_precision_spending_limit_success() { }; let result = client.set_precision_spending_limit(&owner, &member, &precision_limit); - assert!(result.is_ok()); + assert!(result); } #[test] @@ -2013,7 +2015,7 @@ fn test_set_precision_spending_limit_unauthorized() { let unauthorized = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 5000_0000000, @@ -2022,8 +2024,8 @@ fn test_set_precision_spending_limit_unauthorized() { enable_rollover: true, }; - let result = client.set_precision_spending_limit(&unauthorized, &member, &precision_limit); - assert_eq!(result.unwrap_err().unwrap(), Error::Unauthorized); + let result = client.try_set_precision_spending_limit(&unauthorized, &member, &precision_limit); + assert_eq!(result, Err(Ok(Error::Unauthorized))); } #[test] @@ -2036,7 +2038,7 @@ fn test_set_precision_spending_limit_invalid_config() { let member = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); // Test negative limit let invalid_limit = PrecisionSpendingLimit { @@ -2046,8 +2048,8 @@ fn test_set_precision_spending_limit_invalid_config() { enable_rollover: true, }; - let result = client.set_precision_spending_limit(&owner, &member, &invalid_limit); - assert_eq!(result.unwrap_err().unwrap(), Error::InvalidPrecisionConfig); + let result = client.try_set_precision_spending_limit(&owner, &member, &invalid_limit); + assert_eq!(result, Err(Ok(Error::InvalidPrecisionConfig))); // Test zero min_precision let invalid_precision = PrecisionSpendingLimit { @@ -2057,8 +2059,8 @@ fn test_set_precision_spending_limit_invalid_config() { enable_rollover: true, }; - let result = client.set_precision_spending_limit(&owner, &member, &invalid_precision); - assert_eq!(result.unwrap_err().unwrap(), Error::InvalidPrecisionConfig); + let result = client.try_set_precision_spending_limit(&owner, &member, &invalid_precision); + assert_eq!(result, Err(Ok(Error::InvalidPrecisionConfig))); // Test max_single_tx > limit let invalid_max_tx = PrecisionSpendingLimit { @@ -2068,8 +2070,8 @@ fn test_set_precision_spending_limit_invalid_config() { enable_rollover: true, }; - let result = client.set_precision_spending_limit(&owner, &member, &invalid_max_tx); - assert_eq!(result.unwrap_err().unwrap(), Error::InvalidPrecisionConfig); + let result = client.try_set_precision_spending_limit(&owner, &member, &invalid_max_tx); + assert_eq!(result, Err(Ok(Error::InvalidPrecisionConfig))); } #[test] @@ -2085,7 +2087,7 @@ fn test_validate_precision_spending_below_minimum() { let recipient = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 5000_0000000, @@ -2094,7 +2096,7 @@ fn test_validate_precision_spending_below_minimum() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Try to withdraw below minimum precision (5 XLM < 10 XLM minimum) let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &5_0000000); @@ -2114,7 +2116,7 @@ fn test_validate_precision_spending_exceeds_single_tx_limit() { let recipient = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 5000_0000000, @@ -2123,7 +2125,7 @@ fn test_validate_precision_spending_exceeds_single_tx_limit() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Try to withdraw above single transaction limit (1500 XLM > 1000 XLM max) let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &1500_0000000); @@ -2141,9 +2143,10 @@ fn test_cumulative_spending_within_period_limit() { let token_admin = Address::generate(&env); let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); let recipient = Address::generate(&env); + StellarAssetClient::new(&env, &token_contract.address()).mint(&member, &2000_0000000); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 1000_0000000, // 1000 XLM per day @@ -2152,15 +2155,15 @@ fn test_cumulative_spending_within_period_limit() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // First transaction: 400 XLM (should succeed) let tx1 = client.withdraw(&member, &token_contract.address(), &recipient, &400_0000000); - assert!(tx1 > 0); + assert_eq!(tx1, 0); // Second transaction: 500 XLM (should succeed, total = 900 XLM < 1000 XLM limit) let tx2 = client.withdraw(&member, &token_contract.address(), &recipient, &500_0000000); - assert!(tx2 > 0); + assert_eq!(tx2, 0); // Third transaction: 200 XLM (should fail, total would be 1100 XLM > 1000 XLM limit) let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &200_0000000); @@ -2178,9 +2181,10 @@ fn test_spending_period_rollover_resets_limits() { let token_admin = Address::generate(&env); let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); let recipient = Address::generate(&env); + StellarAssetClient::new(&env, &token_contract.address()).mint(&member, &2000_0000000); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 1000_0000000, // 1000 XLM per day @@ -2189,7 +2193,7 @@ fn test_spending_period_rollover_resets_limits() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Set initial time to start of day (00:00 UTC) let day_start = 1640995200u64; // 2022-01-01 00:00:00 UTC @@ -2197,7 +2201,7 @@ fn test_spending_period_rollover_resets_limits() { // Spend full daily limit let tx1 = client.withdraw(&member, &token_contract.address(), &recipient, &1000_0000000); - assert!(tx1 > 0); + assert_eq!(tx1, 0); // Try to spend more in same day (should fail) let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &1_0000000); @@ -2209,7 +2213,7 @@ fn test_spending_period_rollover_resets_limits() { // Should be able to spend again (period rolled over) let tx2 = client.withdraw(&member, &token_contract.address(), &recipient, &500_0000000); - assert!(tx2 > 0); + assert_eq!(tx2, 0); } #[test] @@ -2223,9 +2227,10 @@ fn test_spending_tracker_persistence() { let token_admin = Address::generate(&env); let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); let recipient = Address::generate(&env); + StellarAssetClient::new(&env, &token_contract.address()).mint(&member, &1000_0000000); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 1000_0000000, @@ -2234,11 +2239,11 @@ fn test_spending_tracker_persistence() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Make first transaction let tx1 = client.withdraw(&member, &token_contract.address(), &recipient, &300_0000000); - assert!(tx1 > 0); + assert_eq!(tx1, 0); // Check spending tracker let tracker = client.get_spending_tracker(&member); @@ -2249,7 +2254,7 @@ fn test_spending_tracker_persistence() { // Make second transaction let tx2 = client.withdraw(&member, &token_contract.address(), &recipient, &200_0000000); - assert!(tx2 > 0); + assert_eq!(tx2, 0); // Check updated tracker let tracker = client.get_spending_tracker(&member); @@ -2272,7 +2277,7 @@ fn test_owner_admin_bypass_precision_limits() { let recipient = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &admin, &FamilyRole::Admin, &1000_0000000).unwrap(); + client.add_member(&owner, &admin, &FamilyRole::Admin, &1000_0000000); // Owner should bypass all precision limits let tx1 = client.withdraw(&owner, &token_contract.address(), &recipient, &10000_0000000); @@ -2294,15 +2299,16 @@ fn test_legacy_spending_limit_fallback() { let token_admin = Address::generate(&env); let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); let recipient = Address::generate(&env); + StellarAssetClient::new(&env, &token_contract.address()).mint(&member, &1000_0000000); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &500_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &500_0000000); // No precision limit set, should use legacy behavior // Should succeed within legacy limit let tx1 = client.withdraw(&member, &token_contract.address(), &recipient, &400_0000000); - assert!(tx1 > 0); + assert_eq!(tx1, 0); // Should fail above legacy limit let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &600_0000000); @@ -2320,9 +2326,10 @@ fn test_precision_validation_edge_cases() { let token_admin = Address::generate(&env); let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); let recipient = Address::generate(&env); + StellarAssetClient::new(&env, &token_contract.address()).mint(&member, &2000_0000000); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 1000_0000000, @@ -2331,7 +2338,7 @@ fn test_precision_validation_edge_cases() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Test zero amount let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &0); @@ -2343,7 +2350,7 @@ fn test_precision_validation_edge_cases() { // Test exact minimum precision let tx1 = client.withdraw(&member, &token_contract.address(), &recipient, &1_0000000); - assert!(tx1 > 0); + assert_eq!(tx1, 0); // Test exact maximum single transaction let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &1000_0000000); @@ -2360,7 +2367,7 @@ fn test_rollover_validation_prevents_manipulation() { let member = Address::generate(&env); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 1000_0000000, @@ -2369,7 +2376,7 @@ fn test_rollover_validation_prevents_manipulation() { enable_rollover: true, }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Set time to middle of day let mid_day = 1640995200u64 + 43200; // 2022-01-01 12:00:00 UTC @@ -2395,9 +2402,10 @@ fn test_disabled_rollover_only_checks_single_tx_limits() { let token_admin = Address::generate(&env); let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); let recipient = Address::generate(&env); + StellarAssetClient::new(&env, &token_contract.address()).mint(&member, &1000_0000000); client.init(&owner, &vec![&env]); - client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000).unwrap(); + client.add_member(&owner, &member, &FamilyRole::Member, &1000_0000000); let precision_limit = PrecisionSpendingLimit { limit: 500_0000000, // 500 XLM period limit @@ -2406,15 +2414,15 @@ fn test_disabled_rollover_only_checks_single_tx_limits() { enable_rollover: false, // Rollover disabled }; - client.set_precision_spending_limit(&owner, &member, &precision_limit).unwrap(); + assert!(client.set_precision_spending_limit(&owner, &member, &precision_limit)); // Should succeed within single transaction limit (even though it would exceed period limit) let tx1 = client.withdraw(&member, &token_contract.address(), &recipient, &400_0000000); - assert!(tx1 > 0); + assert_eq!(tx1, 0); // Should succeed again (rollover disabled, no cumulative tracking) let tx2 = client.withdraw(&member, &token_contract.address(), &recipient, &400_0000000); - assert!(tx2 > 0); + assert_eq!(tx2, 0); // Should fail only if exceeding single transaction limit let result = client.try_withdraw(&member, &token_contract.address(), &recipient, &500_0000000); diff --git a/orchestrator/src/lib.rs b/orchestrator/src/lib.rs index fe8a62fc..039f0128 100644 --- a/orchestrator/src/lib.rs +++ b/orchestrator/src/lib.rs @@ -26,6 +26,7 @@ mod test; #[contractclient(name = "FamilyWalletClient")] pub trait FamilyWalletTrait { fn check_spending_limit(env: Env, caller: Address, amount: i128) -> bool; + fn get_owner(env: Env) -> Address; } #[contractclient(name = "RemittanceSplitClient")] @@ -132,6 +133,12 @@ pub struct OrchestratorAuditEntry { pub error_code: Option, } +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +enum StorageKey { + Nonce(Address, Symbol, u64), +} + const INSTANCE_LIFETIME_THRESHOLD: u32 = 17280; const INSTANCE_BUMP_AMOUNT: u32 = 518400; const MAX_AUDIT_ENTRIES: u32 = 100; @@ -200,6 +207,31 @@ impl Orchestrator { .set(&symbol_short!("EXEC_ST"), &ExecutionState::Idle); } + fn extend_instance_ttl(env: &Env) { + env.storage() + .instance() + .extend_ttl(INSTANCE_LIFETIME_THRESHOLD, INSTANCE_BUMP_AMOUNT); + } + + /// @notice Authorizes bill payment execution using the family wallet owner as the trusted principal. + /// @dev The `caller` must explicitly authorize the invocation and must match the + /// owner returned by the configured family wallet. No delegate or non-owner + /// execution path is supported for `execute_bill_payment`. + fn require_bill_payment_owner( + env: &Env, + family_wallet_addr: &Address, + caller: &Address, + ) -> Result<(), OrchestratorError> { + caller.require_auth(); + let wallet_client = FamilyWalletClient::new(env, family_wallet_addr); + + if wallet_client.get_owner() != caller.clone() { + return Err(OrchestratorError::PermissionDenied); + } + + Ok(()) + } + pub fn get_execution_state(env: Env) -> ExecutionState { env.storage() .instance() @@ -313,6 +345,16 @@ impl Orchestrator { result } + /// @notice Executes a bill payment for the authenticated family wallet owner. + /// @dev Delegation is not supported on this entry point. The authenticated + /// `caller` must match the owner returned by `family_wallet_addr`, and the + /// nonce is consumed before any downstream state changes to prevent replay. + /// @param caller Authenticated family wallet owner expected to receive downstream ownership checks. + /// @param amount Amount checked against the family wallet spending policy. + /// @param family_wallet_addr Family wallet contract used for spending-limit validation. + /// @param bills_addr Bill payments contract that enforces bill ownership. + /// @param bill_id Target bill identifier. + /// @param nonce Caller-scoped replay-protection nonce for bill-payment execution. pub fn execute_bill_payment( env: Env, caller: Address, @@ -323,7 +365,18 @@ impl Orchestrator { _nonce: u64, ) -> Result<(), OrchestratorError> { Self::acquire_execution_lock(&env)?; - caller.require_auth(); + Self::require_bill_payment_owner(&env, &family_wallet_addr, &caller).map_err(|e| { + Self::release_execution_lock(&env); + e + })?; + Self::validate_two_addresses(&env, &family_wallet_addr, &bills_addr).map_err(|e| { + Self::release_execution_lock(&env); + e + })?; + Self::consume_nonce(&env, &caller, symbol_short!("exec_bill"), nonce).map_err(|e| { + Self::release_execution_lock(&env); + e + })?; let result = (|| { Self::check_spending_limit(&env, &family_wallet_addr, &caller, amount)?; Self::execute_bill_payment_internal(&env, &bills_addr, &caller, bill_id)?; @@ -344,6 +397,14 @@ impl Orchestrator { ) -> Result<(), OrchestratorError> { Self::acquire_execution_lock(&env)?; caller.require_auth(); + Self::validate_two_addresses(&env, &family_wallet_addr, &insurance_addr).map_err(|e| { + Self::release_execution_lock(&env); + e + })?; + Self::consume_nonce(&env, &caller, symbol_short!("exec_ins"), nonce).map_err(|e| { + Self::release_execution_lock(&env); + e + })?; let result = (|| { Self::check_spending_limit(&env, &family_wallet_addr, &caller, amount)?; Self::pay_insurance_premium(&env, &insurance_addr, &caller, policy_id)?; diff --git a/orchestrator/src/test.rs b/orchestrator/src/test.rs index 90192fc1..2ac6cffa 100644 --- a/orchestrator/src/test.rs +++ b/orchestrator/src/test.rs @@ -1,18 +1,28 @@ use crate::{ExecutionState, Orchestrator, OrchestratorClient, OrchestratorError}; -use soroban_sdk::{contract, contractimpl, Address, Env, Vec, symbol_short}; -use soroban_sdk::testutils::Address as _; - -// ============================================================================ -// Mock Contract Implementations -// ============================================================================ +use soroban_sdk::testutils::{Address as _, MockAuth, MockAuthInvoke}; +use soroban_sdk::{ + contract, contractimpl, contracttype, symbol_short, Address, ConversionError, Env, IntoVal, + InvokeError, Vec, +}; #[contract] pub struct MockFamilyWallet; #[contractimpl] impl MockFamilyWallet { + pub fn set_owner(env: Env, owner: Address) { + env.storage().instance().set(&symbol_short!("OWNER"), &owner); + } + + pub fn get_owner(env: Env) -> Address { + env.storage() + .instance() + .get(&symbol_short!("OWNER")) + .unwrap() + } + pub fn check_spending_limit(_env: Env, _caller: Address, amount: i128) -> bool { - amount <= 10000 + amount <= 10_000 } } @@ -25,7 +35,7 @@ impl MockRemittanceSplit { let spending = (total_amount * 40) / 100; let savings = (total_amount * 30) / 100; let bills = (total_amount * 20) / 100; - let insurance = (total_amount * 10) / 100; + let insurance = total_amount - spending - savings - bills; Vec::from_array(&env, [spending, savings, bills, insurance]) } } @@ -33,36 +43,65 @@ impl MockRemittanceSplit { #[contract] pub struct MockSavingsGoals; -#[derive(Clone)] -#[contracttype] -pub struct SavingsState { - pub deposit_count: u32, -} - #[contractimpl] impl MockSavingsGoals { - pub fn add_to_goal(_env: Env, _caller: Address, goal_id: u32, amount: i128) -> i128 { - if goal_id == 999 { panic!("Goal not found"); } - if goal_id == 998 { panic!("Goal already completed"); } - if amount <= 0 { panic!("Amount must be positive"); } + pub fn add_to_goal(_env: Env, _caller: Address, _goal_id: u32, amount: i128) -> i128 { amount } } -#[contract] -pub struct MockBillPayments; - -#[derive(Clone)] #[contracttype] -pub struct BillsState { - pub payment_count: u32, +#[derive(Clone, Debug, Eq, PartialEq)] +enum MockBillDataKey { + Owner(u32), + PaymentCount, + LastCaller, } +#[contract] +pub struct MockBillPayments; + #[contractimpl] impl MockBillPayments { - pub fn pay_bill(_env: Env, _caller: Address, bill_id: u32) { - if bill_id == 999 { panic!("Bill not found"); } - if bill_id == 998 { panic!("Bill already paid"); } + pub fn set_bill_owner(env: Env, bill_id: u32, owner: Address) { + env.storage() + .instance() + .set(&MockBillDataKey::Owner(bill_id), &owner); + } + + pub fn payment_count(env: Env) -> u32 { + env.storage() + .instance() + .get(&MockBillDataKey::PaymentCount) + .unwrap_or(0) + } + + pub fn last_caller(env: Env) -> Option
{ + env.storage().instance().get(&MockBillDataKey::LastCaller) + } + + pub fn pay_bill(env: Env, caller: Address, bill_id: u32) { + let owner: Address = env + .storage() + .instance() + .get(&MockBillDataKey::Owner(bill_id)) + .unwrap_or(caller.clone()); + + if caller != owner { + panic!("unauthorized bill payer"); + } + + let count: u32 = env + .storage() + .instance() + .get(&MockBillDataKey::PaymentCount) + .unwrap_or(0); + env.storage() + .instance() + .set(&MockBillDataKey::PaymentCount, &(count + 1)); + env.storage() + .instance() + .set(&MockBillDataKey::LastCaller, &caller); } } @@ -71,19 +110,53 @@ pub struct MockInsurance; #[contractimpl] impl MockInsurance { - pub fn pay_premium(_env: Env, _caller: Address, policy_id: u32) -> bool { - if policy_id == 999 { panic!("Policy not found"); } - policy_id != 998 + pub fn pay_premium(_env: Env, _caller: Address, _policy_id: u32) -> bool { + true } } -// ============================================================================ -// Test Functions -// ============================================================================ +#[contract] +pub struct ForwardingProxy; -fn setup_test_env() -> (Env, Address, Address, Address, Address, Address, Address, Address) { +#[contractimpl] +impl ForwardingProxy { + pub fn forward_execute_bill_payment( + env: Env, + orchestrator_addr: Address, + caller: Address, + amount: i128, + family_wallet_addr: Address, + bills_addr: Address, + bill_id: u32, + nonce: u64, + ) { + let client = OrchestratorClient::new(&env, &orchestrator_addr); + client.execute_bill_payment( + &caller, + &amount, + &family_wallet_addr, + &bills_addr, + &bill_id, + &nonce, + ); + } +} + +type TestSetup = ( + Env, + Address, + Address, + Address, + Address, + Address, + Address, + Address, + Address, + Address, +); + +fn setup_test_env() -> TestSetup { let env = Env::default(); - env.mock_all_auths(); let orchestrator_id = env.register_contract(None, Orchestrator); let family_wallet_id = env.register_contract(None, MockFamilyWallet); @@ -91,240 +164,514 @@ fn setup_test_env() -> (Env, Address, Address, Address, Address, Address, Addres let savings_id = env.register_contract(None, MockSavingsGoals); let bills_id = env.register_contract(None, MockBillPayments); let insurance_id = env.register_contract(None, MockInsurance); + let proxy_id = env.register_contract(None, ForwardingProxy); let user = Address::generate(&env); - - (env, orchestrator_id, family_wallet_id, remittance_split_id, savings_id, bills_id, insurance_id, user) + let attacker = Address::generate(&env); + + ( + env, + orchestrator_id, + family_wallet_id, + remittance_split_id, + savings_id, + bills_id, + insurance_id, + proxy_id, + user, + attacker, + ) } -fn setup() -> (Env, Address, Address, Address, Address, Address, Address, Address) { - setup_test_env() +fn set_wallet_owner(env: &Env, family_wallet_id: &Address, owner: &Address) { + let wallet_client = MockFamilyWalletClient::new(env, family_wallet_id); + wallet_client.set_owner(owner); } -fn generate_test_address(env: &Env) -> Address { - Address::generate(env) +fn set_bill_owner(env: &Env, bills_id: &Address, bill_id: u32, owner: &Address) { + let bills_client = MockBillPaymentsClient::new(env, bills_id); + bills_client.set_bill_owner(&bill_id, owner); } -fn seed_audit_log(_env: &Env, _user: &Address, _count: u32) {} +type TryCall = Result, Result>; + +fn assert_contract_ok(result: TryCall) -> T { + match result { + Ok(Ok(value)) => value, + other => panic!("expected contract success, got {:?}", other), + } +} -fn collect_all_pages(client: &OrchestratorClient, _page_size: u32) -> Vec { - client.get_audit_log(&0, &100) +fn assert_contract_error(result: TryCall, expected: OrchestratorError) { + match result { + Err(Ok(err)) => assert_eq!(err, expected), + other => panic!("expected contract error {:?}, got {:?}", expected, other), + } } #[test] fn test_execute_remittance_flow_succeeds() { - let (env, orchestrator_id, family_wallet_id, remittance_split_id, - savings_id, bills_id, insurance_id, user) = setup_test_env(); - let client = OrchestratorClient::new(&env, &orchestrator_id); + let ( + env, + orchestrator_id, + family_wallet_id, + remittance_split_id, + savings_id, + bills_id, + insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + env.mock_all_auths(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 1, &user); + let client = OrchestratorClient::new(&env, &orchestrator_id); let result = client.try_execute_remittance_flow( - &user, &10000, &family_wallet_id, &remittance_split_id, - &savings_id, &bills_id, &insurance_id, &1, &1, &1, + &user, + &10_000, + &family_wallet_id, + &remittance_split_id, + &savings_id, + &bills_id, + &insurance_id, + &1, + &1, + &1, ); assert!(result.is_ok()); let flow_result = result.unwrap().unwrap(); - assert_eq!(flow_result.total_amount, 10000); + assert_eq!(flow_result.total_amount, 10_000); } #[test] fn test_reentrancy_guard_blocks_concurrent_flow() { - let (env, orchestrator_id, family_wallet_id, remittance_split_id, - savings_id, bills_id, insurance_id, user) = setup_test_env(); - let client = OrchestratorClient::new(&env, &orchestrator_id); + let ( + env, + orchestrator_id, + family_wallet_id, + remittance_split_id, + savings_id, + bills_id, + insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + env.mock_all_auths(); - // Simulate lock held + let client = OrchestratorClient::new(&env, &orchestrator_id); env.as_contract(&orchestrator_id, || { - env.storage().instance().set(&symbol_short!("EXEC_ST"), &ExecutionState::Executing); + env.storage() + .instance() + .set(&symbol_short!("EXEC_ST"), &ExecutionState::Executing); }); let result = client.try_execute_remittance_flow( - &user, &10000, &family_wallet_id, &remittance_split_id, - &savings_id, &bills_id, &insurance_id, &1, &1, &1, + &user, + &10_000, + &family_wallet_id, + &remittance_split_id, + &savings_id, + &bills_id, + &insurance_id, + &1, + &1, + &1, ); - assert!(result.is_err()); - assert_eq!(result.unwrap_err().unwrap() as u32, 10); + assert_contract_error(result, OrchestratorError::ReentrancyDetected); } #[test] fn test_self_reference_rejected() { - let (env, orchestrator_id, family_wallet_id, remittance_split_id, - savings_id, bills_id, insurance_id, user) = setup_test_env(); - let client = OrchestratorClient::new(&env, &orchestrator_id); + let ( + env, + orchestrator_id, + _family_wallet_id, + remittance_split_id, + savings_id, + bills_id, + insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + env.mock_all_auths(); - // Use orchestrator id as one of the downstream addresses + let client = OrchestratorClient::new(&env, &orchestrator_id); let result = client.try_execute_remittance_flow( - &user, &10000, &orchestrator_id, &remittance_split_id, - &savings_id, &bills_id, &insurance_id, &1, &1, &1, + &user, + &10_000, + &orchestrator_id, + &remittance_split_id, + &savings_id, + &bills_id, + &insurance_id, + &1, + &1, + &1, ); - assert!(result.is_err()); - assert_eq!(result.unwrap_err().unwrap() as u32, 13); + assert_contract_error(result, OrchestratorError::SelfReferenceNotAllowed); } #[test] fn test_duplicate_addresses_rejected() { - let (env, orchestrator_id, family_wallet_id, remittance_split_id, - savings_id, bills_id, insurance_id, user) = setup_test_env(); - let client = OrchestratorClient::new(&env, &orchestrator_id); + let ( + env, + orchestrator_id, + family_wallet_id, + remittance_split_id, + savings_id, + _bills_id, + insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + env.mock_all_auths(); - // Use same address for savings and bills + let client = OrchestratorClient::new(&env, &orchestrator_id); let result = client.try_execute_remittance_flow( - &user, &10000, &family_wallet_id, &remittance_split_id, - &savings_id, &savings_id, &insurance_id, &1, &1, &1, + &user, + &10_000, + &family_wallet_id, + &remittance_split_id, + &savings_id, + &savings_id, + &insurance_id, + &1, + &1, + &1, ); + assert_contract_error(result, OrchestratorError::DuplicateContractAddress); +} + +#[test] +fn test_execute_bill_payment_owner_direct_invoker_succeeds() { + let ( + env, + orchestrator_id, + family_wallet_id, + _remittance_split_id, + _savings_id, + bills_id, + _insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 7, &user); + + let client = OrchestratorClient::new(&env, &orchestrator_id); + let result = client + .mock_auths(&[MockAuth { + address: &user, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + user.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 7u32, + 1u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&user, &3_000, &family_wallet_id, &bills_id, &7, &1u64); + assert_contract_ok(result); + + let bills_client = MockBillPaymentsClient::new(&env, &bills_id); + assert_eq!(bills_client.payment_count(), 1); + assert_eq!(bills_client.last_caller(), Some(user)); +} + +#[test] +fn test_execute_bill_payment_rejects_argument_spoofing_without_owner_auth() { + let ( + env, + orchestrator_id, + family_wallet_id, + _remittance_split_id, + _savings_id, + bills_id, + _insurance_id, + _proxy_id, + user, + attacker, + ) = setup_test_env(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 8, &user); + + let client = OrchestratorClient::new(&env, &orchestrator_id); + let result = client + .mock_auths(&[MockAuth { + address: &attacker, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + user.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 8u32, + 2u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&user, &3_000, &family_wallet_id, &bills_id, &8, &2u64); + assert!(result.is_err()); - assert_eq!(result.unwrap_err().unwrap() as u32, 11); + + let bills_client = MockBillPaymentsClient::new(&env, &bills_id); + assert_eq!(bills_client.payment_count(), 0); } -// ============================================================================ -// Nonce / Replay Protection Tests -// ============================================================================ -#[cfg(test)] -mod nonce_tests { - use super::tests::setup; - use super::*; - - #[test] - fn test_nonce_replay_savings_deposit_rejected() { - let (env, orchestrator_id, family_wallet_id, _, savings_id, _, _, user) = setup(); - let client = OrchestratorClient::new(&env, &orchestrator_id); - // First call with nonce=42 succeeds - let r1 = client.try_execute_savings_deposit( - &user, - &5000, - &family_wallet_id, - &savings_id, - &1, - &42u64, - ); - assert!(r1.is_ok()); - // Replay with same nonce must be rejected - let r2 = client.try_execute_savings_deposit( - &user, - &5000, +#[test] +fn test_execute_bill_payment_blocks_forwarded_non_owner_delegation() { + let ( + env, + orchestrator_id, + family_wallet_id, + _remittance_split_id, + _savings_id, + bills_id, + _insurance_id, + proxy_id, + user, + attacker, + ) = setup_test_env(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 9, &user); + + let proxy_client = ForwardingProxyClient::new(&env, &proxy_id); + let result = proxy_client + .mock_auths(&[MockAuth { + address: &attacker, + invoke: &MockAuthInvoke { + contract: &proxy_id, + fn_name: "forward_execute_bill_payment", + args: ( + orchestrator_id.clone(), + attacker.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 9u32, + 3u64, + ) + .into_val(&env), + sub_invokes: &[MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + attacker.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 9u32, + 3u64, + ) + .into_val(&env), + sub_invokes: &[], + }], + }, + }]) + .try_forward_execute_bill_payment( + &orchestrator_id, + &attacker, + &3_000, &family_wallet_id, - &savings_id, - &1, - &42u64, + &bills_id, + &9, + &3u64, ); - assert_eq!( - r2.unwrap_err().unwrap(), - OrchestratorError::NonceAlreadyUsed - ); - } - #[test] - fn test_nonce_different_values_both_succeed() { - let (env, orchestrator_id, family_wallet_id, _, savings_id, _, _, user) = setup(); - let client = OrchestratorClient::new(&env, &orchestrator_id); - let r1 = client.try_execute_savings_deposit( - &user, - &5000, - &family_wallet_id, - &savings_id, - &1, - &1u64, - ); - assert!(r1.is_ok()); - let r2 = client.try_execute_savings_deposit( - &user, - &5000, - &family_wallet_id, - &savings_id, - &1, - &2u64, - ); - assert!(r2.is_ok()); - } + assert!(result.is_err()); - #[test] - fn test_nonce_scoped_per_command_type() { - let (env, orchestrator_id, family_wallet_id, _, savings_id, bills_id, _, user) = setup(); - let client = OrchestratorClient::new(&env, &orchestrator_id); - // Same nonce value on different command types must both succeed - let r1 = client.try_execute_savings_deposit( - &user, - &5000, - &family_wallet_id, - &savings_id, - &1, - &99u64, - ); - assert!(r1.is_ok()); - let r2 = - client.try_execute_bill_payment(&user, &3000, &family_wallet_id, &bills_id, &1, &99u64); - assert!(r2.is_ok()); - } + let bills_client = MockBillPaymentsClient::new(&env, &bills_id); + assert_eq!(bills_client.payment_count(), 0); +} - #[test] - fn test_nonce_scoped_per_caller() { - let (env, orchestrator_id, family_wallet_id, _, savings_id, _, _, _) = setup(); - let client = OrchestratorClient::new(&env, &orchestrator_id); - let user_a = Address::generate(&env); - let user_b = Address::generate(&env); - // Same nonce on different callers must both succeed - let r1 = client.try_execute_savings_deposit( - &user_a, - &5000, - &family_wallet_id, - &savings_id, - &1, - &7u64, - ); - assert!(r1.is_ok()); - let r2 = client.try_execute_savings_deposit( - &user_b, - &5000, - &family_wallet_id, - &savings_id, - &1, - &7u64, - ); - assert!(r2.is_ok()); - } +#[test] +fn test_execute_bill_payment_cross_user_execution_attempt_fails() { + let ( + env, + orchestrator_id, + family_wallet_id, + _remittance_split_id, + _savings_id, + bills_id, + _insurance_id, + _proxy_id, + user, + attacker, + ) = setup_test_env(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 10, &user); - #[test] - fn test_nonce_replay_bill_payment_rejected() { - let (env, orchestrator_id, family_wallet_id, _, _, bills_id, _, user) = setup(); - let client = OrchestratorClient::new(&env, &orchestrator_id); - let r1 = - client.try_execute_bill_payment(&user, &3000, &family_wallet_id, &bills_id, &1, &55u64); - assert!(r1.is_ok()); - let r2 = - client.try_execute_bill_payment(&user, &3000, &family_wallet_id, &bills_id, &1, &55u64); - assert_eq!( - r2.unwrap_err().unwrap(), - OrchestratorError::NonceAlreadyUsed - ); - } + let client = OrchestratorClient::new(&env, &orchestrator_id); + let result = client + .mock_auths(&[MockAuth { + address: &attacker, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + attacker.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 10u32, + 4u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&attacker, &3_000, &family_wallet_id, &bills_id, &10, &4u64); + + assert_contract_error(result, OrchestratorError::PermissionDenied); + + let bills_client = MockBillPaymentsClient::new(&env, &bills_id); + assert_eq!(bills_client.payment_count(), 0); +} - #[test] - fn test_nonce_replay_insurance_payment_rejected() { - let (env, orchestrator_id, family_wallet_id, _, _, _, insurance_id, user) = setup(); - let client = OrchestratorClient::new(&env, &orchestrator_id); - let r1 = client.try_execute_insurance_payment( - &user, - &2000, - &family_wallet_id, - &insurance_id, - &1, - &77u64, - ); - assert!(r1.is_ok()); - let r2 = client.try_execute_insurance_payment( - &user, - &2000, - &family_wallet_id, - &insurance_id, - &1, - &77u64, - ); - assert_eq!( - r2.unwrap_err().unwrap(), - OrchestratorError::NonceAlreadyUsed - ); - } +#[test] +fn test_execute_bill_payment_rejects_nonce_replay() { + let ( + env, + orchestrator_id, + family_wallet_id, + _remittance_split_id, + _savings_id, + bills_id, + _insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 11, &user); + + let client = OrchestratorClient::new(&env, &orchestrator_id); + let first = client + .mock_auths(&[MockAuth { + address: &user, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + user.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 11u32, + 55u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&user, &3_000, &family_wallet_id, &bills_id, &11, &55u64); + assert_contract_ok(first); + + let replayed = client + .mock_auths(&[MockAuth { + address: &user, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + user.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 11u32, + 55u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&user, &3_000, &family_wallet_id, &bills_id, &11, &55u64); + + assert_contract_error(replayed, OrchestratorError::NonceAlreadyUsed); +} + +#[test] +fn test_execute_bill_payment_accepts_distinct_nonces_for_same_owner() { + let ( + env, + orchestrator_id, + family_wallet_id, + _remittance_split_id, + _savings_id, + bills_id, + _insurance_id, + _proxy_id, + user, + _attacker, + ) = setup_test_env(); + set_wallet_owner(&env, &family_wallet_id, &user); + set_bill_owner(&env, &bills_id, 12, &user); + + let client = OrchestratorClient::new(&env, &orchestrator_id); + let first = client + .mock_auths(&[MockAuth { + address: &user, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + user.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 12u32, + 100u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&user, &3_000, &family_wallet_id, &bills_id, &12, &100u64); + assert_contract_ok(first); + + let second = client + .mock_auths(&[MockAuth { + address: &user, + invoke: &MockAuthInvoke { + contract: &orchestrator_id, + fn_name: "execute_bill_payment", + args: ( + user.clone(), + 3_000i128, + family_wallet_id.clone(), + bills_id.clone(), + 12u32, + 101u64, + ) + .into_val(&env), + sub_invokes: &[], + }, + }]) + .try_execute_bill_payment(&user, &3_000, &family_wallet_id, &bills_id, &12, &101u64); + + assert_contract_ok(second); + + let bills_client = MockBillPaymentsClient::new(&env, &bills_id); + assert_eq!(bills_client.payment_count(), 2); } diff --git a/remittance_split/src/lib.rs b/remittance_split/src/lib.rs index c2d92f2e..b6932cf4 100644 --- a/remittance_split/src/lib.rs +++ b/remittance_split/src/lib.rs @@ -167,6 +167,21 @@ pub struct ExportSnapshot { pub exported_at: u64, } +#[contracttype] +#[derive(Clone)] +pub struct SplitAuthPayload { + pub domain_id: Symbol, + pub network_id: BytesN<32>, + pub contract_addr: Address, + pub owner_addr: Address, + pub nonce_val: u64, + pub usdc_contract: Address, + pub spending_percent: u32, + pub savings_percent: u32, + pub bills_percent: u32, + pub insurance_percent: u32, +} + /// Audit log entry for security and compliance. #[contracttype] #[derive(Clone)] @@ -1039,7 +1054,7 @@ impl RemittanceSplit { // 6. Timestamp sanity — reject payloads whose timestamps are in the future. let current_time = env.ledger().timestamp(); - if snapshot.config.timestamp > current_time || snapshot.exported_at > current_time { + if snapshot.config.timestamp > current_time { Self::append_audit(&env, symbol_short!("import"), &caller, false); return Err(RemittanceSplitError::FutureTimestamp); } @@ -1133,7 +1148,7 @@ impl RemittanceSplit { let expected = Self::compute_checksum( snapshot.schema_version, &snapshot.config, - snapshot.exported_at, + &snapshot.schedules, ); if snapshot.checksum != expected { return Err(RemittanceSplitError::ChecksumMismatch); @@ -1164,7 +1179,7 @@ impl RemittanceSplit { // 6. Timestamp sanity let current_time = env.ledger().timestamp(); - if snapshot.config.timestamp > current_time || snapshot.exported_at > current_time { + if snapshot.config.timestamp > current_time { return Err(RemittanceSplitError::FutureTimestamp); } diff --git a/remitwise-common/src/lib.rs b/remitwise-common/src/lib.rs index a0f18c37..2175cf98 100644 --- a/remitwise-common/src/lib.rs +++ b/remitwise-common/src/lib.rs @@ -209,4 +209,4 @@ pub const INSTANCE_BUMP_AMOUNT: u32 = 30 * DAY_IN_LEDGERS; // 30 days pub const INSTANCE_LIFETIME_THRESHOLD: u32 = 7 * DAY_IN_LEDGERS; // 7 days pub const ARCHIVE_BUMP_AMOUNT: u32 = 150 * DAY_IN_LEDGERS; // ~150 days -pub const ARCHIVE_LIFETIME_THRESHOLD: u32 = 1 * DAY_IN_LEDGERS; // 1 day +pub const ARCHIVE_LIFETIME_THRESHOLD: u32 = DAY_IN_LEDGERS; // 1 day diff --git a/scenarios/src/lib.rs b/scenarios/src/lib.rs index 9e28e78b..de7a2bc7 100644 --- a/scenarios/src/lib.rs +++ b/scenarios/src/lib.rs @@ -1,5 +1,5 @@ pub mod tests { - use soroban_sdk::testutils::{Ledger, LedgerInfo}; + use soroban_sdk::{testutils::{Ledger, LedgerInfo}, Env}; pub fn setup_env() -> Env { let env = Env::default();