Skip to content

execute_subscription reads fee_bps fresh from global config every execution — admin fee changes retroactively alter live subscriptions without payer re-consent #46

Description

@abayomicornelius

Overview

Subscription has the identical structural gap to the one described in the companion PaymentRequest issue in this batch, but the consequence is more severe because a subscription is meant to execute repeatedly over its entire lifetime, not just once:

// stellar_send/src/subscription.rs:71-96
pub struct Subscription {
    pub payer: Address,
    pub recipient: Address,
    pub token: Address,
    pub amount: i128,
    pub interval_seconds: u64,
    pub next_execution_time: u64,
    pub active: bool,
    pub max_executions: Option<u32>,
    pub expiry_time: Option<u64>,
    pub executions_count: u32,
}

No fee_bps field. execute_subscription reads the live, global fee on every single execution:

// stellar_send/src/subscription.rs:227-236
let config = Self::load_config(&env)?;
let (fee_amount, net_amount) = Self::split_fee(sub.amount, config.fee_bps)?;

let token_client = token::Client::new(&env, &sub.token);
let spender = env.current_contract_address();

if fee_amount > 0 {
    token_client.transfer_from(&spender, &sub.payer, &config.fee_collector, &fee_amount);
}
token_client.transfer_from(&spender, &sub.payer, &sub.recipient, &net_amount);

A payer creates a subscription for amount = 10_000 per interval, expecting a 1% fee (net 9_900 to the recipient each time), and separately grants a SEP-41 approve allowance sized for that expectation over the subscription's anticipated lifetime (the module doc comment, stellar_send/src/subscription.rs:1-11, explicitly describes this pull-based, pre-approved-allowance design). If the admin raises fee_bps at any point during the subscription's life, every future execution from that point on silently starts deducting the new, higher fee — the payer is now paying more per cycle (or the recipient is netting less) than what they agreed to when they set up the subscription and sized their allowance, with zero re-consent, zero notification beyond a generic FeeUpdated event with no per-subscription linkage, and — because execute_subscription is specifically designed to be called by an untrusted keeper with no live signature from the payer at execution time (stellar_send/src/subscription.rs:6-10) — the payer isn't even present at the moment their subscription's economics change out from under them.

This compounds with the module's own already-documented "catch-up burst" behavior (stellar_send/src/subscription.rs:34-56): if a keeper is offline for a stretch during which the admin also raises the fee, the resulting catch-up burst of several executions all apply the new, higher fee to payments the payer originally scheduled expecting the old rate — potentially multiplying the unexpected fee impact across several missed intervals at once.

Requirements

  • Add a fee_bps: u32 field to Subscription, captured from the live global config at create_subscription time, and use that stored value (not a fresh load_config read) in execute_subscription's split_fee call, exactly mirroring the fix needed for PaymentRequest in the companion issue.
  • Decide and document the product intent explicitly: should a payer's subscription economics ever be allowed to change without their re-consent? If yes for some reason (e.g. a deliberate "variable fee" product feature), that needs to be an opt-in the payer explicitly accepts at creation time, not silent default behavior.
  • Expose the locked-in fee_bps via get_subscription so a payer/indexer can verify exactly what rate applies without cross-referencing historical FeeUpdated events against the subscription's creation timestamp.

Acceptance Criteria

  • Subscription stores the fee_bps in effect at creation time.
  • execute_subscription uses the subscription's own stored fee_bps for every execution, not a fresh read of the live global config.
  • test_subscription_fee_locked_at_creation_survives_later_fee_change (or equivalently named) added, proving a set_fee call between creation and any subsequent execution no longer changes what's deducted from an already-live subscription.
  • A test specifically covers the catch-up-burst compounding scenario: a fee change occurring during a period of missed executions doesn't retroactively apply to the backlog once caught up.

Additional Notes

Precise references: stellar_send/src/subscription.rs:71-96 (Subscription struct, no fee_bps field), :120-181 (create_subscription, never reads or stores config.fee_bps), :227-236 (execute_subscription, fresh load_config read on every single call), stellar_send/src/lib.rs:152-166 (set_fee, global and immediate).

Companion issue in this same batch: the identical bug for PaymentRequest/fulfill_payment_request, filed separately since the structs, code paths, and blast radius (one-time invoice vs. every future execution of a recurring charge) differ enough to warrant independent tracking and independent fixes, even though the root cause and the fix shape are the same.

Relationship to existing issue #24 ("execute_subscription permits unbounded rapid catch-up execution after keeper downtime"): #24 is about the volume of executions a catch-up burst can produce being unbounded by anything other than max_executions/expiry_time. This issue is about the rate applied per execution being wrong (silently updated mid-subscription-life) — a distinct dimension of the same feature, and the catch-up-burst compounding scenario above is exactly where the two issues intersect and are worth testing together.

Test/reproduction plan: in stellar_send/src/test.rs, using the pattern from test_subscription_create_and_execute (stellar_send/src/test.rs:409-441, one of the only subscription tests that uses a nonzero fee_bps): initialize with fee_bps = 100u32 (1%), create a subscription for amount = 10_000i128, execute it once (nets 9_900 as expected), then call client.set_fee(&500u32) (raise to 5%) before the next next_execution_time arrives, execute again, and assert that — on main today — the second execution deducts the new 5% rate rather than the 1% rate the payer's allowance and expectations were originally sized around; after the fix, assert every execution across the subscription's life continues to use the 1% rate locked in at creation, regardless of intervening set_fee calls.

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26Third CampaignCampaign: Third CampaignbugSomething isn't workingcontractsSmart contract logicsecuritySecurity concernvery hardVery difficult / senior-level bounty issue

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions