fix(sra): prevent activeQ regression and align remove exclusion with the share map

Two review findings (PR filecoin-project#24 review round 2):

B1 (blocking): correctVolume/postVolume could regress activeQ. The advance
guard assumed the mirror only moves forward, but the constructor did not
enforce non-overlapping windows (POST + VERIFY <= EPOCHS). With overlapping
windows a governance call to correctVolume(older q) legitimately fell inside
the still-open verification window and _advanceMirror rewound activeQ,
clearing the newer quarter's posted FPV, re-enabling duplicate posts and
double-counting totalUsd.

- constructor: require(postPeriod + verificationWindow <= epochsPerQuarter)
  to eliminate overlapping windows at the configuration level;
- _assertMirrorWindow: q must be activeQ or activeQ + 1 (write entry
  semantics - a write can only target the current or the next quarter;
  skipping quarters would misalign prevFpv, same family as the A3 fix).
  Applied before the advance in postVolume and correctVolume as defense
  in depth.

S1 (should fix): remove deducted totalUsd after the quarter was bound,
drifting the aggregatedFPV historical snapshot. The exclusion boundary must
mirror the share map collection, not the freeze boundary: remove drops the
orchestrator from the admitted list (so the map no longer includes it),
while freeze keeps it. Deduct only while the quarter is not yet bound
(!_afterBinding): posting-window and verification-window removals both
exclude the FPV from the aggregate, keeping aggregatedFPV == map sum;
after binding the snapshot stays fixed.

Regression tests (Red -> Green):
- Ctor_WindowOverlap_Rejected / Ctor_WindowBoundary_Accepted
- PostVolume_SkipQuarter_Reverts / CorrectVolume_SkipQuarter_Reverts
- Remove_InPostingWindow_DeductsAggregate
- Remove_InVerificationWindow_Excludes (probe scenario formalised)
- Remove_AfterBinding_KeepsSnapshot

311 tests pass (17 suites incl. invariant fuzz); 3 seeds; halmos 2/2.

Author: LinZexiao

src/ServiceRewardsActor.sol

@@ -165,6 +165,11 @@ contract ServiceRewardsActor is UnanimousGovernance {
         // deployment-time parameter validation, aligned with setPricingParams
         require(priceBand <= BASIS_POINTS, InvalidParameter());
         require(epochsPerQuarter > 0 && postPeriod > 0 && verificationWindow > 0, InvalidParameter());
+        // Review B1: the mirror advances only forward, so the windows must not overlap — a quarter's
+        // verification window closing after the next quarter has begun would let a governance
+        // CorrectVolume target an already-advanced quarter, rewinding activeQ (uint256 intermediate
+        // guards the addition against overflow).
+        require(uint256(postPeriod) + uint256(verificationWindow) <= uint256(epochsPerQuarter), InvalidParameter());
 
         EPOCHS_PER_QUARTER = Epoch.wrap(epochsPerQuarter);
         POST_PERIOD = Epoch.wrap(postPeriod);
@@ -237,6 +242,18 @@ contract ServiceRewardsActor is UnanimousGovernance {
         return (false, 0);
     }
 
+    /// @dev Mirror advance guard (review B1): a write may target the active quarter (q == activeQ,
+    ///      same-quarter updates) or the next quarter (q == activeQ + 1 — the first write of a new
+    ///      quarter advances the mirror). Anything else is rejected: q < activeQ would rewind the
+    ///      mirror (backing up and clearing a later quarter's contributions — possible when the
+    ///      windows overlap, hence also forbidden at the constructor), and q > activeQ + 1 would
+    ///      skip a quarter, misaligning the prevFpv mirror (it can only hold activeQ - 1's data).
+    ///      The window checks already bound q in a non-overlapping configuration; this guard is
+    ///      defense-in-depth if the deployment parameters ever change.
+    function _assertMirrorWindow(SraStorage.SraStorageQuarter storage qt, uint64 q) internal view {
+        require(q == qt.activeQ || q == qt.activeQ + 1, InvalidParameter());
+    }
+
     /// @dev Mirror advance: the first write of a new quarter (postVolume or correctVolume with
     ///      q != activeQ) backs the active-quarter contributions up into the previous-quarter
     ///      mirror — exclusion-fixed (frozenAtPostEnd ? 0 : fpv), because the freeze state of the
@@ -303,6 +320,7 @@ contract ServiceRewardsActor is UnanimousGovernance {
         // Mirror advance on the first write of the quarter (q == activeQ afterwards; the
         // previous quarter's contributions back up into prevFpv, exclusion-fixed). The advance
         // clears the active slot, so the already-posted check below is against the new quarter.
+        _assertMirrorWindow(qt, q);
         if (qt.activeQ != q) _advanceMirror(qt, q);
         require(FixedU18.unwrap(o.fpv) == 0, AlreadyPosted(q));
         o.fpv = fpv; // FixedU18 — 18-decimal USD, type-checked from the entry
@@ -349,11 +367,17 @@ contract ServiceRewardsActor is UnanimousGovernance {
         require(o.admitted, NotAdmitted(orch));
         (bool hasPending, uint64 pendingQ) = _pendingSharesQuarter();
         if (hasPending) revert PendingShares(pendingQ);
-        // Mirror: drop the active-quarter contribution if it is still effective — a freeze
-        // before E+POST already deducted it (frozenAtPostEnd), so no double deduction.
-        if (!o.frozenAtPostEnd && FixedU18.unwrap(o.fpv) > 0) {
-            SraStorage.SraStorageQuarter storage qt = _quarter();
-            qt.totalUsd[qt.activeQ] = qt.totalUsd[qt.activeQ] - o.fpv;
+        // Mirror: drop the active-quarter contribution from the aggregate while the quarter is not
+        // yet bound — an orchestrator removed before binding is excluded: omitted from the
+        // submitted share map (it leaves the admitted list, which submitShares collects) and its
+        // FPV does not enter AggregatedFPV(Q) (spec §2.2). Once the verification window has closed
+        // the aggregate is a binding snapshot (the read view exposes the bound values directly) and
+        // a later removal must not rewrite it. The boundary is binding (not E+POST — freeze's
+        // boundary): unlike freeze, removal drops the orchestrator from the admitted list, so the
+        // map and the aggregate must exclude it together for every pre-binding removal (review S1).
+        uint64 q = _quarter().activeQ;
+        if (!_afterBinding(q) && !o.frozenAtPostEnd && FixedU18.unwrap(o.fpv) > 0) {
+            _quarter().totalUsd[q] = _quarter().totalUsd[q] - o.fpv;
         }
         o.admitted = false;
         o.frozenSince = Epoch.wrap(0);
@@ -518,7 +542,10 @@ contract ServiceRewardsActor is UnanimousGovernance {
         SraStorage.SraStorageQuarter storage qt = _quarter();
 
         // Mirror advance on the first write of the quarter — correctVolume can be the first
-        // writer (supplying recomputed figures for a quarter nobody posted).
+        // writer (supplying recomputed figures for a quarter nobody posted). Bounded by the
+        // window guard: q must be the active or the next quarter (rewinding would clear a later
+        // quarter's contributions — review B1).
+        _assertMirrorWindow(qt, q);
         if (qt.activeQ != q) _advanceMirror(qt, q);
 
         // Read the old value *after* the advance: on an advance the previous

test/SRAAdversarial.t.sol

@@ -331,6 +331,72 @@ contract SRAAdversarial is SRATestBase {
     // helpers
     // ------------------------------------------------------------------------
 
+    // ------------------------------------------------------------------------
+    // Review B1: mirror-advance direction guard + window-overlap constructor constraint
+    // ------------------------------------------------------------------------
+
+    /// Constructor rejects window overlap: a quarter's verification window must close before the
+    /// next quarter begins (POST + VERIFY <= EPOCHS), otherwise a governance CorrectVolume could
+    /// target an already-advanced quarter and rewind the mirror (review B1).
+    function test_Ctor_WindowOverlap_Rejected() public {
+        vm.expectRevert(abi.encodeWithSelector(ServiceRewardsActor.InvalidParameter.selector));
+        new ServiceRewardsActor(
+            owner1,
+            owner2,
+            500, // EPOCHS
+            300, // POST
+            400, // VERIFY: 300 + 400 = 700 > 500 -> overlap
+            SRA_CANCEL_HOLD,
+            ACTIVATION_EPOCH,
+            MIN_LOT,
+            PRICE_BAND
+        );
+    }
+
+    /// Constructor accepts the exact boundary: POST + VERIFY == EPOCHS (verification closes at the
+    /// next quarter's boundary — no overlap, the mirror stays forward-only).
+    function test_Ctor_WindowBoundary_Accepted() public {
+        // deployment succeeded (no revert) — the boundary POST + VERIFY == EPOCHS is accepted
+        ServiceRewardsActor tight = new ServiceRewardsActor(
+            owner1,
+            owner2,
+            700, // EPOCHS
+            300, // POST
+            400, // VERIFY: 300 + 400 = 700 == EPOCHS -> accepted
+            SRA_CANCEL_HOLD,
+            ACTIVATION_EPOCH,
+            MIN_LOT,
+            PRICE_BAND
+        );
+    }
+
+    /// A write must target the active or the next quarter: skipping a quarter (q > activeQ + 1)
+    /// would misalign the prevFpv mirror (it can only hold activeQ - 1's data) — rejected by the
+    /// mirror-window guard (review B1).
+    function test_PostVolume_SkipQuarter_Reverts() public {
+        address orch = makeAddr("orch");
+        _admit(orch);
+
+        vm.roll(_qEnd(2) + 1); // Q2 posting window; mirror still at genesis activeQ = 0
+        vm.prank(orch);
+        vm.expectRevert(abi.encodeWithSelector(ServiceRewardsActor.InvalidParameter.selector));
+        sra.postVolume(2, FixedU18.wrap(_fpv(100e18)));
+    }
+
+    /// Same guard on the governance path: CorrectVolume may not target a quarter beyond the next
+    /// one (unanimousNoHold: the second approval executes the body and reverts).
+    function test_CorrectVolume_SkipQuarter_Reverts() public {
+        address orch = makeAddr("orch");
+        _admit(orch);
+
+        vm.roll(_qEnd(2) + POST_PERIOD + 1); // Q2 verification window; activeQ still 0
+        vm.prank(owner1);
+        sra.correctVolume(orch, 2, FixedU18.wrap(_fpv(100e18)));
+        vm.prank(owner2);
+        vm.expectRevert(abi.encodeWithSelector(ServiceRewardsActor.InvalidParameter.selector));
+        sra.correctVolume(orch, 2, FixedU18.wrap(_fpv(100e18)));
+    }
+
     function _sumShares(Share[] memory shares) internal pure returns (uint256 sum) {
         for (uint256 i = 0; i < shares.length; i++) {
             sum += shares[i].share;

test/SRAggregateMirror.t.sol

@@ -395,4 +395,75 @@ contract SRAggregateMirrorTest is SRATestBase {
         assertEq(shares[0].wallet, a, "map still the q1 distribution");
         assertEq(FixedU18.unwrap(sra.aggregatedFPV(2)), 0, "q2 has no contributions");
     }
+
+    /// Review S1: an orchestrator removed before the close of the posting period is excluded —
+    /// its FPV does not enter AggregatedFPV(Q) (spec §2.2). With activeQ still in its posting
+    /// window the removal deducts the contribution from the aggregate (read once the quarter binds).
+    function test_Mirror_Remove_InPostingWindow_DeductsAggregate() public {
+        address a = makeAddr("a");
+        address b = makeAddr("b");
+        _admit(a);
+        _admit(b);
+
+        vm.roll(_qEnd(0) + 1); // Q0 posting window
+        _postAs(a, 0, _fpv(100e18));
+        _postAs(b, 0, _fpv(200e18));
+
+        _remove(b); // still within E+POST (hold 100 < POST 300): contribution excluded
+
+        vm.roll(_qVerifyEnd(0) + 1); // Q0 binds — aggregate readable
+        assertEq(FixedU18.unwrap(sra.aggregatedFPV(0)), 100e18, "removed pre-E+POST FPV excluded");
+    }
+
+    /// Review S1: once the verification window closes, AggregatedFPV(activeQ) is a fixed binding
+    /// snapshot (spec §2.2: the read view exposes the bound values directly). A removal after
+    /// binding must not rewrite it — only a pre-E+POST removal excludes the contribution. The
+    /// former code deducted totalUsd unconditionally, drifting the bound aggregate.
+    function test_Mirror_Remove_AfterBinding_KeepsSnapshot() public {
+        address a = makeAddr("a");
+        address b = makeAddr("b");
+        _admit(a);
+        _admit(b);
+
+        vm.roll(_qEnd(0) + 1); // Q0 posting window
+        _postAs(a, 0, _fpv(100e18));
+        _postAs(b, 0, _fpv(200e18));
+
+        vm.roll(_qVerifyEnd(0) + 1); // Q0 binds
+        sra.submitShares(0);
+        Share[] memory shares = rewardActor().getShares(SERVICE_STREAM_ID);
+        assertEq(shares.length, 2, "both contributors in the bound map");
+        assertEq(FixedU18.unwrap(sra.aggregatedFPV(0)), 300e18, "bound aggregate");
+
+        _remove(b); // post-binding removal: aggregate is a binding snapshot, must not drift
+        assertEq(FixedU18.unwrap(sra.aggregatedFPV(0)), 300e18, "bound aggregate unchanged after removal");
+        shares = rewardActor().getShares(SERVICE_STREAM_ID);
+        assertEq(shares.length, 2, "submitted map stands (removal does not rewrite a submitted map)");
+    }
+
+    /// Review S1: a removal in the verification window (E+POST passed, not yet bound) must exclude
+    /// the orchestrator from BOTH the share map (it leaves the admitted list, which submitShares
+    /// collects) and the aggregate — otherwise aggregatedFPV(0) = 300 != map sum 100. The former
+    /// E+POST boundary (freeze's) left the aggregate at 300 while the map dropped b (probe).
+    function test_Mirror_Remove_InVerificationWindow_Excludes() public {
+        address a = makeAddr("a");
+        address b = makeAddr("b");
+        _admit(a);
+        _admit(b);
+
+        vm.roll(_qEnd(0) + 1); // Q0 posting window
+        _postAs(a, 0, _fpv(100e18));
+        _postAs(b, 0, _fpv(200e18));
+
+        vm.roll(_qPostEnd(0) + 1); // Q0 verification window (E+POST+1); the 100-epoch hold keeps the
+        // executing removal inside the window (E+401 < E+700), not yet bound
+        _remove(b);
+
+        vm.roll(_qVerifyEnd(0) + 1); // Q0 binds
+        sra.submitShares(0);
+        Share[] memory shares = rewardActor().getShares(SERVICE_STREAM_ID);
+        assertEq(shares.length, 1, "removed orchestrator absent from the map");
+        assertEq(shares[0].wallet, a, "map = a only");
+        assertEq(FixedU18.unwrap(sra.aggregatedFPV(0)), 100e18, "removed FPV excluded -- consistent with map");
+    }
 }