From e2480a261225974c1e0d3634e259f05f0715ccd4 Mon Sep 17 00:00:00 2001 From: devmiracle Date: Sat, 27 Jun 2026 14:33:53 +0000 Subject: [PATCH] feat: add max_fee_bps slippage guard to deduct Closes #498 --- contracts/vault/src/lib.rs | 27 +++ contracts/vault/src/test.rs | 217 ++++++++++++++----- contracts/vault/src/test_balance_property.rs | 20 +- contracts/vault/src/test_idempotency.rs | 46 ++-- contracts/vault/src/test_reentrancy.rs | 10 +- docs/interfaces/vault.json | 8 +- tests/e2e_full_cycle.rs | 4 +- 7 files changed, 234 insertions(+), 98 deletions(-) diff --git a/contracts/vault/src/lib.rs b/contracts/vault/src/lib.rs index dfcfc4ab..6cc00c3b 100644 --- a/contracts/vault/src/lib.rs +++ b/contracts/vault/src/lib.rs @@ -111,6 +111,8 @@ pub enum VaultError { NewRevenuePoolSameAsCurrent = 33, /// No revenue pool transfer is pending (code 34). NoRevenuePoolTransferPending = 34, + /// Calculated fee in basis points exceeds the caller-supplied `max_fee_bps` limit (code 35). + Slippage = 35, } #[contracttype] @@ -727,6 +729,18 @@ impl CalloraVault { /// - `caller` must be the owner or `authorized_caller`. /// - Vault balance must cover `amount`. /// + /// # Per-Call Slippage Guard (`max_fee_bps`) + /// `max_fee_bps` limits the deducted amount expressed as a fraction of the current + /// vault balance in basis points (1 bps = 0.01%). + /// + /// `calculated_fee_bps = (amount * 10_000) / balance` + /// + /// If `calculated_fee_bps > max_fee_bps` the call reverts with + /// `VaultError::Slippage` **before** any state is mutated. + /// + /// Pass `u16::MAX` (65535) to disable the guard and preserve the existing + /// unrestricted behaviour — this is the default for backward compatibility. + /// /// # Idempotency /// When `request_id` is `Some(id)`, the contract checks whether `id` has /// already been processed. If so, `VaultError::DuplicateRequestId` is @@ -744,6 +758,7 @@ impl CalloraVault { caller: Address, amount: i128, request_id: Option, + max_fee_bps: u16, ) -> Result { Self::require_not_paused(env.clone())?; caller.require_auth(); @@ -763,6 +778,18 @@ impl CalloraVault { if meta.balance < amount { return Err(VaultError::InsufficientBalance); } + // Slippage guard: reject if the deducted amount exceeds max_fee_bps of the + // current balance. Calculated before any state mutation or external call. + // Uses u16::MAX as the sentinel for "no limit" (backward-compatible default). + if max_fee_bps < u16::MAX && meta.balance > 0 { + let calculated_fee_bps = amount + .checked_mul(10_000) + .ok_or(VaultError::Overflow)? + / meta.balance; + if calculated_fee_bps > max_fee_bps as i128 { + return Err(VaultError::Slippage); + } + } let settlement = Self::require_settlement(&env)?; let ut: Address = env .storage() diff --git a/contracts/vault/src/test.rs b/contracts/vault/src/test.rs index 431b5495..120c1e78 100644 --- a/contracts/vault/src/test.rs +++ b/contracts/vault/src/test.rs @@ -776,7 +776,7 @@ fn set_authorized_caller_sets_and_emits_event() { assert_eq!(now, Some(new_caller.clone())); assert_eq!(nonce, 0u64); - let remaining = client.deduct(&new_caller, &50, &None); + let remaining = client.deduct(&new_caller, &50, &None, &u16::MAX); assert_eq!(remaining, 150); } @@ -793,7 +793,7 @@ fn deduct_reduces_balance() { let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); - let returned = client.deduct(&owner, &50, &None); + let returned = client.deduct(&owner, &50, &None, &u16::MAX); assert_eq!(returned, 250); assert_eq!(client.balance(), 250); } @@ -811,7 +811,7 @@ fn deduct_with_request_id() { let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); - let remaining = client.deduct(&owner, &100, &Some(Symbol::new(&env, "req123"))); + let remaining = client.deduct(&owner, &100, &Some(Symbol::new(&env, "req123")), &u16::MAX); assert_eq!(remaining, 900); } @@ -826,7 +826,7 @@ fn deduct_insufficient_balance_fails() { fund_vault(&usdc_admin, &vault_address, 10); client.init(&owner, &usdc, &Some(10), &None, &None, &None, &None); - let result = client.try_deduct(&owner, &100, &None); + let result = client.try_deduct(&owner, &100, &None, &u16::MAX); assert!(result.is_err(), "expected error for insufficient balance"); } @@ -843,7 +843,7 @@ fn deduct_exact_balance_succeeds() { let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); - let remaining = client.deduct(&owner, &75, &None); + let remaining = client.deduct(&owner, &75, &None, &u16::MAX); assert_eq!(remaining, 0); assert_eq!(client.balance(), 0); } @@ -862,7 +862,7 @@ fn deduct_event_contains_request_id() { client.set_settlement(&owner, &settlement); let request_id = Symbol::new(&env, "api_call_42"); - client.deduct(&owner, &150, &Some(request_id.clone())); + client.deduct(&owner, &150, &Some(request_id.clone()), &u16::MAX); let events = env.events().all(); let ev = events.last().expect("expected deduct event"); @@ -891,7 +891,7 @@ fn deduct_zero_amount_fails() { env.mock_all_auths(); fund_vault(&usdc_admin, &client.address, 100); client.init(&owner, &usdc, &Some(100), &None, &None, &None, &None); - client.deduct(&owner, &0, &None); + client.deduct(&owner, &0, &None, &u16::MAX); } #[test] @@ -905,7 +905,7 @@ fn deduct_exceeding_max_fails() { fund_vault(&usdc_admin, &client.address, 1000); // Set max_deduct to 500 client.init(&owner, &usdc, &Some(1000), &None, &None, &None, &Some(500)); - client.deduct(&owner, &501, &None); + client.deduct(&owner, &501, &None, &u16::MAX); } #[test] @@ -928,7 +928,7 @@ fn deduct_authorized_caller_succeeds() { ); let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); - let remaining = client.deduct(&authorized, &100, &None); + let remaining = client.deduct(&authorized, &100, &None, &u16::MAX); assert_eq!(remaining, 900); } @@ -943,7 +943,7 @@ fn deduct_paused_fails() { fund_vault(&usdc_admin, &client.address, 1000); client.init(&owner, &usdc, &Some(1000), &None, &None, &None, &None); client.pause(&owner); - client.deduct(&owner, &100, &None); + client.deduct(&owner, &100, &None, &u16::MAX); } #[test] @@ -958,7 +958,7 @@ fn deduct_event_no_request_id_uses_empty_symbol() { client.init(&owner, &usdc, &Some(300), &None, &None, &None, &None); let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); - client.deduct(&owner, &100, &None); + client.deduct(&owner, &100, &None, &u16::MAX); let events = env.events().all(); let ev = events.last().expect("expected deduct event"); @@ -987,7 +987,7 @@ fn deduct_zero_panics() { env.mock_all_auths(); fund_vault(&usdc_admin, &vault_address, 500); client.init(&owner, &usdc, &Some(500), &None, &None, &None, &None); - client.deduct(&owner, &0, &None); + client.deduct(&owner, &0, &None, &u16::MAX); } #[test] @@ -1001,7 +1001,7 @@ fn deduct_negative_panics() { env.mock_all_auths(); fund_vault(&usdc_admin, &vault_address, 100); client.init(&owner, &usdc, &Some(100), &None, &None, &None, &None); - client.deduct(&owner, &-50, &None); + client.deduct(&owner, &-50, &None, &u16::MAX); } #[test] @@ -1015,7 +1015,7 @@ fn deduct_exceeds_balance_panics() { env.mock_all_auths(); fund_vault(&usdc_admin, &vault_address, 50); client.init(&owner, &usdc, &Some(50), &None, &None, &None, &None); - client.deduct(&owner, &100, &None); + client.deduct(&owner, &100, &None, &u16::MAX); } #[test] @@ -1029,7 +1029,7 @@ fn balance_unchanged_after_failed_deduct() { fund_vault(&usdc_admin, &vault_address, 100); client.init(&owner, &usdc, &Some(100), &None, &None, &None, &None); - let _ = client.try_deduct(&owner, &200, &None); + let _ = client.try_deduct(&owner, &200, &None, &u16::MAX); assert_eq!(client.balance(), 100); } @@ -1267,7 +1267,7 @@ fn get_revenue_pool_consistent_after_deduct_operations() { assert_eq!(before, Some(revenue_pool.clone())); // Perform deduct operation (routes to settlement, not revenue_pool) - client.deduct(&caller, &200, &None); + client.deduct(&caller, &200, &None, &u16::MAX); // Query revenue pool after deduct - should be unchanged let after = client.get_revenue_pool(); @@ -2132,7 +2132,7 @@ fn vault_full_lifecycle() { assert_eq!(client.balance(), 525); // Single deduct - let after_deduct = client.deduct(&owner, &25, &Some(Symbol::new(&env, "r4"))); + let after_deduct = client.deduct(&owner, &25, &Some(Symbol::new(&env, "r4")), &u16::MAX); assert_eq!(after_deduct, 500); // Admin change @@ -2206,7 +2206,7 @@ fn deduct_with_only_revenue_pool_panics() { &None, ); - client.deduct(&caller, &300, &None); + client.deduct(&caller, &300, &None, &u16::MAX); } #[test] @@ -2231,7 +2231,7 @@ fn deduct_with_settlement_transfers_usdc() { ); client.set_settlement(&owner, &settlement); - client.deduct(&caller, &250, &None); + client.deduct(&caller, &250, &None, &u16::MAX); assert_eq!(client.balance(), 550); assert_eq!(usdc_client.balance(&settlement), 250); @@ -2476,7 +2476,7 @@ fn get_revenue_pool_consistent_after_deduct_operations() { assert_eq!(before, Some(revenue_pool.clone())); // Perform deduct operation (routes to settlement, not revenue_pool) - client.deduct(&caller, &200, &None); + client.deduct(&caller, &200, &None, &u16::MAX); // Query revenue pool after deduct - should be unchanged let after = client.get_revenue_pool(); @@ -2626,7 +2626,7 @@ fn deduct_routes_to_settlement_when_both_configured() { ); client.set_settlement(&owner, &settlement); - client.deduct(&caller, &400, &None); + client.deduct(&caller, &400, &None, &u16::MAX); // settlement gets the funds, revenue_pool gets nothing assert_eq!(usdc_client.balance(&settlement), 400); @@ -2799,7 +2799,7 @@ fn get_settlement_consistent_after_deduct_operations() { assert_eq!(before, settlement); // Perform deduct operation - client.deduct(&caller, &200, &None); + client.deduct(&caller, &200, &None, &u16::MAX); // Query settlement after deduct - should be unchanged let after = client.get_settlement(); @@ -3122,7 +3122,7 @@ fn test_deduct_with_settlement_success() { ); client.set_settlement(&owner, &settlement); - client.deduct(&owner, &300, &None); + client.deduct(&owner, &300, &None, &u16::MAX); assert_eq!(client.balance(), 700); assert_eq!(usdc_client.balance(&settlement), 300); @@ -3184,7 +3184,7 @@ fn deduct_to_zero_succeeds() { let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); - assert_eq!(client.deduct(&owner, &500, &None), 0); + assert_eq!(client.deduct(&owner, &500, &None, &u16::MAX), 0); } #[test] @@ -3416,7 +3416,7 @@ fn deduct_while_paused_fails() { let settlement = create_settlement(&env, &owner, &vault_address); client.set_settlement(&owner, &settlement); client.pause(&owner); - client.deduct(&owner, &100, &None); + client.deduct(&owner, &100, &None, &u16::MAX); } #[test] @@ -3455,7 +3455,7 @@ fn deduct_unauthorized_caller_fails() { // init with an authorized_caller so the None branch is not taken let auth = Address::generate(&env); client.init(&owner, &usdc, &Some(500), &Some(auth), &None, &None, &None); - client.deduct(&attacker, &100, &None); + client.deduct(&attacker, &100, &None, &u16::MAX); } #[test] @@ -3490,7 +3490,7 @@ fn deduct_exceeds_max_deduct_fails() { env.mock_all_auths(); fund_vault(&usdc_admin, &vault_address, 1000); client.init(&owner, &usdc, &Some(1000), &None, &None, &None, &Some(50)); - client.deduct(&owner, &100, &None); // 100 > max_deduct(50) + client.deduct(&owner, &100, &None, &u16::MAX); // 100 > max_deduct(50) } #[test] @@ -3644,7 +3644,7 @@ fn deduct_without_settlement_panics() { env.mock_all_auths(); fund_vault(&usdc_admin, &vault_address, 500); client.init(&owner, &usdc, &Some(500), &None, &None, &None, &None); - client.deduct(&owner, &200, &None); + client.deduct(&owner, &200, &None, &u16::MAX); } #[test] @@ -3658,7 +3658,7 @@ fn deduct_without_settlement_does_not_mutate_state() { fund_vault(&usdc_admin, &vault_address, 500); client.init(&owner, &usdc, &Some(500), &None, &None, &None, &None); - let result = client.try_deduct(&owner, &200, &None); + let result = client.try_deduct(&owner, &200, &None, &u16::MAX); assert!(result.is_err(), "expected panic for missing settlement"); assert_eq!(client.balance(), 500); assert_eq!(usdc_client.balance(&vault_address), 500); @@ -3925,13 +3925,13 @@ mod fuzz { let amount: i128 = rng.gen_range(1..=op_cap); if paused { // deduct must fail while paused - assert!(client.try_deduct(&caller, &amount, &None).is_err()); + assert!(client.try_deduct(&caller, &amount, &None, &u16::MAX).is_err()); } else if sim >= amount { sim -= amount; - client.deduct(&caller, &amount, &None); + client.deduct(&caller, &amount, &None, &u16::MAX); } else { // must fail — balance unchanged (insufficient) - assert!(client.try_deduct(&caller, &amount, &None).is_err()); + assert!(client.try_deduct(&caller, &amount, &None, &u16::MAX).is_err()); } } @@ -4209,11 +4209,11 @@ mod fuzz { let amount: i128 = rng.gen_range(1..=max_d); if sim >= amount { sim -= amount; - client.deduct(&caller, &amount, &None); + client.deduct(&caller, &amount, &None, &u16::MAX); } else { // Must be rejected; balance and sim are unchanged. assert!( - client.try_deduct(&caller, &amount, &None).is_err(), + client.try_deduct(&caller, &amount, &None, &u16::MAX).is_err(), "deduct exceeding balance must fail at step {step}" ); } @@ -4390,15 +4390,15 @@ mod fuzz { let amount: i128 = rng.gen_range(1..=max_d); if paused { assert!( - client.try_deduct(&caller, &amount, &None).is_err(), + client.try_deduct(&caller, &amount, &None, &u16::MAX).is_err(), "deduct must fail while paused at step {step}" ); } else if sim >= amount { sim -= amount; - client.deduct(&caller, &amount, &None); + client.deduct(&caller, &amount, &None, &u16::MAX); } else { assert!( - client.try_deduct(&caller, &amount, &None).is_err(), + client.try_deduct(&caller, &amount, &None, &u16::MAX).is_err(), "insufficient deduct must fail at step {step}" ); } @@ -4552,11 +4552,11 @@ mod fuzz { client.deposit(&owner, &1); } else if sim >= 1 { sim -= 1; - client.deduct(&caller, &1, &None); + client.deduct(&caller, &1, &None, &u16::MAX); } else { // Balance exhausted: deduct must fail. assert!( - client.try_deduct(&caller, &1, &None).is_err(), + client.try_deduct(&caller, &1, &None, &u16::MAX).is_err(), "deduct must fail when balance=0 at step {step}" ); } @@ -4618,10 +4618,10 @@ mod fuzz { let amount: i128 = rng.gen_range(1..=max_d); if sim >= amount { sim -= amount; - client.deduct(&owner, &amount, &None); + client.deduct(&owner, &amount, &None, &u16::MAX); } else { assert!( - client.try_deduct(&owner, &amount, &None).is_err(), + client.try_deduct(&owner, &amount, &None, &u16::MAX).is_err(), "owner deduct must fail when balance insufficient at step {step}" ); } @@ -4630,10 +4630,10 @@ mod fuzz { let amount: i128 = rng.gen_range(1..=max_d); if sim >= amount { sim -= amount; - client.deduct(&caller_b, &amount, &None); + client.deduct(&caller_b, &amount, &None, &u16::MAX); } else { assert!( - client.try_deduct(&caller_b, &amount, &None).is_err(), + client.try_deduct(&caller_b, &amount, &None, &u16::MAX).is_err(), "caller_b deduct must fail when balance insufficient at step {step}" ); } @@ -4804,7 +4804,7 @@ fn deduct_equal_to_max_deduct_succeeds() { usdc_client.approve(&owner, &vault_address, &200, &1000); client.deposit(&owner, &200); // deduct exactly equal to max_deduct — must succeed - let balance = client.deduct(&owner, &100, &None); + let balance = client.deduct(&owner, &100, &None, &u16::MAX); assert_eq!(balance, 600); } @@ -4822,7 +4822,7 @@ fn deduct_above_max_deduct_panics() { usdc_client.approve(&owner, &vault_address, &200, &1000); client.deposit(&owner, &200); // deduct 101 > max_deduct 100 — must panic - client.deduct(&owner, &101, &None); + client.deduct(&owner, &101, &None, &u16::MAX); } #[test] @@ -4841,7 +4841,7 @@ fn deduct_default_cap_is_i128_max() { usdc_client.approve(&owner, &vault_address, &1_000_000, &1000); client.deposit(&owner, &1_000_000); // large deduct well below i128::MAX should succeed - let balance = client.deduct(&owner, &999_999, &None); + let balance = client.deduct(&owner, &999_999, &None, &u16::MAX); assert_eq!(balance, 1); } @@ -5134,7 +5134,7 @@ fn instance_ttl_extended_on_deduct_and_batch_deduct() { client.deposit(&owner, &500); // deduct — bumps TTL - client.deduct(&owner, &100, &None); + client.deduct(&owner, &100, &None, &u16::MAX); let seq = env.ledger().sequence(); env.ledger() .set_sequence_number(seq + INSTANCE_BUMP_THRESHOLD - 1); @@ -5243,7 +5243,7 @@ mod malicious_token { let vault_client = CalloraVaultClient::new(&env, &vault_addr); // 😈 ATTACK: Call back into the vault - vault_client.deduct(&caller, &attack_amount, &Some(Symbol::new(&env, "reentry"))); + vault_client.deduct(&caller, &attack_amount, &Some(Symbol::new(&env, "reentry")), &u16::MAX); } } @@ -5317,7 +5317,7 @@ fn test_reentry_protection_single_deduct() { // Call 2 (Re-entrant): deduct(600) -> sees balance 500 -> PANIC "insufficient balance". malicious_client.set_attack_config(&vault_address, &owner, &600, &1); - let result = vault_client.try_deduct(&owner, &500, &None); + let result = vault_client.try_deduct(&owner, &500, &None, &u16::MAX); // Acceptable Outcome A: Re-entrant call fails due to state guard (insufficient balance). assert!( @@ -5439,7 +5439,7 @@ fn test_reentry_success_preserves_accounting() { // Call 1 resumes. malicious_client.set_attack_config(&vault_address, &owner, &100, &1); - vault_client.deduct(&owner, &200, &None); + vault_client.deduct(&owner, &200, &None, &u16::MAX); // Final balance must be exactly 700. assert_eq!( @@ -5479,7 +5479,7 @@ fn test_nested_reentry_protection() { // Total should be: 100 (original) + 3 * 100 (re-entries) = 400. token_client.set_attack_config(&vault_address, &owner, &100, &3); - vault_client.deduct(&owner, &100, &None); + vault_client.deduct(&owner, &100, &None, &u16::MAX); assert_eq!(vault_client.balance(), 600); } @@ -5523,7 +5523,7 @@ fn test_reentry_exact_balance_exhaustion() { // re-entry deduct(500) -> balance 0. (Success) malicious_client.set_attack_config(&vault_address, &owner, &500, &1); - vault_client.deduct(&owner, &500, &None); + vault_client.deduct(&owner, &500, &None, &u16::MAX); assert_eq!(vault_client.balance(), 0); // Try again with over-exhaustion @@ -5531,7 +5531,7 @@ fn test_reentry_exact_balance_exhaustion() { // deduct(600) -> balance 400. // re-entry deduct(401) -> Fail. malicious_client.set_attack_config(&vault_address, &owner, &401, &1); - let result = vault_client.try_deduct(&owner, &600, &None); + let result = vault_client.try_deduct(&owner, &600, &None, &u16::MAX); assert!(result.is_err()); assert_eq!(vault_client.balance(), 1000); } @@ -5571,7 +5571,7 @@ fn test_reentry_near_zero_balance() { // Call 2 (Re-entrant): deduct(1) -> sees balance 0 -> PANIC "insufficient balance" malicious_client.set_attack_config(&vault_address, &owner, &1, &1); - let result = vault_client.try_deduct(&owner, &1, &None); + let result = vault_client.try_deduct(&owner, &1, &None, &u16::MAX); // Must fail due to insufficient balance in re-entrant call assert!(result.is_err()); @@ -5737,7 +5737,7 @@ fn test_reentry_repeated_attempts() { // This tests that the vault's balance validation prevents over-deduction malicious_client.set_attack_config(&vault_address, &owner, &100, &5); - vault_client.deduct(&owner, &100, &None); + vault_client.deduct(&owner, &100, &None, &u16::MAX); // With 5 re-entries of 100 each, plus original 100, total should be 600 // So final balance should be 1000 - 600 = 400 @@ -5971,7 +5971,7 @@ fn budget_measure_single_deduct() { let (owner, client) = setup_vault_for_deduct(&env, 100_000_000); let before = BudgetSnapshot::capture(&env); - client.deduct(&owner, &1_000_000, &None); + client.deduct(&owner, &1_000_000, &None, &u16::MAX); let after = BudgetSnapshot::capture(&env); let delta = after.delta(&before); @@ -6114,3 +6114,108 @@ fn budget_measure_all() { std::println!("\n=== END VAULT BUDGET MEASUREMENTS ===\n"); } + +// --------------------------------------------------------------------------- +// max_fee_bps slippage guard tests (issue #498) +// --------------------------------------------------------------------------- + +/// Helper: vault with `balance` and settlement configured. +fn setup_slippage_vault(env: &Env, balance: i128) -> (Address, CalloraVaultClient) { + setup_vault_for_deduct(env, balance) +} + +/// Deducting 50 bps (amount = 5, balance = 1000) with limit = 50 bps → succeeds. +#[test] +fn slippage_fee_below_limit_succeeds() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 1000); + env.mock_all_auths(); + // 5 / 1000 * 10_000 = 50 bps; limit = 50 → should succeed + let remaining = client.deduct(&owner, &5, &None, &50); + assert_eq!(remaining, 995); +} + +/// Deducting exactly at the limit (fee_bps == max_fee_bps) → succeeds. +#[test] +fn slippage_fee_equal_to_limit_succeeds() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 1000); + env.mock_all_auths(); + // 10 / 1000 * 10_000 = 100 bps; limit = 100 → exactly equal, should succeed + let remaining = client.deduct(&owner, &10, &None, &100); + assert_eq!(remaining, 990); +} + +/// Deducting above the limit → returns Slippage error. +#[test] +fn slippage_fee_above_limit_returns_slippage_error() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 1000); + env.mock_all_auths(); + // 11 / 1000 * 10_000 = 110 bps; limit = 100 → exceeds, should fail + let result = client.try_deduct(&owner, &11, &None, &100); + assert_eq!( + result, + Err(Ok(VaultError::Slippage)), + "expected Slippage error" + ); +} + +/// Passing u16::MAX behaves like the old unrestricted deduct. +#[test] +fn slippage_max_u16_is_unrestricted() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 1000); + env.mock_all_auths(); + // Deduct 99% of balance — would fail any real limit, but u16::MAX = no limit + let remaining = client.deduct(&owner, &999, &None, &u16::MAX); + assert_eq!(remaining, 1); +} + +/// Boundary: max_fee_bps = 0 → any deduction of positive amount reverts. +#[test] +fn slippage_zero_limit_always_fails() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 1000); + env.mock_all_auths(); + // Even 1 stroop / 1000 = 10 bps > 0 → Slippage + let result = client.try_deduct(&owner, &1, &None, &0); + assert_eq!(result, Err(Ok(VaultError::Slippage))); +} + +/// Boundary: max_fee_bps = 1 → deduction at 1 bps passes; 2 bps fails. +#[test] +fn slippage_one_bps_limit() { + let env = Env::default(); + // balance = 100_000; 1 bps = 10 units, 2 bps = 20 units + let (owner, client) = setup_slippage_vault(&env, 100_000); + env.mock_all_auths(); + // 10 / 100_000 * 10_000 = 1 bps → equal to limit, succeeds + let remaining = client.deduct(&owner, &10, &None, &1); + assert_eq!(remaining, 99_990); + // 20 / 99_990 * 10_000 = 2000_000/99990 = 2 bps → exceeds limit of 1 + let result = client.try_deduct(&owner, &20, &None, &1); + assert_eq!(result, Err(Ok(VaultError::Slippage))); +} + +/// Slippage check fires before any state mutation (balance unchanged on failure). +#[test] +fn slippage_check_before_state_mutation() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 1000); + env.mock_all_auths(); + let balance_before = client.balance(); + // This should fail with Slippage + let _ = client.try_deduct(&owner, &500, &None, &10); + assert_eq!(client.balance(), balance_before, "balance must be unchanged after slippage revert"); +} + +/// Existing deductions (u16::MAX) continue to work — no regression. +#[test] +fn slippage_no_regression_existing_deductions() { + let env = Env::default(); + let (owner, client) = setup_slippage_vault(&env, 500); + env.mock_all_auths(); + assert_eq!(client.deduct(&owner, &200, &None, &u16::MAX), 300); + assert_eq!(client.deduct(&owner, &300, &None, &u16::MAX), 0); +} \ No newline at end of file diff --git a/contracts/vault/src/test_balance_property.rs b/contracts/vault/src/test_balance_property.rs index 32f9bd1b..dc4e543b 100644 --- a/contracts/vault/src/test_balance_property.rs +++ b/contracts/vault/src/test_balance_property.rs @@ -304,7 +304,7 @@ fn run_property_trace(seed: u64) { None }; if paused { - let result = client.try_deduct(&owner, &amount, &rid); + let result = client.try_deduct(&owner, &amount, &rid, &u16::MAX); trace.push( step, "deduct (paused, expect fail)", @@ -312,7 +312,7 @@ fn run_property_trace(seed: u64) { ); assert!(result.is_err()); } else if balance_before >= amount { - client.deduct(&owner, &amount, &rid); + client.deduct(&owner, &amount, &rid, &u16::MAX); if let Some(ref id) = rid { used_request_ids.push(id.clone()); } @@ -322,7 +322,7 @@ fn run_property_trace(seed: u64) { std::format!("amount={amount} rid={with_id:?}"), ); } else { - let result = client.try_deduct(&owner, &amount, &rid); + let result = client.try_deduct(&owner, &amount, &rid, &u16::MAX); trace.push( step, "deduct (insufficient, expect fail)", @@ -464,8 +464,8 @@ fn run_property_trace(seed: u64) { if !paused && balance_before >= amount { let rid = make_request_id(&env, rid_counter); rid_counter += 1; - client.deduct(&owner, &amount, &Some(rid.clone())); - let retry = client.try_deduct(&owner, &amount, &Some(rid.clone())); + client.deduct(&owner, &amount, &Some(rid.clone()), &u16::MAX); + let retry = client.try_deduct(&owner, &amount, &Some(rid.clone()), &u16::MAX); trace.push( step, "request_id_reuse", @@ -480,7 +480,7 @@ fn run_property_trace(seed: u64) { let idx = rng.gen_range_usize(0, used_request_ids.len()); let rid = used_request_ids[idx].clone(); let amount = rng.gen_range_i128(1, AMOUNT_CAP); - let retry = client.try_deduct(&owner, &amount, &Some(rid.clone())); + let retry = client.try_deduct(&owner, &amount, &Some(rid.clone()), &u16::MAX); trace.push( step, "request_id_reuse", @@ -546,7 +546,7 @@ fn test_balance_property_pause_mid_sequence() { client.pause(&owner); assert!(client.try_deposit(&owner, &50).is_err()); - assert!(client.try_deduct(&owner, &10, &None).is_err()); + assert!(client.try_deduct(&owner, &10, &None, &u16::MAX).is_err()); assert_balance_in_sync(&client, &usdc_client, &vault_addr, &Trace::new(42), 2); // Withdraw is allowed while paused. @@ -554,7 +554,7 @@ fn test_balance_property_pause_mid_sequence() { assert_balance_in_sync(&client, &usdc_client, &vault_addr, &Trace::new(42), 3); client.unpause(&owner); - client.deduct(&owner, &25, &None); + client.deduct(&owner, &25, &None, &u16::MAX); assert_balance_in_sync(&client, &usdc_client, &vault_addr, &Trace::new(42), 4); } @@ -620,10 +620,10 @@ fn test_balance_property_request_id_reuse() { client.set_settlement(&owner, &settlement); let rid = Symbol::new(&env, "reuse_test_id"); - client.deduct(&owner, &100, &Some(rid.clone())); + client.deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert_balance_in_sync(&client, &usdc_client, &vault_addr, &Trace::new(13), 1); - let retry = client.try_deduct(&owner, &50, &Some(rid.clone())); + let retry = client.try_deduct(&owner, &50, &Some(rid.clone()), &u16::MAX); assert!(retry.is_err()); assert_balance_in_sync(&client, &usdc_client, &vault_addr, &Trace::new(13), 2); assert_eq!(client.balance(), INITIAL_BALANCE - 100); diff --git a/contracts/vault/src/test_idempotency.rs b/contracts/vault/src/test_idempotency.rs index 0fef5ed9..9f091526 100644 --- a/contracts/vault/src/test_idempotency.rs +++ b/contracts/vault/src/test_idempotency.rs @@ -150,11 +150,11 @@ fn deduct_duplicate_request_id_rejected() { let rid = Symbol::new(&env, "req_001"); // First call — must succeed. - let remaining = client.deduct(&owner, &100, &Some(rid.clone())); + let remaining = client.deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert_eq!(remaining, 900); // Second call with same request_id — must be rejected. - let result = client.try_deduct(&owner, &100, &Some(rid.clone())); + let result = client.try_deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert!(result.is_err(), "duplicate request_id must be rejected"); // Balance must be unchanged after the rejected retry. @@ -174,10 +174,10 @@ fn deduct_distinct_request_ids_both_succeed() { let rid_a = Symbol::new(&env, "req_a"); let rid_b = Symbol::new(&env, "req_b"); - let after_a = client.deduct(&owner, &100, &Some(rid_a.clone())); + let after_a = client.deduct(&owner, &100, &Some(rid_a.clone()), &u16::MAX); assert_eq!(after_a, 900); - let after_b = client.deduct(&owner, &200, &Some(rid_b.clone())); + let after_b = client.deduct(&owner, &200, &Some(rid_b.clone()), &u16::MAX); assert_eq!(after_b, 700); assert_eq!(client.balance(), 700); @@ -190,9 +190,9 @@ fn deduct_none_request_id_not_deduplicated() { let (_, client, _, owner) = setup_vault(&env, 1_000); // Three calls with None — all must succeed. - assert_eq!(client.deduct(&owner, &100, &None), 900); - assert_eq!(client.deduct(&owner, &100, &None), 800); - assert_eq!(client.deduct(&owner, &100, &None), 700); + assert_eq!(client.deduct(&owner, &100, &None, &u16::MAX), 900); + assert_eq!(client.deduct(&owner, &100, &None, &u16::MAX), 800); + assert_eq!(client.deduct(&owner, &100, &None, &u16::MAX), 700); assert_eq!(client.balance(), 700); } @@ -205,7 +205,7 @@ fn deduct_failed_due_to_insufficient_balance_does_not_mark_id() { let rid = Symbol::new(&env, "req_fail"); // Attempt to deduct more than the balance — must fail. - let result = client.try_deduct(&owner, &100, &Some(rid.clone())); + let result = client.try_deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert!(result.is_err(), "expected insufficient balance error"); // The id must NOT be marked — a retry with sufficient balance should succeed. @@ -226,7 +226,7 @@ fn deduct_failed_due_to_paused_does_not_mark_id() { let rid = Symbol::new(&env, "req_paused"); client.pause(&owner); - let result = client.try_deduct(&owner, &100, &Some(rid.clone())); + let result = client.try_deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert!(result.is_err(), "expected paused error"); assert!( @@ -256,7 +256,7 @@ fn is_request_processed_true_after_successful_deduct() { let (_, client, _, owner) = setup_vault(&env, 500); let rid = Symbol::new(&env, "seen"); - client.deduct(&owner, &50, &Some(rid.clone())); + client.deduct(&owner, &50, &Some(rid.clone()), &u16::MAX); assert!( client.is_request_processed(&rid), @@ -273,7 +273,7 @@ fn is_request_processed_false_for_different_id() { let rid_a = Symbol::new(&env, "id_a"); let rid_b = Symbol::new(&env, "id_b"); - client.deduct(&owner, &50, &Some(rid_a.clone())); + client.deduct(&owner, &50, &Some(rid_a.clone()), &u16::MAX); assert!(client.is_request_processed(&rid_a)); assert!(!client.is_request_processed(&rid_b)); @@ -292,7 +292,7 @@ fn batch_deduct_duplicate_request_id_rejected_atomically() { let rid = Symbol::new(&env, "batch_dup"); // First single deduct marks the id. - client.deduct(&owner, &100, &Some(rid.clone())); + client.deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert_eq!(client.balance(), 900); // Batch that reuses the same id — must be rejected atomically. @@ -478,10 +478,10 @@ fn deduct_retry_with_different_amount_still_rejected() { let rid = Symbol::new(&env, "retry_amt"); - client.deduct(&owner, &100, &Some(rid.clone())); + client.deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); // Retry with a different amount — still rejected. - let result = client.try_deduct(&owner, &50, &Some(rid.clone())); + let result = client.try_deduct(&owner, &50, &Some(rid.clone()), &u16::MAX); assert!( result.is_err(), "retry with different amount must be rejected" @@ -521,11 +521,11 @@ fn batch_deduct_mixed_ids_marks_only_some_ids() { assert!(client.is_request_processed(&rid_z)); // Retrying either Some id must fail. - assert!(client.try_deduct(&owner, &10, &Some(rid_x)).is_err()); - assert!(client.try_deduct(&owner, &10, &Some(rid_z)).is_err()); + assert!(client.try_deduct(&owner, &10, &Some(rid_x)).is_err(), &u16::MAX); + assert!(client.try_deduct(&owner, &10, &Some(rid_z)).is_err(), &u16::MAX); // None deducts still go through. - assert_eq!(client.deduct(&owner, &10, &None), 765); + assert_eq!(client.deduct(&owner, &10, &None, &u16::MAX), 765); } #[test] @@ -536,7 +536,7 @@ fn replay_across_long_window_rejected() { let rid = Symbol::new(&env, "req_long_win"); // First call succeeds - client.deduct(&owner, &100, &Some(rid.clone())); + client.deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); // Fast-forward ledger 6 months (approx 6 * 30 days) let new_timestamp = env.ledger().timestamp() + 180 * 24 * 60 * 60; @@ -552,7 +552,7 @@ fn replay_across_long_window_rejected() { }); // Retry should still be rejected because it's persistent and hasn't been explicitly pruned. - let res = client.try_deduct(&owner, &100, &Some(rid.clone())); + let res = client.try_deduct(&owner, &100, &Some(rid.clone()), &u16::MAX); assert!(res.is_err(), "should still reject after multi-month window"); } @@ -564,8 +564,8 @@ fn gc_entrypoint_prunes_and_emits_event() { let rid1 = Symbol::new(&env, "req_gc_1"); let rid2 = Symbol::new(&env, "req_gc_2"); - client.deduct(&owner, &100, &Some(rid1.clone())); - client.deduct(&owner, &100, &Some(rid2.clone())); + client.deduct(&owner, &100, &Some(rid1.clone()), &u16::MAX); + client.deduct(&owner, &100, &Some(rid2.clone()), &u16::MAX); let mut ids_to_prune = soroban_sdk::Vec::new(&env); ids_to_prune.push_back(rid1.clone()); @@ -588,7 +588,7 @@ fn gc_entrypoint_prunes_and_emits_event() { assert!(has_event, "Should emit request_id_pruned event"); // Should now be able to replay rid1 - client.deduct(&owner, &100, &Some(rid1)); + client.deduct(&owner, &100, &Some(rid1), &u16::MAX); } #[test] @@ -611,7 +611,7 @@ fn gc_allowed_during_pause() { let (_, client, _, owner) = setup_vault(&env, 1_000); let rid1 = Symbol::new(&env, "req_gc_pause"); - client.deduct(&owner, &100, &Some(rid1.clone())); + client.deduct(&owner, &100, &Some(rid1.clone()), &u16::MAX); client.pause(&owner); assert!(client.is_paused()); diff --git a/contracts/vault/src/test_reentrancy.rs b/contracts/vault/src/test_reentrancy.rs index f043ad57..b0a54aaa 100644 --- a/contracts/vault/src/test_reentrancy.rs +++ b/contracts/vault/src/test_reentrancy.rs @@ -41,7 +41,7 @@ impl MaliciousToken { let client = CalloraVaultClient::new(&env, &vault); // Attempt re-entry into deduct - let _ = client.try_deduct(&caller, &1, &Some(Symbol::new(&env, "reentry_token"))); + let _ = client.try_deduct(&caller, &1, &Some(Symbol::new(&env, "reentry_token")), &u16::MAX); } } } @@ -102,7 +102,7 @@ impl MaliciousSettlement { let client = CalloraVaultClient::new(&env, &vault); // Attempt re-entry into deduct - let _ = client.try_deduct(&caller, &1, &Some(Symbol::new(&env, "reentry_settle"))); + let _ = client.try_deduct(&caller, &1, &Some(Symbol::new(&env, "reentry_settle")), &u16::MAX); } } } @@ -154,7 +154,7 @@ fn test_reentrancy_via_token_transfer_is_blocked_by_auth() { assert_eq!(initial_balance, 1000); // Trigger deduct -> calls token.transfer -> calls vault.deduct (re-entry) - let result = vault_client.try_deduct(&owner, &100, &Some(Symbol::new(&env, "first_call"))); + let result = vault_client.try_deduct(&owner, &100, &Some(Symbol::new(&env, "first_call")), &u16::MAX); assert!(result.is_ok(), "First deduct should succeed"); assert_eq!( @@ -196,7 +196,7 @@ fn test_reentrancy_via_settlement_callback_is_blocked() { assert_eq!(initial_balance, 1000); // Trigger deduct -> calls settlement.receive_payment -> calls vault.deduct (re-entry) - let result = vault_client.try_deduct(&owner, &100, &Some(Symbol::new(&env, "first_call"))); + let result = vault_client.try_deduct(&owner, &100, &Some(Symbol::new(&env, "first_call")), &u16::MAX); assert!(result.is_ok(), "First deduct should succeed"); assert_eq!( @@ -297,7 +297,7 @@ fn test_reentrancy_by_authorized_attacker() { assert_eq!(initial_balance, 1000); // Attacker calls deduct -> token.transfer -> attacker calls vault.deduct (re-entry) - let result = vault_client.try_deduct(&attacker, &100, &Some(Symbol::new(&env, "first_call"))); + let result = vault_client.try_deduct(&attacker, &100, &Some(Symbol::new(&env, "first_call")), &u16::MAX); assert!(result.is_ok(), "First deduct should succeed"); assert_eq!( diff --git a/docs/interfaces/vault.json b/docs/interfaces/vault.json index ec3d6367..5ee6eb54 100644 --- a/docs/interfaces/vault.json +++ b/docs/interfaces/vault.json @@ -37,6 +37,7 @@ { "code": 27, "name": "MetadataTooLong", "description": "Metadata exceeds maximum length." }, { "code": 28, "name": "PriceParseError", "description": "Price parsing error or non‑positive price." }, { "code": 29, "name": "DuplicateRequestId", "description": "Duplicate request ID detected; this request_id has already been processed." } + { "code": 35, "name": "Slippage", "description": "Calculated fee bps (amount * 10_000 / balance) exceeds the caller-supplied max_fee_bps limit." } ] }, @@ -186,9 +187,10 @@ "description": "Deduct USDC for a single API call. Decreases the tracked balance and forwards funds to the configured settlement contract. Blocked when paused.", "access": "owner OR authorized_caller", "params": [ - { "name": "caller", "type": "Address", "optional": false, "description": "Must be the owner or the authorized_caller; must authorize." }, - { "name": "amount", "type": "i128", "optional": false, "description": "Amount to deduct; must be > 0 and <= max_deduct." }, - { "name": "request_id", "type": "Symbol | null", "optional": true, "description": "Optional tracking key emitted in the event. When provided it must be unique across successful deductions." } + { "name": "caller", "type": "Address", "optional": false, "description": "Must be the owner or the authorized_caller; must authorize." }, + { "name": "amount", "type": "i128", "optional": false, "description": "Amount to deduct; must be > 0 and <= max_deduct." }, + { "name": "request_id", "type": "Symbol | null", "optional": true, "description": "Optional tracking key emitted in the event. When provided it must be unique across successful deductions." }, + { "name": "max_fee_bps", "type": "u16", "optional": false, "description": "Per-call slippage guard. Reverts with VaultError::Slippage (35) if (amount * 10_000 / balance) > max_fee_bps. Pass u16::MAX (65535) for no limit (backward-compatible default)." } ], "returns": "i128 (new balance)", "panics": [ diff --git a/tests/e2e_full_cycle.rs b/tests/e2e_full_cycle.rs index 22e2c941..78eb2d53 100644 --- a/tests/e2e_full_cycle.rs +++ b/tests/e2e_full_cycle.rs @@ -163,6 +163,7 @@ fn e2e_full_cycle() { &h.backend, &single_deduct_amount, &Some(Symbol::new(&env, "req_single_1")), + &u16::MAX, ); let batch_items = vec![ @@ -203,6 +204,7 @@ fn e2e_full_cycle() { &h.backend, &1, &Some(Symbol::new(&env, "req_single_1")), + &u16::MAX, ); assert!(dup_result.is_err(), "duplicate request_id must be rejected"); assert_eq!(h.vault.balance(), deposit_amount - total_deducted); @@ -286,7 +288,7 @@ fn e2e_full_cycle() { let blocked_deposit = h.vault.try_deposit(&h.owner, &1_000); assert!(blocked_deposit.is_err(), "deposit must be blocked while paused"); - let blocked_deduct = h.vault.try_deduct(&h.backend, &1_000, &None); + let blocked_deduct = h.vault.try_deduct(&h.backend, &1_000, &None, &u16::MAX); assert!(blocked_deduct.is_err(), "deduct must be blocked while paused"); // Owner withdraw is explicitly allowed while paused (emergency recovery).