diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md new file mode 100644 index 0000000..2655f73 --- /dev/null +++ b/SECURITY-AUDIT.md @@ -0,0 +1,148 @@ +# Security Audit: Subscription Smart Contract + +**Auditor:** [Triumph Synergy Digital Financial Ecosystem](https://github.com/jdrains110-beep/Triumph-Synergy-Digital-Financial-Ecosystem) +**Contract:** `contracts/subscription/src/lib.rs` +**Date:** April 2026 +**Scope:** Full review of the subscription contract Rust source code and architecture + +--- + +## Executive Summary + +The Pi Network Subscription Smart Contract is a well-engineered Soroban contract implementing merchant-initiated pull-payment subscriptions. This audit reviews the `lib.rs` source for security vulnerabilities, logic errors, and best-practice adherence. Overall, the contract demonstrates strong security fundamentals including proper auth checks, overflow protection, and failure isolation. We identify several areas for hardening and a few informational findings. + +--- + +## Findings + +### [S-01] No `deactivate_service` Method — Severity: Medium + +**Location:** `SubscriptionContract` impl block + +**Description:** The `Service` struct includes an `is_active` field, and `subscribe()` correctly checks `!service.is_active` before allowing new subscriptions. However, there is no public method for a merchant to set `is_active = false`. Once a service is registered, it accepts subscribers indefinitely. + +**Impact:** Merchants cannot sunset a service or stop accepting new subscribers without an admin contract upgrade. + +**Recommendation:** Add a `deactivate_service(merchant, service_id)` method that sets `is_active = false` after verifying `merchant.require_auth()` and service ownership. Existing subscriptions should continue to process normally. + +--- + +### [S-02] Unbounded `Vec` Growth in Index Keys — Severity: Medium + +**Location:** `register_service()`, `subscribe()` + +**Description:** `MerchantServices`, `SubscriberSubs`, and `ServiceSubs` are `Vec` that grow unboundedly. Each new service/subscription appends to these vectors. Expired subscription IDs are never pruned from the vectors (only gracefully skipped during reads). + +**Impact:** +- Over time, these vectors may grow large enough to exceed Soroban's per-entry storage size limits or make read operations expensive. +- `get_subscriber_subs` and `get_merchant_subs` iterate the full vector, loading every entry. + +**Recommendation:** Consider either: +1. A lazy cleanup strategy — remove expired IDs during `process()` or query calls. +2. A paginated index structure to avoid unbounded single-entry growth. +3. Document the practical subscription limit per service/subscriber for integrators. + +--- + +### [S-03] `next_service_id` / `next_sub_id` Increment Without Overflow Check — Severity: Low + +**Location:** `next_service_id()`, `next_sub_id()` helper functions + +**Description:** +```rust +env.storage().instance().set(&DataKey::NextServiceId, &(id + 1)); +``` + +The ID counters use `id + 1` without `checked_add`. While `u64::MAX` (~18.4 quintillion) is practically unreachable, the contract uses `checked_add` elsewhere (`checked_add_ts`) demonstrating awareness of overflow. + +**Recommendation:** Use `id.checked_add(1).expect("ID overflow")` for consistency with the contract's overflow-safe design philosophy. + +--- + +### [S-04] `do_approve` Rounds Expiration Down — May Shorten Approval — Severity: Low + +**Location:** `do_approve()` helper + +**Description:** +```rust +let expiration_ledger = (capped / LEDGER_BUCKET) * LEDGER_BUCKET; +``` + +Rounding down to 720-ledger buckets ensures simulate/execute consistency, but can reduce the effective approval duration by up to 719 ledgers (~60 minutes). For short billing periods (e.g., hourly), this could represent a meaningful fraction of the approval window. + +**Recommendation:** Consider rounding **up** instead of down: `((capped + LEDGER_BUCKET - 1) / LEDGER_BUCKET) * LEDGER_BUCKET` (capped at `max_expiration`). This ensures the approval is never shorter than intended. + +--- + +### [S-05] `process()` Does Not Bump `pair_key` TTL — Severity: Low + +**Location:** `process()` function + +**Description:** During batch processing, `process()` bumps the `Sub(sub_id)` TTL on every charge but does not bump the `SubServicePair` TTL. If a subscription is active for a very long time without the subscriber calling any lifecycle method (`cancel`, `toggle_auto_renew`, `extend_subscription`), the `SubServicePair` entry could expire before the subscription itself. + +**Impact:** After `SubServicePair` expires, the deduplication check in `subscribe()` would allow a second active subscription to the same service. + +**Recommendation:** Bump `SubServicePair` TTL in `process()` alongside `Sub(sub_id)`, or document that subscribers must call a lifecycle method periodically. + +--- + +### [S-06] Single Admin Key for `upgrade` — Severity: Informational + +**Location:** `upgrade()` + +**Description:** The `upgrade` function is gated by a single admin address. There is no key rotation mechanism, time lock, or multi-sig pattern. + +**Impact:** Loss of the admin key permanently prevents upgrades. Compromise allows immediate malicious WASM replacement. + +**Recommendation:** For mainnet, consider: +1. An `update_admin` method with dual-auth (old admin + new admin). +2. A time-lock pattern giving the community a window to react before upgrades take effect. + +--- + +### [S-07] `is_subscription_active` Does Not Bump TTLs — Severity: Informational + +**Location:** `is_subscription_active()` + +**Description:** This is a read-only function that accesses `SubServicePair` and `Sub` persistent storage without bumping TTLs. This is by design (query functions use `simulateTransaction`), but integrators relying solely on this function without calling mutating methods may see data expire. + +**Recommendation:** Document that `is_subscription_active` does not extend storage TTLs and that active subscriptions require periodic `process()` calls to maintain storage liveness. + +--- + +## Positive Findings + +These design choices demonstrate strong security engineering: + +| # | Finding | Location | +|---|---------|----------| +| P-01 | **Proper `require_auth()`** on all state-changing functions | All mutating methods | +| P-02 | **`checked_add` for timestamps** prevents overflow panics | `checked_add_ts()`, `do_approve()` | +| P-03 | **`checked_mul` for approval amounts** prevents integer overflow | `do_approve()` | +| P-04 | **Failure isolation** — failed charges disable individual subscriptions, not the batch | `process()` | +| P-05 | **No-drift billing** — `next_charge_ts` advances from previous value, not `now` | `process()` | +| P-06 | **Trial abuse prevention** — prevents infinite free trial reuse | `subscribe()` dedup logic | +| P-07 | **Dynamic TTL** — persistent storage TTL scales with billing period | `ttl_extend_for_period()` | +| P-08 | **`saturating_mul` / `saturating_add`** for TTL calculations prevents panic | `do_approve()`, `ttl_extend_for_period()` | +| P-09 | **Deduplication via `SubServicePair`** prevents duplicate active subscriptions | `subscribe()` | +| P-10 | **Pre-authorized approve before transfer** in non-trial path | `subscribe()` | + +--- + +## Summary Table + +| ID | Title | Severity | Status | +|----|-------|----------|--------| +| S-01 | No `deactivate_service` method | Medium | Open | +| S-02 | Unbounded `Vec` growth in index keys | Medium | Open | +| S-03 | ID counters lack `checked_add` | Low | Open | +| S-04 | Approval expiration rounds down | Low | Open | +| S-05 | `process()` does not bump `SubServicePair` TTL | Low | Open | +| S-06 | Single admin key for `upgrade` | Informational | Open | +| S-07 | `is_subscription_active` TTL behavior | Informational | Open | + +--- + +## Disclaimer + +This audit is provided as a community contribution and does not constitute a formal security guarantee. It is based on source code review as of the audit date. The findings and recommendations are intended to help improve the contract's security posture. \ No newline at end of file