Root + Impact
Description
Root Cause
ConfidencePool maintains two parallel sets of accumulators to track time-weighted stakes for its bonus formula:
The global variables are designed to represent the sum of the time-weighted stakes of all currently eligible stakers. When a staker exits the pool using withdraw(), the contract correctly maintains this invariant by decrementing the global accumulators before deleting the user's storage record:
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
However, this maintenance is completely omitted in the claim paths. In both claimSurvived() and claimExpired() (specifically in its payout tail), the contract deletes the claimant's per-user tracking variables but never decrements the global accumulators:
delete eligibleStake[msg.sender];
delete userSumStakeTime[msg.sender];
delete userSumStakeTimeSq[msg.sender];
Vulnerability Details
As a result of this omission, the moment any staker claims their payout, the global accumulators sumStakeTime/sumStakeTimeSq become permanently stale. They continue to include the time-weighted contributions of users who have already claimed their funds and whose per-user variables have been zeroed/deleted.
This issue does not cause immediate loss of funds or incorrect payout calculations in the current codebase because the bonus distribution calculation (_bonusShare()) does not read the live accumulators. Instead, it reads the frozen snapshots (snapshotSumStakeTime and snapshotSumStakeTimeSq), which are captured at resolution time before any staker is allowed to claim.
However, leaving the live global accumulators stale directly violates the protocol’s core accounting invariants, creating a fragile codebase where any future upgrade or integration that references the live global values (such as partial claims or custom sweep structures) would silently inherit a severe accounting bug.
/ Root cause in the codebase with @> marks to highlight the relevant section
Risk
Likelihood:
-
Reason 1: During the execution of claimSurvived() when a staker claims their payout, immediately following the deletion of their individual stake-time accumulators.
-
Reason 2: During the execution of claimExpired() when a staker resolves the expired pool and receives their principal and bonus, immediately following the clearing of their individual stake-time variables.
Impact:
-
Permanent divergence of global variables sumStakeTime and sumStakeTimeSq from the true sum of eligible users' values, breaking the protocol's core accounting invariants.
-
Introduction of a latent vulnerability/regression vector for future contract upgrades or external integrations that may read the live global variables, as they will read stale and inflated figures.
Proof of Concept
Full walkthrough test (`test/unit/StaleGlobalSumFullNarratedPoC.t.sol`), self-contained from pool deployment to full drain:
```solidity
// Two stakers deposit pre-window (100 ONE, 300 ONE), window opens, both floored to the
// same riskWindowStart, a 40 ONE bonus is contributed, and the pool resolves SURVIVED --
// freezing snapshotSumStakeTime/snapshotSumStakeTimeSq at that instant.
uint256 globalSumBeforeAliceClaims = pool.sumStakeTime();
uint256 aliceOwnSumBeforeClaim = pool.userSumStakeTime(alice); // nonzero
vm.prank(alice);
pool.claimSurvived();
uint256 globalSumAfterAliceClaims = pool.sumStakeTime();
uint256 aliceOwnSumAfterClaim = pool.userSumStakeTime(alice); // correctly 0
assertEq(aliceOwnSumAfterClaim, 0); // per-user: correct
assertEq(globalSumAfterAliceClaims, globalSumBeforeAliceClaims); // global: UNCHANGED — the break
assertTrue(globalSumAfterAliceClaims > 0); // and it's a real, nonzero, orphaned figure
// Bob then claims. His bonus is checked against an INDEPENDENTLY hand-computed
// amount-weighted value (both stakers shared the same floor, so k=2 reduces to a clean
// 100:300 split of the 40 ONE pool) -- confirming Alice's now-stale contribution did not
// leak into his payout:
uint256 expectedBobBonus = (300 * 40 ether) / 400;
assertApproxEqAbs(bobBonus, expectedBobBonus, 1); // exact match
// Pool fully drains to zero; sweepUnclaimedBonus() correctly reverts NothingToSweep
// (it reserves against totalEligibleStake/claimedBonus, not the stale sums); the orphaned
// value is now permanently unreachable (no further deposits or window-observations are
// possible once the pool is resolved).
```
Companion tests in `test/unit/GlobalSumStaleAfterClaimPoC.t.sol` isolate the contrast directly:
```solidity
function testWithdrawCorrectlyDecrementsGlobalSum_ContrastWithClaim() external {
_stake(alice, 100 ether);
_stake(bob, 300 ether);
uint256 sumStakeTimeBefore = pool.sumStakeTime();
uint256 aliceUserSum = pool.userSumStakeTime(alice);
_withdraw(alice);
assertEq(pool.userSumStakeTime(alice), 0);
// withdraw() DOES correctly decrement the global sum -- confirms the asymmetry is real
// and specific to the claim paths.
assertEq(pool.sumStakeTime(), sumStakeTimeBefore - aliceUserSum);
}
```
And a dedicated Foundry invariant (`test/invariant/ConfidencePoolInvariant.t.sol`,
`invariant_globalVsPerUserSumReconciliation`) fuzzes the entire permissionless surface (stake, withdraw, all six claim/sweep paths, flagOutcome, registry transitions, time warps) for 256 runs × 500 calls each and reproducibly breaks on the first claim in the sequence, e.g.:
```
[FAIL: global sumStakeTime diverged from the per-user (post-clamp) reconciliation:
1299318157480819391034000000000 != 0]
stake(actor, ~2.25e47 wei)
warp(...) x3
claimExpired() by a different actor
```
Recommended Mitigation
Mirror `withdraw()`'s pattern in both `claimSurvived()` and `claimExpired()`'s payout tail — decrement the global accumulators by the claimant's own per-user contribution immediately before deleting it:
```solidity
_clampUserSums(msg.sender);
hasClaimed[msg.sender] = true;
uint256 bonusShare = _bonusShare(msg.sender, userEligible);
uint256 payout = userEligible + bonusShare;
totalEligibleStake -= userEligible;
claimedBonus += bonusShare;
+ sumStakeTime -= userSumStakeTime[msg.sender];
+ sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
delete eligibleStake[msg.sender];
delete userSumStakeTime[msg.sender];
delete userSumStakeTimeSq[msg.sender];
```
This costs two extra `SLOAD`s worth of gas (the values are already being read/deleted in the same function) and restores the invariant unconditionally, removing the current reliance on "nothing happens to read the stale value" as an implicit safety property.
- remove this code
+ add this code
