BattleChain Confidence Pools
Cyfrin
FoundrySolidityLayer 2
7.25 ETH
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: medium

`_clampUserSums()` call inside `withdraw()` is unreachable dead code, masking a latent accounting underflow on future upgrades

Author Revealed upon completion

Short Summary

withdraw() calls _clampUserSums(msg.sender) at line 305, but the function's own gate at line 293 ensures riskWindowStart == 0 before allowing execution. _clampUserSums immediately returns when riskWindowStart == 0 (line 680), making the call a guaranteed no-op. This wastes gas on every withdrawal and introduces a silent regression trap: if the withdrawal gate is ever relaxed in a future upgrade, the live-but-wrong clamp would corrupt global accounting.

Root Cause

https://github.com/CodeHawks-Contests/2026-07-bc-confidence-pools/blob/58e8ba4ce3f3277866e4926f3140e597f9554a1e/src/ConfidencePool.sol#L288-L318

https://github.com/CodeHawks-Contests/2026-07-bc-confidence-pools/blob/58e8ba4ce3f3277866e4926f3140e597f9554a1e/src/ConfidencePool.sol#L677-L684

// src/ConfidencePool.sol L288-318
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
​
// Gate: riskWindowStart MUST be 0 to reach past this point.
if (
riskWindowStart != 0 // ← one-way latch
|| (state != IAttackRegistry.ContractState.NOT_DEPLOYED
&& state != IAttackRegistry.ContractState.NEW_DEPLOYMENT
&& state != IAttackRegistry.ContractState.ATTACK_REQUESTED)
) {
revert WithdrawsDisabled();
}
​
uint256 amount = eligibleStake[msg.sender];
if (amount == 0) revert InvalidAmount();
​
_clampUserSums(msg.sender); // DEAD: start == 0 → immediate return below
​
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
...
}
​
// src/ConfidencePool.sol L677-684
function _clampUserSums(address u) internal {
uint256 start = riskWindowStart; // always 0 when called from withdraw()
if (start == 0 || stake_ == 0) return; // ← ALWAYS fires from withdraw()
...
}

Proper Description

The developer intended _clampUserSums to adjust a staker's per-user sums to reflect a non-zero riskWindowStart before any arithmetic that uses those sums. This is correct when called from stake() or claimSurvived() — in those contexts riskWindowStart may be non-zero.

In withdraw(), however, the intent is moot. The withdrawal gate (riskWindowStart != 0 → WithdrawsDisabled) is a one-way latch: once the risk window opens, riskWindowStart can never return to 0. Therefore, any call that reaches _clampUserSums inside withdraw() is provably in a state where start == 0, causing an immediate early return. The clamp body — the part that actually adjusts values — is physically unreachable from withdraw().

Latent regression hazard: The comments above line 309 ("subtract their full contribution from the global accumulators") imply the clamp ran first. If a future developer relaxes the withdrawal gate (e.g., adds a new PREVIEW state where risk is open but withdrawal is allowed), the call becomes live — but now it adjusts per-user sums without updating the already-eagerly-reset globals. The subtraction on lines 309-310 would then use stale unclamped values against the post-reset globals, causing sumStakeTime to underflow and brick the pool's accounting.

Internal Pre-condition

  • Any staker calls withdraw() while riskWindowStart == 0. This is the only condition under which withdraw() can succeed — the call is always a no-op.

External Pre-condition

None. This is a pure code-level defect observable on any withdrawal call.

Details Attack Path

This finding has no direct attack path (dead code, not exploitable as-is). The risk is:

  1. A developer ships a future upgrade that introduces a new registry state (e.g., PREVIEW) treated as non-risk but still allowing withdrawals.

  2. A staker calls withdraw() after riskWindowStart is set.

  3. _clampUserSums runs and updates the staker's userSumStakeTime to eligibleStake * riskWindowStart.

  4. Line 309: sumStakeTime -= userSumStakeTime[msg.sender] — the global was already eagerly reset to totalEligibleStake * riskWindowStart by _markRiskWindowStart. Subtracting the newly-clamped value causes an underflow.

  5. sumStakeTime wraps to type(uint256).max, corrupting the bonus denominator for all future claimants permanently.

Impact

Current: ~600 gas wasted per withdraw() call (one SLOAD + one conditional branch). Misleading code that creates false confidence in reviewers that per-user sums are being adjusted.

Potential (upgrade regression): Complete corruption of sumStakeTime global via underflow → all bonus shares computed from a garbage denominator → some stakers receive 0 bonus, others receive amounts exceeding snapshotTotalBonus, breaking the pool's solvency invariant.

Proof of Concept

forge test --match-path "test/audit/M1*" -vvv
# [PASS] testPoC_M1_ClampNeverExecutesInWithdraw()
# [PASS] testPoC_M1_DeadCodeMasksLatentUnderflowRisk()
// test/audit/M1_ClampUserSumsDeadCode.t.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {BaseConfidencePoolTest} from "test/helpers/BaseConfidencePoolTest.sol";
​
contract M1_ClampUserSumsDeadCodePoC is BaseConfidencePoolTest {
​
function testPoC_M1_ClampNeverExecutesInWithdraw() external {
_stake(alice, 100 * ONE);
uint256 aliceSumBefore = pool.userSumStakeTime(alice);
uint256 aliceSumSqBefore = pool.userSumStakeTimeSq(alice);
​
// Advance time: sums now hold "old" timestamps.
vm.warp(block.timestamp + 10 days);
​
attackRegistry.setAgreementState(IAttackRegistry.ContractState.ATTACK_REQUESTED);
assertEq(pool.riskWindowStart(), 0, "withdraw gate requires start == 0");
​
vm.prank(alice);
pool.withdraw();
​
// Global sums correctly zeroed — but via unclamped values, not via clamp.
assertEq(pool.sumStakeTime(), 0, "global sum zeroed post-withdraw");
assertEq(pool.sumStakeTimeSq(), 0, "global sq zeroed post-withdraw");
assertEq(token.balanceOf(alice), 100 * ONE, "principal returned");
​
// The clamp condition `aliceSumBefore < 100*ONE * 0` is always false:
// _clampUserSums returned at start == 0 every time.
assertTrue(aliceSumBefore > 0, "alice had a non-zero pre-withdraw sum");
assertTrue(aliceSumSqBefore > 0, "alice had a non-zero pre-withdraw sq sum");
}
​
function testPoC_M1_DeadCodeMasksLatentUnderflowRisk() external {
_stake(alice, 100 * ONE);
_stake(bob, 50 * ONE);
​
assertEq(
pool.sumStakeTime(),
pool.userSumStakeTime(alice) + pool.userSumStakeTime(bob)
);
​
vm.prank(alice);
pool.withdraw();
​
// Correctly decremented using unclamped values (only correct because start == 0).
assertEq(pool.sumStakeTime(), pool.userSumStakeTime(bob));
}
}

Mitigation

// src/ConfidencePool.sol
function withdraw() external nonReentrant {
...
uint256 amount = eligibleStake[msg.sender];
if (amount == 0) revert InvalidAmount();
​
- _clampUserSums(msg.sender);
+ // No clamp needed: withdraw() is gated on riskWindowStart == 0.
+ // Per-user sums equal their original historical values and the global
+ // sum (not yet eagerly reset) decrements correctly without clamping.
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
...
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!