From 0df50cd08216ad96b69c4be8a404d168b6e5e599 Mon Sep 17 00:00:00 2001 From: Antonio Ventilii Date: Wed, 15 Jul 2026 09:40:34 +0200 Subject: [PATCH] fix(wrapper): require fee to cover forwarded cycles [stopgap] Prevent a caller from draining the wrapper's cycle balance by pairing a trivial fee with a large cycles_to_forward. Forwarding is now only allowed when the payment is cycle-denominated (AttachedCycles / CallerPaysIcrc2Cycles / PatronPaysIcrc2Cycles) and fee_amount >= cycles_to_forward, so the wrapper never forwards more cycles than the payment credits to its balance. This is a minimal stopgap; operator-controlled pricing via MethodConfig is the proper fix (see follow-up). --- src/wrapper/src/api/call.rs | 43 +++++++++++++++++++++-- src/wrapper/src/domain/errors.rs | 18 ++++++++++ src/wrapper/tests/it/bridge_tests.rs | 52 ++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/wrapper/src/api/call.rs b/src/wrapper/src/api/call.rs index 69fc481..c75d832 100644 --- a/src/wrapper/src/api/call.rs +++ b/src/wrapper/src/api/call.rs @@ -16,16 +16,53 @@ pub async fn bridge_call(args: BridgeCallArgs) -> Result, String> { return Err("Self-calls are not allowed through the bridge.".to_string()); } - // 1) Charge fee using the payment guard let p = args.payment.unwrap_or(PaymentType::AttachedCycles); + let cycles = args.cycles_to_forward.unwrap_or(0); + + // 1) Validate that the forwarded cycles are covered by the fee *before* + // charging anything. + // + // The forwarded cycles are attached to the outbound call from this + // canister's own balance. They are only replenished when the payment is + // cycle-denominated: `AttachedCycles` (via `msg_cycles_accept`) and the + // ICRC-2 *cycles* flows (via the cycles-ledger `withdraw_from(to: self)`) + // credit this canister's cycle balance; the ICRC-2 *token* flows credit a + // token account instead. Since `fee_amount` and `cycles_to_forward` are + // both caller-controlled, we must ensure the fee is paid in cycles and + // covers the amount forwarded -- otherwise a caller could pay a trivial + // fee and drain the wrapper's cycles. + if cycles > 0 { + if !is_cycle_payment(&p) { + return Err(BridgeError::ForwardRequiresCyclePayment.to_string()); + } + if args.fee_amount < cycles { + return Err(BridgeError::ForwardExceedsFee { + fee: args.fee_amount, + forward: cycles, + } + .to_string()); + } + } + + // 2) Charge fee using the payment guard PAYMENT_GUARD .deduct(p, args.fee_amount) .await .map_err(map_guard_err)?; - // 2) Forward the call to target - let cycles = args.cycles_to_forward.unwrap_or(0); + // 3) Forward the call to target forward_raw(args.target, &args.method, args.args, cycles) .await .map_err(|e| BridgeError::TargetRejected(e).to_string()) } + +/// Whether a payment type credits this canister's *cycle* balance (as opposed to +/// a token ledger account), and can therefore fund forwarded cycles. +fn is_cycle_payment(payment: &PaymentType) -> bool { + matches!( + payment, + PaymentType::AttachedCycles + | PaymentType::CallerPaysIcrc2Cycles + | PaymentType::PatronPaysIcrc2Cycles(_) + ) +} diff --git a/src/wrapper/src/domain/errors.rs b/src/wrapper/src/domain/errors.rs index b19355c..de335ee 100644 --- a/src/wrapper/src/domain/errors.rs +++ b/src/wrapper/src/domain/errors.rs @@ -11,6 +11,13 @@ pub enum BridgeError { TargetRejected(String), /// Fee deduction failed (insufficient cycles/allowance/etc.). GuardError(String), + /// Cycles were requested to be forwarded, but the chosen payment type is not + /// cycle-denominated, so the forwarded cycles would come out of the wrapper's + /// own balance rather than being funded by the payment. + ForwardRequiresCyclePayment, + /// The requested cycles to forward exceed the fee charged, so the wrapper + /// would be forwarding more cycles than it was paid. + ForwardExceedsFee { fee: u128, forward: u128 }, } impl fmt::Display for BridgeError { @@ -19,6 +26,17 @@ impl fmt::Display for BridgeError { BridgeError::Candid(e) => write!(f, "Candid error: {e}"), BridgeError::TargetRejected(e) => write!(f, "Target canister rejected call: {e}"), BridgeError::GuardError(e) => write!(f, "Payment guard error: {e}"), + BridgeError::ForwardRequiresCyclePayment => write!( + f, + "Forwarding cycles requires a cycle-denominated payment type \ + (AttachedCycles, CallerPaysIcrc2Cycles or PatronPaysIcrc2Cycles); \ + token payments do not credit the wrapper's cycle balance." + ), + BridgeError::ForwardExceedsFee { fee, forward } => write!( + f, + "Cycles to forward ({forward}) exceed the fee charged ({fee}). \ + The fee must cover the forwarded cycles." + ), } } } diff --git a/src/wrapper/tests/it/bridge_tests.rs b/src/wrapper/tests/it/bridge_tests.rs index e4d4ce3..cf7acbd 100644 --- a/src/wrapper/tests/it/bridge_tests.rs +++ b/src/wrapper/tests/it/bridge_tests.rs @@ -22,6 +22,58 @@ fn bridge_call_fails_if_target_is_self() { assert!(err.contains("Self-calls are not allowed through the bridge.")); } +#[test] +fn bridge_call_rejects_forward_exceeding_fee() { + // Regression test for the cycle-drain: a caller must not be able to forward + // more cycles than the fee it pays. Here the classic attack -- a zero fee + // with a large forward amount -- must be rejected before any forwarding. + let setup = TestSetup::default(); + let args = Call0Args { + target: setup.target.canister_id(), + method: "any_method".to_string(), + fee_amount: 0, + payment: Some(PaymentType::AttachedCycles), + cycles_to_forward: Some(1_000_000_000_000), + }; + + let result: Result, String>, String> = + setup.wrapper.update(setup.user, "call0", args); + let inner_result = result.expect("Failed to reach canister"); + let err = inner_result.expect_err("Should have rejected forward exceeding fee"); + assert!( + err.contains("exceed the fee charged"), + "unexpected error: {err}" + ); +} + +#[test] +fn bridge_call_rejects_forward_with_token_payment() { + // Token payments credit a token account, not the wrapper's cycle balance, so + // forwarding cycles against them would drain the wrapper. Even with a fee + // that nominally covers the forward, a token payment type must be rejected. + let setup = TestSetup::default(); + let args = Call0Args { + target: setup.target.canister_id(), + method: "any_method".to_string(), + fee_amount: 1_000_000_000_000, + payment: Some(PaymentType::CallerPaysIcrc2Tokens( + ic_papi_api::caller::CallerPaysIcrc2Tokens { + ledger: Principal::anonymous(), + }, + )), + cycles_to_forward: Some(1_000), + }; + + let result: Result, String>, String> = + setup.wrapper.update(setup.user, "call0", args); + let inner_result = result.expect("Failed to reach canister"); + let err = inner_result.expect_err("Should have rejected forward with token payment"); + assert!( + err.contains("requires a cycle-denominated payment type"), + "unexpected error: {err}" + ); +} + #[test] fn bridge_call_fails_if_insufficient_cycles() { let setup = TestSetup::default();