fix(services): block subscription termination while an earned period is unbilled

[drewstone]

What

Owner-path terminateService now reverts SubscriptionPeriodUnbilled while a subscription has a fully-elapsed, still-billable, operator-owed period. Anyone can clear it with the permissionless billSubscription(serviceId), after which the owner terminates and reclaims only the unserved tail.

Why

A subscription owner could terminate inside the keeper's billing-latency window (period elapsed, not yet billed) and withdrawRemainingEscrow to reclaim the entire escrow — including the period operators had already served. Once status flips to Terminated, _billSubscriptionImpl rejects the service (status != Active), so the earned period becomes permanently unbillable and operators lose those fees. Confirmed real by a red-team exploit PoC.

Design

• Gate, don't auto-settle. In-place settlement during terminate is impossible here: terminateService runs in TangleServicesLifecycleFacet and billing lives in TanglePaymentsFacet — separate facets, separate delegatecall code paths. An internal-call hook compile-conflicts and silently no-ops at runtime. So the lifecycle facet computes eligibility from shared storage and reverts, directing the caller to bill first (permissionless, anyone can).
• Owner path only. The gate is in terminateService, not _terminateService, so terminateServiceForNonPayment — which fires precisely when escrow can't cover a period and therefore can never be billed — is not deadlocked.
• Conditions mirror PaymentsBilling._billSubscriptionImpl (subscription, within TTL, full interval elapsed since lastPaymentAt) and must be kept in lockstep. A non-empty operator set is a conservative "fees are owed" proxy; an all-inactive set just costs one harmless cursor-advancing bill before terminate proceeds.

Verification

• New regression test_Regression_TerminateBlockedUntilEarnedPeriodSettled: terminate reverts while period unbilled → permissionless bill settles ~1 period to operators → owner then terminates and is refunded only the unserved tail. Passes.
• Broader subscription + lifecycle suites: 132/132 pass (no existing terminate/billing flow, including non-payment termination, regressed).

Lineage

