Skip to content

fix(subscription): close infinite free-trial bypass#4

Open
maycuatroi1 wants to merge 1 commit intoPiNetwork:mainfrom
maycuatroi1:fix/trial-abuse-bypass
Open

fix(subscription): close infinite free-trial bypass#4
maycuatroi1 wants to merge 1 commit intoPiNetwork:mainfrom
maycuatroi1:fix/trial-abuse-bypass

Conversation

@maycuatroi1
Copy link
Copy Markdown

Summary

The subscription contract's trial-abuse prevention check is ineffective: a subscriber can obtain unlimited free trials on the same service by inducing a single try_transfer_from failure after each trial. No privileged role is needed.

This PR adds a dedicated TrialUsed(subscriber, service_id) persistent key that records trial consumption independently of the subscription lifecycle state, closing the bypass. All 48 existing tests still pass; 3 new regression tests are added.

Vulnerability

subscribe() currently gates trial abuse with:

if had_trial && !auto_renew && service.trial_period_secs > 0 {
    return Err(ContractError::AlreadySubscribed);
}

The check only blocks re-subscription when auto_renew=false. The implicit assumption is that auto_renew=true forces payment on the first post-trial cycle. On-chain, that assumption is not enforced — the subscriber can revoke the token allowance directly via token.approve(contract, 0, 0) (one signature, no contract cooperation). When process() later calls try_transfer_from, it fails; the contract's failure-isolation path sets sub.auto_renew = false on the victim subscription without rolling back the consumed trial. Once service_end_ts lapses, the dedup gate at line 350 admits a new subscription, and the subscriber calls subscribe(auto_renew=true) again — had_trial is true but the check does not fire, so a fresh trial is granted.

Loop cost: one subscribe + one token.approve(0) per trial. Time cost: trial_period_secs. Attacker can scale horizontally by creating Stellar addresses (effectively free).

Concrete impact is proportional to the merchant's cost-to-serve during the trial (compute, bandwidth, API quota, etc.). A reusable trial is not a theft of funds already in the merchant's wallet but a permanent revenue-denial vector and a griefing channel for any service whose trial delivers real value.

Proof of concept

#[test]
fn test_trial_not_regranted_after_payment_failure() {
    let s = setup();
    let svc = register_trial_service(&s);

    let sub1 = s.client.subscribe(&s.subscriber, &svc.service_id, &true);
    assert_eq!(sub1.trial_period_secs, WEEK);

    s.token.approve(&s.subscriber, &s.contract_addr, &0, &0);

    advance_time(&s.env, WEEK + 1);
    let result = s.client.process(&s.merchant, &svc.service_id, &0, &100);
    assert_eq!(result.failed, 1);

    let sub2 = s.client.subscribe(&s.subscriber, &svc.service_id, &true);
    assert_eq!(sub2.trial_period_secs, 0);  // <-- after fix: no second trial
    assert_eq!(sub2.trial_end_ts, 0);
    assert_eq!(s.token.balance(&s.subscriber), INITIAL_BALANCE - PRICE);
}

Before the fix this test would assert sub2.trial_period_secs == WEEK.

Fix

  1. Add DataKey::TrialUsed(Address, u64) to the storage key enum.
  2. In subscribe(), read this flag before computing has_trial; if set, force has_trial = false so re-subscribers skip the trial branch entirely.
  3. When a trial is granted, write the flag and bump its TTL to max_ttl - 1. This outlasts any subscription lifecycle — including TTL-driven GC of the Sub and SubServicePair records, which would otherwise reopen the bypass on a slower timescale.

Behavioural change vs. original contract:

  • Before fix, subscribe(auto_renew=false) after trial expiry returned AlreadySubscribed. After fix, it is allowed and charges the first period upfront. This is a friendlier UX and matches the paid !has_trial branch.
  • subscribe(auto_renew=true) after trial expiry still succeeds; it just no longer receives a second trial.
  • Dedup of overlapping subscriptions is unchanged.

Test plan

  • cargo build --target wasm32v1-none --release -p subscription — compiles clean.
  • cargo test -p subscription --lib — 51/51 pass (48 baseline + 3 new regressions).
  • test_subscribe_trial_abuse_blocked updated to assert the new (correct) semantics.
  • test_trial_not_regranted_after_payment_failure — bypass via allowance revoke is blocked.
  • test_trial_not_regranted_after_balance_drain — bypass via balance drain + re-mint is blocked.
  • test_paid_subscriber_does_not_set_trial_used — a non-trial subscribe to service A does not interfere with a trial grant on service B.

Notes for reviewers

  • No migration needed for fresh deployments. For already-deployed instances, existing subscribers without a TrialUsed flag keep working as before, and the flag gets set lazily the next time anyone calls subscribe() on a trial service.
  • The fix does not touch process(), cancel(), toggle_auto_renew(), or extend_subscription().
  • Discovered during an independent source security review of commit 74c48c6. Separate follow-ups (unbounded index-list growth, is_active dead write, admin upgrade trust boundary) are not addressed in this PR; happy to send follow-up PRs if useful.

Research author: Binhna.

The original subscribe() gates trial abuse with
`had_trial && !auto_renew && service.trial_period_secs > 0`, assuming
that `auto_renew=true` implies the merchant will collect payment. This
assumption is not enforced on-chain.

A subscriber can deterministically bypass the gate:

  1. subscribe(auto_renew=true, trial=T) — trial granted, no payment.
  2. subscriber revokes token allowance (or drains balance) — a direct
     token.approve(contract, 0, 0) only needs the subscriber's sig.
  3. at trial_end_ts the merchant calls process(), try_transfer_from
     fails, contract sets sub.auto_renew = false (failure isolation).
  4. once service_end_ts (== trial_end_ts) lapses the dedup gate lets
     the subscriber call subscribe() again. The had_trial check reads
     the previous subscription's trial_period_secs, but only blocks the
     auto_renew=false branch — the subscriber retries with auto_renew=
     true and receives a fresh trial. Repeatable indefinitely.

Fix: track trial consumption on a dedicated persistent key
`DataKey::TrialUsed(Address, u64)`. Set it whenever a trial is granted,
with a TTL bumped to `max_ttl - 1` so the flag survives long beyond
any subscription lifecycle. A subscriber that already consumed the
trial does not receive another one; `has_trial` falls through to the
paid branch (immediate charge, no trial_end_ts).

This changes the documented re-subscribe UX slightly: after trial
expiry a subscriber may still subscribe with auto_renew=false, but is
charged upfront for the first period instead of being rejected with
AlreadySubscribed. The dedup gate for overlapping subscriptions is
unchanged.

Regression tests:

- test_trial_not_regranted_after_payment_failure — subscriber revokes
  allowance, process() fails, re-subscribe returns trial_period_secs=0
  and charges the first period.
- test_trial_not_regranted_after_balance_drain — same via balance
  drain and re-mint.
- test_paid_subscriber_does_not_set_trial_used — a non-trial service
  subscription does not set the TrialUsed flag, so a subsequent trial
  service from a different merchant still grants its trial.

The pre-existing test_subscribe_trial_abuse_blocked is updated to
reflect the new semantics (re-subscribe allowed, no trial re-granted).
All 51 tests pass (48 baseline + 3 new).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant