fix(wrapper): require fee to cover forwarded cycles [stopgap]#230
Closed
AntonioVentilii wants to merge 1 commit into
Closed
fix(wrapper): require fee to cover forwarded cycles [stopgap]#230AntonioVentilii wants to merge 1 commit into
AntonioVentilii wants to merge 1 commit into
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 0df50cd. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a cycle-drain vulnerability in the ic-papi-wrapper bridge by ensuring any cycles_to_forward are funded from the caller’s attached cycles (rather than the wrapper’s own balance), and adds a regression test to prevent reintroduction.
Changes:
- Accept and forward
cycles_to_forwardonly when the caller has attached sufficient cycles, otherwise return a newInsufficientForwardCycles { needed, available }error. - Add a regression integration test to ensure unfunded forward-cycle requests are rejected.
- Extend
BridgeErrorto represent and format the new failure mode.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wrapper/src/api/call.rs | Adds forwarded-cycle funding via msg_cycles_available/accept before making the outbound call. |
| src/wrapper/src/domain/errors.rs | Introduces InsufficientForwardCycles error variant and Display formatting. |
| src/wrapper/tests/it/bridge_tests.rs | Adds regression test to ensure unfunded forward-cycle attempts are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
37
to
54
| let cycles = args.cycles_to_forward.unwrap_or(0); | ||
| if cycles > 0 { | ||
| let available = msg_cycles_available(); | ||
| if available < cycles { | ||
| return Err(BridgeError::InsufficientForwardCycles { | ||
| needed: cycles, | ||
| available, | ||
| } | ||
| .to_string()); | ||
| } | ||
| let accepted = msg_cycles_accept(cycles); | ||
| debug_assert_eq!(accepted, cycles, "accepted fewer cycles than checked"); | ||
| } | ||
|
|
||
| // 3) Forward the call to target | ||
| forward_raw(args.target, &args.method, args.args, cycles) | ||
| .await | ||
| .map_err(|e| BridgeError::TargetRejected(e).to_string()) |
Comment on lines
+25
to
+29
| BridgeError::InsufficientForwardCycles { needed, available } => write!( | ||
| f, | ||
| "Insufficient cycles attached to forward: needed {needed}, attached {available}. \ | ||
| The caller must attach at least the cycles requested via `cycles_to_forward`." | ||
| ), |
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).
AntonioVentilii
force-pushed
the
av/cycle-forwarding-validation-4275f3
branch
from
July 15, 2026 07:40
2fd46c9 to
0df50cd
Compare
AntonioVentilii
changed the title
AntonioVentilii
marked this pull request as draft
July 15, 2026 07:40
Collaborator
Author
|
Closing in favor of #232. Both close the cycle-drain, but this one keeps pricing caller-supplied, which contradicts PAPI's scope (the API operator sets the price, not the caller). #232 makes pricing operator-controlled via the MethodConfig registry — the scope-correct fix, and what the unused MethodConfig scaffolding was intended for. Keeping the branch as a reference for the minimal-guard approach. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The vulnerability
bridge_callcharges the caller-controlledfee_amountand, independently, forwards the caller-controlledcycles_to_forwardto the target. The forwarded cycles come from the wrapper's own balance. Because the two values are decoupled and both caller-controlled, a caller can pay a trivial (or zero) fee while forwarding a large amount, draining the wrapper. Ingress callers can't even attach cycles, so they pay nothing.This PR: minimal stopgap (no new state)
Keeps the current caller-supplied
fee_amount/cycles_to_forwardAPI, but refuses to forward cycles the fee doesn't cover. Before charging, whencycles_to_forward > 0:AttachedCycles,CallerPaysIcrc2Cycles, orPatronPaysIcrc2Cycles. These credit the wrapper's cycle balance (msg_cycles_accept/ cycles-ledgerwithdraw_from(to: self)). Token payments (ckUSDC/ckBTC/…) credit a token account, not cycles, so forwarding against them is rejected (ForwardRequiresCyclePayment).fee_amount >= cycles_to_forward, elseForwardExceedsFee.Net effect: the wrapper never forwards more cycles than the payment credited to its balance — worst case break-even, never a loss. Validation runs before the fee is charged, so a rejected call isn't billed.
Trade-offs
fee_amount == cycles_to_forwardearns the operator nothing on the forwarded portion); cycle-forwarding under token payment is disabled (would need a price oracle). Proper margin/pricing is the companion PR's job.Tests
bridge_call_rejects_forward_exceeding_fee— zero fee + large forward is rejected (the drain).bridge_call_rejects_forward_with_token_payment— token payment + forward is rejected.