Description
-
ConfidencePool.withdraw() lets a staker exit only before the pool has observed active risk. Withdrawals are allowed in the pre-risk registry states and are permanently disabled once riskWindowStart becomes nonzero.
-
The function still calls _clampUserSums(msg.sender) after the withdraw gate. This call is redundant because reaching that line already proves riskWindowStart == 0. _clampUserSums() immediately returns when riskWindowStart is zero, so it cannot update user accounting on any successful withdraw path.
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
@> 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();
@> _clampUserSums(msg.sender);
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
...
}
function _clampUserSums(address u) internal {
uint256 start = riskWindowStart;
uint256 stake_ = eligibleStake[u];
@> if (start == 0 || stake_ == 0) return;
if (userSumStakeTime[u] < stake_ * start) {
userSumStakeTime[u] = stake_ * start;
userSumStakeTimeSq[u] = stake_ * start * start;
}
}
Risk
Likelihood:
-
The redundant call is reached on every successful withdrawal.
-
Successful withdrawals are only possible while riskWindowStart == 0, so _clampUserSums() always returns at its first guard.
-
After active risk is observed, riskWindowStart becomes nonzero and withdraw() reverts before the clamp can be useful.
Impact:
-
No funds are at risk. The call cannot change accounting on the successful withdraw path.
-
Each successful withdrawal pays a small amount of unnecessary gas for the internal call and initial storage reads.
-
The extra call makes the withdraw accounting harder to reason about because it suggests user sums may need clamping even though the preceding gate rules that case out.
Proof of Concept
function testPOCWithdrawClampUserSumsIsRedundant() external {
uint256 aliceStakeTs = vm.getBlockTimestamp();
_stake(alice, 100 * ONE);
vm.warp(aliceStakeTs + 1 days);
uint256 bobStakeTs = vm.getBlockTimestamp();
_stake(bob, 50 * ONE);
attackRegistry.setAgreementState(IAttackRegistry.ContractState.ATTACK_REQUESTED);
assertEq(pool.riskWindowStart(), 0, "withdraw-eligible states have no risk window");
assertEq(pool.userSumStakeTime(alice), 100 * ONE * aliceStakeTs, "alice has unclamped pre-risk sums");
uint256 aliceBefore = token.balanceOf(alice);
_withdraw(alice);
assertEq(token.balanceOf(alice) - aliceBefore, 100 * ONE, "alice receives principal");
assertEq(pool.riskWindowStart(), 0, "withdraw did not and could not open the risk window");
assertEq(pool.eligibleStake(alice), 0, "alice stake removed");
assertEq(pool.userSumStakeTime(alice), 0, "alice sums cleared after subtraction");
assertEq(pool.sumStakeTime(), 50 * ONE * bobStakeTs, "global sum only subtracts alice's original value");
attackRegistry.setAgreementState(IAttackRegistry.ContractState.UNDER_ATTACK);
pool.pokeRiskWindow();
vm.prank(bob);
vm.expectRevert(IConfidencePool.WithdrawsDisabled.selector);
pool.withdraw();
}
Recommended Mitigation
Remove the unreachable accounting clamp from withdraw().
uint256 amount = eligibleStake[msg.sender];
if (amount == 0) revert InvalidAmount();
-_clampUserSums(msg.sender);
// Withdrawing forfeits the caller's bonus claim: subtract their full contribution from
// the global accumulators so honest stakers' shares aren't diluted by the exiter's
// forfeited weight.
sumStakeTime -= userSumStakeTime[msg.sender];
sumStakeTimeSq -= userSumStakeTimeSq[msg.sender];
