From 928af17b9e93ea9eed31f53ca1a6da90023ef5d4 Mon Sep 17 00:00:00 2001 From: Drew Stone Date: Wed, 24 Jun 2026 14:25:33 -0600 Subject: [PATCH] fix(services): block subscription termination while an earned period is unbilled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subscription owner could call terminateService in the latency window between a billing period fully elapsing and a keeper calling billSubscription, then 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. Gate the owner-path terminateService: revert SubscriptionPeriodUnbilled while a subscription has a fully-elapsed, still-billable (within TTL), operator-owed period. billSubscription is permissionless, so anyone can settle that period in one call, after which the owner terminates and reclaims only the genuinely-unserved tail. The gate is on the owner path only — NOT in _terminateService — so the non-payment remedy terminateServiceForNonPayment (which fires precisely when escrow cannot cover a period and so can never be billed) is not deadlocked. Conditions mirror the eligibility gates in PaymentsBilling._billSubscriptionImpl and must stay in lockstep; in-place settlement is impossible because billing lives in a separate facet unreachable by internal call from the lifecycle facet. Found via red-team exploit PoC, retained as a passing regression. --- src/core/ServicesLifecycle.sol | 22 ++++ src/libraries/Errors.sol | 4 + ...ecycle_TerminateStealsUnbilledPeriod.t.sol | 116 ++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol diff --git a/src/core/ServicesLifecycle.sol b/src/core/ServicesLifecycle.sol index 2e95643..de3fc65 100644 --- a/src/core/ServicesLifecycle.sol +++ b/src/core/ServicesLifecycle.sol @@ -58,12 +58,34 @@ abstract contract ServicesLifecycle is Base { // ═══════════════════════════════════════════════════════════════════════════ /// @notice Terminate a service + /// @dev Owner path only. Blocks while a fully-elapsed subscription period is still unbilled and + /// owed to operators, so the owner cannot terminate inside the keeper's billing-latency + /// window and reclaim (via withdrawRemainingEscrow) escrow operators already earned — the + /// period becomes permanently unbillable once status != Active. `billSubscription` is + /// permissionless, so anyone can clear this in one call, then terminate. The gate lives here + /// (not in `_terminateService`) so the non-payment remedy `terminateServiceForNonPayment` — + /// which fires precisely when escrow cannot cover a period and so can never be billed — is + /// not deadlocked. Conditions mirror the eligibility gates in + /// `PaymentsBilling._billSubscriptionImpl` (subscription, within TTL, full interval elapsed) + /// and MUST be kept in lockstep; settlement itself cannot run here because billing lives in a + /// separate facet unreachable by internal call. A non-empty operator set is a conservative + /// "fees are owed" proxy — an all-inactive set just costs one harmless cursor-advancing bill. function terminateService(uint64 serviceId) external nonReentrant { Types.Service storage svc = _getService(serviceId); if (svc.owner != msg.sender) { revert Errors.NotServiceOwner(serviceId, msg.sender); } + if (svc.status == Types.ServiceStatus.Active && svc.pricing == Types.PricingModel.Subscription) { + uint64 interval = _blueprintConfigs[svc.blueprintId].subscriptionInterval; + bool withinTtl = svc.ttl == 0 || block.timestamp <= svc.createdAt + svc.ttl; + bool periodElapsed = interval != 0 && block.timestamp >= svc.lastPaymentAt + interval; + bool hasOperators = _serviceOperatorSet[serviceId].length() != 0; + if (withinTtl && periodElapsed && hasOperators) { + revert Errors.SubscriptionPeriodUnbilled(serviceId); + } + } + _terminateService(serviceId); } diff --git a/src/libraries/Errors.sol b/src/libraries/Errors.sol index 79ced2e..20f383c 100644 --- a/src/libraries/Errors.sol +++ b/src/libraries/Errors.sol @@ -262,6 +262,10 @@ library Errors { /// Indicates a service-creation path that skipped `initSubscriptionBaseline`. error SubscriptionBaselineNotInitialized(uint64 serviceId); + /// @notice Termination blocked: a fully-elapsed subscription period is still unbilled and owed to + /// operators. Call the permissionless `billSubscription(serviceId)` first, then terminate. + error SubscriptionPeriodUnbilled(uint64 serviceId); + /// @notice EventDriven services are funded per-job via `msg.value`; upfront paymentAmount /// at request is rejected so funds aren't collected only to be locked. error UpfrontPaymentNotAllowedForEventDriven(); diff --git a/test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol b/test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol new file mode 100644 index 0000000..87cd83c --- /dev/null +++ b/test/exploit/Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod.t.sol @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.26; + +import { BaseTest } from "../BaseTest.sol"; +import { Types } from "../../src/libraries/Types.sol"; +import { Errors } from "../../src/libraries/Errors.sol"; + +/// @title Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod +/// @notice Regression for the terminate-gate fix. A subscription owner used to be able to terminate +/// the service while a FULLY-ELAPSED billing period was still unbilled, then sweep the entire +/// escrow — including the period operators already served (the period becomes permanently +/// unbillable once status != Active). +/// +/// Invariant restored: operators must be paid for every subscription period they actually served +/// before the customer can reclaim escrow for that period. +/// +/// Fix: owner-path `terminateService` reverts `SubscriptionPeriodUnbilled` while a fully-elapsed, +/// operator-owed, still-billable period exists. `billSubscription` is permissionless, so anyone can +/// settle that period first; only then can the owner terminate and reclaim the genuinely-unserved +/// tail. The gate is on the owner path only, so the non-payment remedy is not deadlocked. +contract Exploit_ServicesLifecycle_TerminateStealsUnbilledPeriod is BaseTest { + uint256 constant SUBSCRIPTION_RATE = 1 ether; + uint64 constant SUBSCRIPTION_INTERVAL = 30 days; + + function test_Regression_TerminateBlockedUntilEarnedPeriodSettled() public { + uint256 startTime = 1_000_000; + vm.warp(startTime); + + Types.BlueprintConfig memory config = Types.BlueprintConfig({ + membership: Types.MembershipModel.Fixed, + pricing: Types.PricingModel.Subscription, + minOperators: 1, + maxOperators: 5, + subscriptionRate: SUBSCRIPTION_RATE, + subscriptionInterval: SUBSCRIPTION_INTERVAL, + eventRate: 0 + }); + + vm.prank(developer); + uint64 blueprintId = _createBlueprintWithConfigAsSender("ipfs://terminate-steal", address(0), config); + + // Operator stakes and registers to actually serve the subscription. + vm.prank(operator1); + staking.registerOperator{ value: 5 ether }(); + vm.prank(operator1); + staking.setDelegationMode(Types.DelegationMode.Open); + _directRegisterOperator(operator1, blueprintId, ""); + + // Customer (service owner = user1) funds 6 periods of escrow. + uint256 escrowAmount = SUBSCRIPTION_RATE * 6; + address[] memory operators = new address[](1); + operators[0] = operator1; + address[] memory callers = new address[](0); + + vm.prank(user1); + uint64 requestId = tangle.requestService{ value: escrowAmount }( + blueprintId, operators, "", callers, 0, address(0), escrowAmount, Types.ConfidentialityPolicy.Any + ); + + vm.prank(operator1); + tangle.approveService(_approve(requestId)); + + uint64 serviceId = 0; + assertTrue(tangle.isServiceActive(serviceId), "service should be active"); + + // --- Period 1: served AND billed (the honest case). --- + vm.warp(startTime + 31 days); + tangle.billSubscription(serviceId); + + uint256 escrowAfterPeriod1 = tangle.getServiceEscrow(serviceId).balance; + // ~5 periods of escrow remain after billing the first period (TWAP dust tolerance). + assertApproxEqAbs(escrowAfterPeriod1, SUBSCRIPTION_RATE * 5, 1, "5 periods should remain"); + + // --- Period 2: operator serves a FULL second interval, but no keeper has billed yet + // (the normal latency window between period end and billing). --- + vm.warp(startTime + 62 days); + assertGe(block.timestamp, startTime + 62 days, "period 2 fully elapsed"); + + uint256 owedToOperatorsForPeriod2 = SUBSCRIPTION_RATE; + + // ── FIX: the owner CANNOT terminate while the earned period is unbilled. ── + vm.prank(user1); + vm.expectRevert(abi.encodeWithSelector(Errors.SubscriptionPeriodUnbilled.selector, serviceId)); + tangle.terminateService(serviceId); + assertTrue(tangle.isServiceActive(serviceId), "service still active: termination blocked"); + + // Anyone (here, an operator acting as keeper) settles the earned period permissionlessly. + vm.prank(operator2); + tangle.billSubscription(serviceId); + + uint256 escrowAfterSettle = tangle.getServiceEscrow(serviceId).balance; + // The earned period-2 fee left escrow to operators, dropping the balance by ~one period. + assertApproxEqAbs( + escrowAfterPeriod1 - escrowAfterSettle, + owedToOperatorsForPeriod2, + 1e12, + "billing settled ~1 earned period out of escrow to operators" + ); + + // ── Now the owner CAN terminate, and reclaims only the genuinely-unserved tail (~4 periods). ── + uint256 ownerBalanceBefore = user1.balance; + vm.prank(user1); + tangle.terminateService(serviceId); + assertFalse(tangle.isServiceActive(serviceId), "service terminated after settlement"); + + vm.prank(user1); + tangle.withdrawRemainingEscrow(serviceId); + uint256 refundedToOwner = user1.balance - ownerBalanceBefore; + + // Owner gets only the post-settlement remainder — one full period SHORT of the pre-termination + // escrow, exactly the earned value the exploit used to steal. + uint256 honestMaxRefund = escrowAfterPeriod1 - owedToOperatorsForPeriod2; + assertApproxEqAbs(refundedToOwner, honestMaxRefund, 1e12, "owner refunded only the unserved tail"); + assertLt(refundedToOwner, escrowAfterPeriod1 - 1e12, "owner did NOT sweep the earned period"); + } +}