Third and final fix from the red-team pass. Siblings: emergencyWithdraw token guard (#189, merged) and vesting delegatee front-run (#190). The "HIGH" cross-operator cost-basis claim was debunked (its PoC failed its own drain assertion).

[tangletools]

✅ Auto-approved PR — 928af17b

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-24T20:26:00Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	2 (2 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	93.9s (2 bridge agents)
Total	93.9s

💰 Value — sound-with-nits

Blocks owner-initiated subscription termination while a fully-elapsed period remains unbillable, preventing owners from sweeping escrow that operators already earned; a targeted, facet-aware fix that mirrors existing codebase patterns but duplicates billing-eligibility logic that must stay in lockst

• What it does: Adds a revert gate in ServicesLifecycle.terminateService (src/core/ServicesLifecycle.sol:79-87) that rejects owner termination of an active subscription when: the service is within TTL, a full billing interval has elapsed since lastPaymentAt, and at least one operator is in the service operator set. It adds a new error SubscriptionPeriodUnbilled (src/libraries/Errors.sol:265-268) and a regre
• Goals it achieves: Closes a red-team exploit where a subscription owner could terminate during the keeper billing-latency window and reclaim the entire escrow, including the period operators had already served. Once status flips to Terminated, _billSubscriptionImpl refuses to bill (PaymentsBilling.sol:90-93), so that earned period became permanently unbillable and operators lost the fee. The change restores the
• Assessment: Good change on its merits. The gate is placed only on the owner path (terminateService), leaving the non-payment remedy (terminateServiceForNonPayment) unblocked, which is correct because that path fires precisely when escrow cannot cover a period and therefore can never be billed. The design is coherent with the diamond/facet architecture: lifecycle and billing live on separate facets (Tangle
• Better / existing approach: Partial existing equivalent, but not a drop-in replacement. PaymentsRewards._isBillable (src/core/PaymentsRewards.sol:196-210) already mirrors _billSubscriptionImpl preconditions for off-chain keepers, but it lives in a separate facet and includes checks (escrow.balance >= subscriptionRate and subscriptionBaselineStake != 0) that deliberately must NOT be enforced here — insufficient escrow
• Model: opencode/kimi-for-coding/k2p7
• Bridge attempts: 1

🎯 Usefulness — sound-with-nits

A well-placed, minimal gate on the owner termination path that blocks the real escrow-theft exploit and correctly avoids deadlocking the non-payment remedy; the only improvement is extracting the duplicated eligibility predicate so it cannot drift from billing.

• Integration: Fully reachable and wired into live surface. The gate is inline in terminateService (src/core/ServicesLifecycle.sol:73-90), whose selector is registered on TangleServicesLifecycleFacet (src/facets/tangle/TangleServicesLifecycleFacet.sol:16) as the owner-facing termination path — it runs on every subscription terminate. The new Errors.SubscriptionPeriodUnbilled (src/libraries/Errors.sol:265-2
• Fit with existing patterns: Fits the diamond architecture precisely. The 'gate-and-revert, point to the permissionless bill' design is the right shape: terminateService lives in TangleServicesLifecycleFacet while billing lives in the payments facet, and an internal call cannot cross facet contracts — verified the facet split is real (src/facets/tangle/TangleServicesLifecycleFacet.sol:11 vs PaymentsBilling). Auto-settleme
• Real-world viability: Holds under realistic paths. The owner is never permanently stuck: if a period elapsed and billing succeeds, one billSubscription call clears the gate; the owner can also self-clear before terminating. The conservative hasOperators = _serviceOperatorSet[serviceId].length() != 0 (ServicesLifecycle.sol:83) vs billing's _activeServiceOperators (PaymentsBilling.sol:114) divergence is benign and
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

💰 Value Audit

🟡 Billing-eligibility predicate duplicated and must stay in lockstep [maintenance] ``

ServicesLifecycle.terminateService (src/core/ServicesLifecycle.sol:79-87) manually reimplements a subset of _billSubscriptionImpl eligibility (PaymentsBilling.sol:89-112). The dev comment explicitly warns the conditions "MUST be kept in lockstep." This is the same maintenance hazard already accepted for PaymentsRewards._isBillable (src/core/PaymentsRewards.sol:196-210), but any future change to billing preconditions must be reflected here or the gate could drift. Consider a shared view/lib

🎯 Usefulness Audit

🟡 Eligibility conditions duplicated across the gate and _billSubscriptionImpl — drift risk [ergonomics] ``

The gate re-derives the four billing eligibility predicates inline (ServicesLifecycle.sol:79-84: status/pricing/withinTtl/periodElapsed) and the PR correctly flags they 'MUST be kept in lockstep' with PaymentsBilling._billSubscriptionImpl (PaymentsBilling.sol:90-112). Both ServicesLifecycle and PaymentsBilling inherit from Base (verified: PaymentsBilling → PaymentsCore → Base, ServicesLifecycle → Base) and read the same shared storage, so a small internal view predicate like `_subscripti

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260624T212716Z}

[tangletools]

✅ No Blockers — 928af17b

Readiness 83/100 · Confidence 75/100 · 9 findings (9 low)

	opencode-kimi	glm	deepseek	aggregate
Readiness	92	83	83	83
Confidence	75	75	75	75
Correctness	92	83	83	83
Security	92	83	83	83
Testing	92	83	83	83
Architecture	92	83	83	83

Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.

🟡 LOW Billing eligibility predicate duplicated in terminateService — src/core/ServicesLifecycle.sol

Lines 79-87 reproduce the active/subscription/TTL/period-due checks from PaymentsBilling._billSubscriptionImpl (PaymentsBilling.sol:90-112) and add an operator-set proxy. The NatSpec explicitly states these conditions 'MUST be kept in lockstep.' If a future change updates billing eligibility (e.g., a new precondition, TTL comparison, or operator-count check) without updating this gate, the owner path could either deadlock legitimate terminations or reintroduce the unbilled-period escrow sweep the fix closes. Prefer extracting a shared internal view in Base/PaymentsCore so both paths evaluate the same predicate.

🟡 LOW Billing-eligibility arithmetic duplicated across facets; lockstep maintained by comment only — src/core/ServicesLifecycle.sol

Lines 79-87 re-derive the same TTL/period-due conditions present in PaymentsBilling._billSubscriptionImpl (lines 90-112): withinTtl = svc.ttl == 0 || block.timestamp <= svc.createdAt + svc.ttl mirrors billing's svc.ttl > 0 && block.timestamp > svc.createdAt + svc.ttl, and periodElapsed = interval != 0 && block.timestamp >= svc.lastPaymentAt + interval mirrors billing's block.timestamp < periodEnd where periodEnd = lastPaymentAt + interval. The dev comment at [lines 68-72](https://github.com/tangle-network/tnt-core/blob/928a

🟡 LOW Gate conditions require ongoing lockstep with _billSubscriptionImpl — src/core/ServicesLifecycle.sol

L79-87: The gate's eligibility conditions (withinTtl, periodElapsed, hasOperators) mirror _billSubscriptionImpl's pre-checks. If billing eligibility changes (e.g., a new early-exit condition is added to _billSubscriptionImpl without updating this gate), the gate could either deadlock (blocking termination for unbillable periods) or permit the exploit (allowing termination for billable periods). The dev comment at L68-70 does note this requirement, but there is no automated enforcement. Consider adding a shared internal view function (e.g., _isSubscriptionPeriodBillable) consumed by both sites to eliminate the fork risk.

🟡 LOW No test coverage for pause-window liveness interaction — src/core/ServicesLifecycle.sol

terminateService (line 73) has nonReentrant but NOT whenNotPaused; billSubscription (PaymentsBilling.sol:19) has whenNotPaused nonReentrant. If the protocol is paused after a period elapses but before it's billed, the owner cannot terminate (gate reverts SubscriptionPeriodUnbilled) and no one can bill (pause reverts) — the service is liveness-locked until unpause. I verified this is correct-by-design (allowing termination during pause would re-open the exact theft vector, since billing can't pay operators while paused; admin unpause is the recovery path, escrow is safe not lost). But there is no test asserting this behavior, so a future refactor could accidentally

🟡 LOW Error doc omits two gating conjuncts used at the revert site — src/libraries/Errors.sol

Lines 265-267: the @notice says termination is blocked because 'a fully-elapsed subscription period is still unbilled', but the actual gate in ServicesLifecycle.sol:80-85 ALSO requires withinTtl (svc.ttl == 0 || block.timestamp <= createdAt + ttl) AND hasOperators (operator set non-empty). A reader inspecting only the error could believe a fully-elapsed period always blocks termination even when the service is past TTL or has no operators. Impact: documentation precision only — no behavioral risk. Fix: extend the @notice, e.g. 'Termination blocked: a fully-elapsed subscription period within TTL is still unbilled and owed to a non-empty operator set.'

🟡 LOW Brittle 1-wei tolerance on escrow-after-first-bill assertion — test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol

Line 72: assertApproxEqAbs(escrowAfterPeriod1, SUBSCRIPTION_RATE * 5, 1, "5 periods should remain") uses 1 wei tolerance. PaymentLib.twapBillAmount applies TWAP ratio scaling that can round below the exact nominal rate even when stake hasn't changed. While the current setup (operator staked pre-activation, zero stake delta) should produce exactly 1 ether, the tolerance is tighter than every other billing assertion in this test (which use 1e12). Future billing math changes could break this assertion without a real bug. Suggestion: relax to 1e12 for consistency.

🟡 LOW Missing gate-bypass boundary tests — test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol

The gate has four AND conditions plus a pricing/status pre-check (Lines 79-87 of ServicesLifecycle.sol). The test only verifies the happy-path gate-blocking case. Untested bypasses: (a) termination when a period is NOT yet fully elapsed should succeed, (b) termination when all operators have left (hasOperators==false) should succeed, (c) termination when service TTL has expired (withinTtl==false) should succeed. These are secondary to the core regression but the gate comment explicitly says conditions MUST be kept in lockstep with _billSubscriptionImpl — verifying the negative cases ensures no future dev accidentally

🟡 LOW Negative-branch coverage gap on the 3 gate conditions — test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol

The fix at ServicesLifecycle.sol:79-87 has three gating conditions (withinTtl, periodElapsed, hasOperators) and the test only exercises the all-true branch (line 82-86). No assertion verifies termination still PROCEEDS when any single condition is false — e.g. when ttl has expired (withinTtl=false, which is the boundary the dev comment flags as intentional so non-payment remedy isn't deadlocked), when operators have deregistered (hasOperators=false), or before the period fully elapses (periodElapsed=false). A future tightening regression (e.g., someone changes && to ||, or drops the withinTtl clause) would

🟡 LOW Test coverage: edge cases not exercised — test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol

Single regression test covers the main exploit scenario (period elapsed + operators + within TTL). Missing: (a) TTL-expired service — verify termination is unblocked when the period can't be billed, (b) zero-operator service — verify termination is unblocked when no one served, (c) interval=0 degenerate config — verify no revert, (d) mid-period termination (period not yet elapsed) — verify unblocked. Each is a correctness boundary.

---

_{tangletools · 2026-06-24T22:06:42Z · trace}

[tangletools]

✅ Approved — 9 non-blocking findings — 928af17b

Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-06-24T22:06:42Z · immutable trace}