Root + Impact
Description
-
When a pool resolves as bad-faith CORRUPTED, unclaimed good-faith CORRUPTED after the 180 day window, or SURVIVED/EXPIRED with leftover bonus, the pool sends the relevant funds to recoveryAddress through claimCorrupted, sweepUnclaimedCorrupted, or sweepUnclaimedBonus. This is meant to land at a real wallet the sponsor controls.
Neither ConfidencePoolFactory.createPool nor ConfidencePool.initialize check that recoveryAddress is different from the pool's own address. The pool's address is fully predictable before deployment through the standard CREATE2 formula, the team's own tests already rely on this same predictability. If recoveryAddress ends up equal to the pool's own address, whether that happens at creation through a tooling mistake or later through a routine setRecoveryAddress call, every one of those sweep functions becomes a self transfer. The transaction succeeds, all the bookkeeping marks the pool as resolved, but the balance never actually leaves the contract.
function initialize(
address agreement_,
address stakeToken_,
address safeHarborRegistry_,
address outcomeModerator_,
uint256 expiry_,
uint256 minStake_,
address recoveryAddress_,
address owner_,
address[] calldata accounts
) external initializer {
if (agreement_ == address(0)) revert ZeroAddress();
if (stakeToken_ == address(0)) revert ZeroAddress();
if (safeHarborRegistry_ == address(0)) revert ZeroAddress();
if (outcomeModerator_ == address(0)) revert ZeroAddress();
if (owner_ == address(0)) revert ZeroAddress();
@> if (recoveryAddress_ == address(0)) revert InvalidRecoveryAddress();
function setRecoveryAddress(address newRecoveryAddress) external onlyOwner {
@> if (newRecoveryAddress == address(0)) revert InvalidRecoveryAddress();
address oldRecoveryAddress = recoveryAddress;
recoveryAddress = newRecoveryAddress;
emit RecoveryAddressUpdated(oldRecoveryAddress, newRecoveryAddress);
}
Every sweep path pays whatever recoveryAddress currently is, with no separate check on the destination:
function claimCorrupted() external nonReentrant {
if (outcome != PoolStates.Outcome.CORRUPTED) revert OutcomeNotSet();
if (goodFaith && bountyClaimed < bountyEntitlement) revert MustClaimBountyFirst();
uint256 toSweep = stakeToken.balanceOf(address(this));
if (toSweep == 0) revert NothingToSweep();
corruptedReserve = toSweep <= corruptedReserve ? corruptedReserve - toSweep : 0;
if (!goodFaith) {
bountyClaimed = bountyEntitlement;
}
if (!claimsStarted) claimsStarted = true;
@> stakeToken.safeTransfer(recoveryAddress, toSweep);
emit ClaimCorrupted(msg.sender, recoveryAddress, toSweep);
}
Risk
Likelihood:
The mistake happens two separate ways. At creation, a script that computes the pool's future address ahead of time for other purposes can reuse that same value in the wrong parameter slot. Later on, an owner managing several pools can simply mix up addresses and call setRecoveryAddress with the pool's own address by accident, this path needs no CREATE2 knowledge at all.
Nothing on chain signals the mistake happened. The sweep transaction succeeds cleanly with a normal looking event, so the mistake can sit unnoticed indefinitely until someone specifically checks whether the pool's balance actually moved.
Impact:
Every function that pays recoveryAddress breaks the same way. claimCorrupted, sweepUnclaimedCorrupted, and sweepUnclaimedBonus all turn into no-ops, so this reaches bad-faith CORRUPTED, unclaimed good-faith CORRUPTED after the 180 day window, and the healthy SURVIVED/EXPIRED outcome alike.
All the finality bookkeeping, claimsStarted, corruptedReserve, bountyClaimed, marks the pool as resolved even though the balance never left, so the mistake looks completely successful from the outside.
Funds only come back if the owner notices and fixes recoveryAddress, then someone re-triggers the sweep. If that never happens the funds stay stuck for good, the pool has no generic token rescue function.
Proof of Concept
The test below sets up the exact mistake and shows what it actually does on chain. The pool's own address is computed ahead of time with the standard CREATE2 prediction helper, then passed in as recoveryAddress at creation, which neither the factory nor initialize rejects. A real staker deposits normally, with nothing about the staking flow revealing that anything is wrong. A genuine breach then drives the registry to CORRUPTED and the moderator flags bad-faith CORRUPTED. At that point anyone calls claimCorrupted, and the call succeeds, sets every finality flag, but the balance stays exactly where it was because the destination is the pool itself. The test then confirms the outcome really is locked in by showing the moderator can no longer correct it.
function testSelfReferentialRecoveryAddressPermanentlyLocksBadFaithSweep() external {
uint256 expiry = block.timestamp + 31 days;
bytes32 salt = keccak256(abi.encode(address(agreementContract), uint256(0)));
address predictedPool = Clones.predictDeterministicAddress(address(poolImpl), salt, address(factory));
vm.prank(agreementOwner);
address pool = factory.createPool(
address(agreementContract), address(token), expiry, ONE, predictedPool, _scope()
);
assertEq(pool, predictedPool);
token.mint(alice, 100 * ONE);
vm.startPrank(alice);
token.approve(pool, 100 * ONE);
ConfidencePool(pool).stake(100 * ONE);
vm.stopPrank();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.UNDER_ATTACK);
ConfidencePool(pool).pokeRiskWindow();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
vm.prank(moderator);
ConfidencePool(pool).flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
uint256 poolBalanceBefore = token.balanceOf(pool);
vm.prank(randomFrontRunner);
ConfidencePool(pool).claimCorrupted();
assertEq(token.balanceOf(pool), poolBalanceBefore);
assertEq(ConfidencePool(pool).claimsStarted(), true);
assertEq(ConfidencePool(pool).corruptedReserve(), 0);
vm.prank(moderator);
vm.expectRevert();
ConfidencePool(pool).flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
assertEq(token.balanceOf(pool), 100 * ONE);
}
The three assertions right after the sweep call are the actual proof, the balance is unchanged, claimsStarted is true, and corruptedReserve reads zero as if the money had genuinely left. The final revert on the moderator's attempt to correct the outcome shows there is no way back out through the normal resolution flow either.
The same self transfer no-op was confirmed separately for sweepUnclaimedCorrupted (good-faith CORRUPTED, attacker never claims within the 180 day window) and for sweepUnclaimedBonus (healthy SURVIVED outcome, leftover bonus donation), and for the case where the self reference is introduced later through a normal setRecoveryAddress call instead of at creation. In every case the mistake is fully recoverable if the owner notices, calls setRecoveryAddress with a working address, and the sweep is triggered again, since none of these functions use a one shot already swept flag, they all re-derive the amount from the live balance.
Recommended Mitigation
The pool's own address is always known inside the contract through address(this), so there is no reason either entry point should accept it as a valid recoveryAddress. Reject it in both places it can be set, at creation and on any later update:
function initialize(
...
address recoveryAddress_,
...
) external initializer {
...
if (recoveryAddress_ == address(0)) revert InvalidRecoveryAddress();
+ if (recoveryAddress_ == address(this)) revert InvalidRecoveryAddress();
function setRecoveryAddress(address newRecoveryAddress) external onlyOwner {
if (newRecoveryAddress == address(0)) revert InvalidRecoveryAddress();
+ if (newRecoveryAddress == address(this)) revert InvalidRecoveryAddress();
address oldRecoveryAddress = recoveryAddress;
recoveryAddress = newRecoveryAddress;
emit RecoveryAddressUpdated(oldRecoveryAddress, newRecoveryAddress);
}
Both checks are one line, reuse the existing revert, and cost nothing at runtime beyond a single comparison, so there is no tradeoff against applying this everywhere recoveryAddress can be written.
