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

Dead _clampUserSums Call In withdraw() Creates Latent Accumulator Corruption Risk

Author Revealed upon completion

Description

withdraw() calls _clampUserSums(msg.sender) at L305. However, the gate at L293-300 ensures that execution only reaches L305 when riskWindowStart == 0. The _clampUserSums function immediately returns when start == 0 (L680), making it an unconditional no-op in this code path.

The subsequent lines (L309-310) subtract the user's raw (unclamped) per-user sums from the global accumulators, which are also raw when riskWindowStart == 0. This is arithmetically correct today.

// ConfidencePool.sol L288-319
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
// @> This gate guarantees riskWindowStart == 0 when execution continues
if (
riskWindowStart != 0
|| (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();
​
// @> Always a no-op: riskWindowStart == 0 at this point
_clampUserSums(msg.sender);
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
// ...
}

Risk

Likelihood:

  • This is a code quality issue. The dead call is harmless today because the gate prevents reaching _clampUserSums when riskWindowStart != 0.

Impact:

  • The dead call is misleading: it suggests clamping occurs during withdrawal, which it never does. If the gate is ever relaxed in a future upgrade, _clampUserSums would update per-user sums to stake * riskWindowStart while the global was already eagerly reset in _markRiskWindowStart. The subtraction at L309-310 would then remove the clamped value from a global that doesn't account for it, corrupting sumStakeTime and sumStakeTimeSq for all remaining stakers and breaking the k=2 bonus distribution formula.

Proof of Concept

contract Finding4_DeadClampInWithdraw is BaseConfidencePoolTest {
​
function test_finding4_clampIsAlwaysNoOpInWithdraw() external {
_stake(alice, 100 * ONE);
assertEq(pool.riskWindowStart(), 0, "pre: riskWindowStart == 0");
​
vm.prank(alice);
pool.withdraw();
​
assertEq(pool.totalEligibleStake(), 0);
assertEq(pool.sumStakeTime(), 0);
assertEq(pool.sumStakeTimeSq(), 0);
}
​
function test_finding4_withdrawBlockedWhenRiskWindowOpen() external {
_stake(alice, 100 * ONE);
attackRegistry.setAgreementState(IAttackRegistry.ContractState.UNDER_ATTACK);
​
vm.prank(alice);
vm.expectRevert(IConfidencePool.WithdrawsDisabled.selector);
pool.withdraw();
​
assertGt(pool.riskWindowStart(), 0);
}
}

Run: forge test --match-contract Finding3_DeadClampInWithdraw -vvv

Terminal Output:

Ran 2 tests for test/audit/AuditPoC.t.sol:Finding3_DeadClampInWithdraw
[PASS] test_finding3_clampIsAlwaysNoOpInWithdraw() (gas: 287125)
[PASS] test_finding3_withdrawBlockedWhenRiskWindowOpen() (gas: 322117)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 1.77ms (547.01µs CPU time)

Recommended Mitigation

- _clampUserSums(msg.sender);
+ // _clampUserSums is intentionally omitted: the gate above guarantees
+ // riskWindowStart == 0, so no per-user clamp is needed. The global
+ // subtraction below uses raw (unclamped) sums — consistent with the
+ // global accumulators which have not been eagerly reset.
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];

To prevent latent accumulator corruption risks, the dead _clampUserSums call should be completely removed from withdraw(). A detailed inline comment is added to explain that the preceding execution gate guarantees riskWindowStart == 0, making clamping unnecessary and safely relying on the raw unclamped global accumulators.

Updates

Lead Judging Commences

inallhonesty Lead Judge
18 days ago
inallhonesty Lead Judge 2 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

QA / Informational / Gas

inallhonesty Lead Judge 2 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

QA / Informational / Gas

Support

FAQs

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

Give us feedback!