Root + Impact
Description
The withdraw() function allows withdrawals while the registry state is ATTACK_REQUESTED, provided riskWindowStart is still 0.
Although ATTACK_REQUESTED indicates that a potential attack has already been reported, withdrawals are not immediately disabled. Instead, the pool only freezes withdrawals after riskWindowStart is set, which appears to occur in a separate transaction or execution path.
This creates a temporary window where the registry has already signaled risk, but users can still withdraw their entire stake before the pool records the risk locally.
Risk
Likelihood:
Impact:
-
Impact
Users or automated bots monitoring the registry can immediately withdraw their funds as soon as the state changes to ATTACK_REQUESTED, before riskWindowStart is updated.
-
This can trigger a stake run, allowing a large portion of the pool's liquidity to leave just before the pool is expected to cover potential claims. As a result, the insurance/confidence pool may become underfunded or unable to satisfy legitimate payouts after the attack is confirmed.
Proof of Concept
contract AttackRequestedWithdrawPoC is Test {
MockAttackRegistry registry;
MockERC20 token;
VulnerablePool vulnerable;
FixedPool fixedPool;
address staker = makeAddr("staker");
uint256 constant STAKE_AMOUNT = 1_000 ether;
function setUp() public {
registry = new MockAttackRegistry();
token = new MockERC20();
vulnerable = new VulnerablePool(registry, token);
fixedPool = new FixedPool(registry, token);
vulnerable.depositForTest(staker, STAKE_AMOUNT);
fixedPool.depositForTest(staker, STAKE_AMOUNT);
}
function test_PoC_VulnerablePool_AllowsWithdrawDuringAttackRequested() public {
registry.setState(IAttackRegistry.ContractState.ATTACK_REQUESTED);
assertEq(vulnerable.riskWindowStart(), 0);
uint256 balBefore = token.balanceOf(staker);
vm.prank(staker);
vulnerable.withdraw();
uint256 balAfter = token.balanceOf(staker);
assertEq(balAfter - balBefore, STAKE_AMOUNT);
assertEq(vulnerable.eligibleStake(staker), 0);
assertEq(vulnerable.totalEligibleStake(), 0);
assertEq(token.balanceOf(address(vulnerable)), 0);
}
function test_PoC_FixedPool_BlocksWithdrawDuringAttackRequested() public {
registry.setState(IAttackRegistry.ContractState.ATTACK_REQUESTED);
assertEq(fixedPool.riskWindowStart(), 0);
vm.prank(staker);
vm.expectRevert(WithdrawsDisabled.selector);
fixedPool.withdraw();
assertEq(fixedPool.eligibleStake(staker), STAKE_AMOUNT);
assertEq(token.balanceOf(address(fixedPool)), STAKE_AMOUNT);
}
function test_PoC_FixedPool_AllowsWithdrawInSafeState() public {
registry.setState(IAttackRegistry.ContractState.NEW_DEPLOYMENT);
vm.prank(staker);
fixedPool.withdraw();
assertEq(fixedPool.eligibleStake(staker), 0);
assertEq(token.balanceOf(staker), STAKE_AMOUNT);
}
}
Recommended Mitigation
Treat ATTACK_REQUESTED as a withdrawal-blocking state by removing it from the list of states that permit withdrawals.
If withdrawals must remain available during ATTACK_REQUESTED for business reasons (e.g., to account for false-positive reports), ensure that riskWindowStart is set atomically in the same transaction that transitions the registry into ATTACK_REQUESTED.
This removes the observable gap that allows users to exit before the pool is frozen.
- remove this code
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
// `riskWindowStart` is the pool's one-way record that risk has materialised;
// gate on it so an upstream registry rewind cannot re-open withdrawals.
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);
// 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];
eligibleStake[msg.sender] = 0;
userSumStakeTime[msg.sender] = 0;
userSumStakeTimeSq[msg.sender] = 0;
totalEligibleStake -= amount;
stakeToken.safeTransfer(msg.sender, amount);
emit Withdrawn(msg.sender, amount);
+ add this code
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
// `riskWindowStart` is the pool's one-way record that risk has materialised;
// gate on it so an upstream registry rewind cannot re-open withdrawals.
//
// FIX: `ATTACK_REQUESTED` is no longer treated as a safe state. It signals
// that an attack has been flagged against the protected protocol and is
// pending confirmation — allowing withdrawals here would let stakers front-run
// `riskWindowStart` being set and drain the pool exactly when solvency is needed.
// Only NOT_DEPLOYED and NEW_DEPLOYMENT are genuinely risk-free states.
if (
riskWindowStart != 0
|| (state != IAttackRegistry.ContractState.NOT_DEPLOYED
&& state != IAttackRegistry.ContractState.NEW_DEPLOYMENT)
) {
revert WithdrawsDisabled();
}
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];
eligibleStake[msg.sender] = 0;
userSumStakeTime[msg.sender] = 0;
userSumStakeTimeSq[msg.sender] = 0;
totalEligibleStake -= amount;
stakeToken.safeTransfer(msg.sender, amount);
emit Withdrawn(msg.sender, amount);
}
