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

recoveryAddress can be set to the pool itself, allowing corrupted finality without moving funds

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: claimCorrupted() should close the correction window only after value has actually moved out of the pool to recoveryAddress.

  • I observed that the pool owner can set recoveryAddress to the pool itself. When claimCorrupted() later transfers tokens to recoveryAddress, the ERC20 transfer becomes a self-transfer. The pool balance does not decrease, but claimsStarted is set and corruptedReserve is reduced to zero. This closes the moderator correction window even though no external value movement occurred.

// src/ConfidencePool.sol
​
function setRecoveryAddress(address newRecoveryAddress) external onlyOwner {
@> if (newRecoveryAddress == address(0)) revert ZeroAddress();
​
@> recoveryAddress = newRecoveryAddress;
​
emit RecoveryAddressUpdated(newRecoveryAddress);
}
​
function claimCorrupted() external nonReentrant {
if (outcome != PoolStates.Outcome.CORRUPTED) revert InvalidOutcome();
​
uint256 toSweep = stakeToken.balanceOf(address(this));
if (toSweep == 0) revert NothingToClaim();
​
@> corruptedReserve = toSweep <= corruptedReserve ? corruptedReserve - toSweep : 0;
@> claimsStarted = true;
​
@> stakeToken.safeTransfer(recoveryAddress, toSweep);
​
emit ClaimCorrupted(recoveryAddress, toSweep);
}

The missing validation is that recoveryAddress cannot be address(this). With a standard ERC20, transferring from the pool to itself creates no net balance change.

Risk

Likelihood:

  • This occurs when the pool owner sets recoveryAddress to the pool address and the pool later resolves as bad-faith CORRUPTED.

  • The path uses normal protocol functions and a standard ERC20. No fee-on-transfer behavior, reentrancy, token callback, or rebasing token is required.

Impact:

  • claimCorrupted() can set claimsStarted = true and zero corruptedReserve without any funds leaving the pool.

  • The moderator correction window can be closed even though the stated reason for closing finality, irreversible value movement, did not occur.

Proof of Concept

Create this file:

cat > test/unit/RecoverySelfFinalityPoC.t.sol <<'EOF'
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {BaseConfidencePoolTest} from "test/helpers/BaseConfidencePoolTest.sol";
​
contract RecoverySelfFinalityPoC is BaseConfidencePoolTest {
function testPoC_selfRecoveryClosesCorrectionWindowWithoutMovingFunds() external {
_stake(alice, 100 * ONE);
_contributeBonus(carol, 50 * ONE);
​
// Pool owner can set recoveryAddress to the pool itself.
pool.setRecoveryAddress(address(pool));
assertEq(pool.recoveryAddress(), address(pool));
​
// Registry is terminal CORRUPTED and moderator flags bad-faith CORRUPTED.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
​
assertEq(uint256(pool.outcome()), uint256(PoolStates.Outcome.CORRUPTED));
assertEq(pool.corruptedReserve(), 150 * ONE);
assertFalse(pool.claimsStarted());
​
uint256 poolBalanceBefore = token.balanceOf(address(pool));
assertEq(poolBalanceBefore, 150 * ONE);
​
// claimCorrupted() transfers to the pool itself.
// Accounting/finality changes, but the ERC20 balance is unchanged.
vm.prank(dave);
pool.claimCorrupted();
​
assertTrue(pool.claimsStarted(), "correction window closed");
assertEq(pool.corruptedReserve(), 0, "reserve marked swept");
assertEq(token.balanceOf(address(pool)), poolBalanceBefore, "self-transfer moved no funds");
assertEq(token.balanceOf(recovery), 0, "real recovery address received nothing");
​
// Moderator can no longer correct even though no external value moved.
vm.prank(moderator);
vm.expectRevert(IConfidencePool.OutcomeAlreadySet.selector);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
}
​
function testPoC_fundsOnlyMoveAfterOwnerChangesRecoveryAwayFromPool() external {
_stake(alice, 100 * ONE);
_contributeBonus(carol, 50 * ONE);
​
pool.setRecoveryAddress(address(pool));
​
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
​
uint256 poolBalanceBefore = token.balanceOf(address(pool));
​
pool.claimCorrupted();
​
assertEq(token.balanceOf(address(pool)), poolBalanceBefore);
assertTrue(pool.claimsStarted());
​
// Only after owner changes recoveryAddress can the funds actually leave.
pool.setRecoveryAddress(recovery);
​
uint256 recoveryBefore = token.balanceOf(recovery);
​
pool.claimCorrupted();
​
assertEq(token.balanceOf(recovery) - recoveryBefore, 150 * ONE);
assertEq(token.balanceOf(address(pool)), 0);
}
}
EOF

Run:

forge test --match-path test/unit/RecoverySelfFinalityPoC.t.sol -vvv

Observed output:

Ran 2 tests for test/unit/RecoverySelfFinalityPoC.t.sol:RecoverySelfFinalityPoC
[PASS] testPoC_fundsOnlyMoveAfterOwnerChangesRecoveryAwayFromPool() (gas: 536540)
[PASS] testPoC_selfRecoveryClosesCorrectionWindowWithoutMovingFunds() (gas: 535953)
Suite result: ok. 2 passed; 0 failed; 0 skipped

The PoC proves that:

  1. recoveryAddress can be set to address(pool).

  2. claimCorrupted() sets claimsStarted = true.

  3. corruptedReserve is marked as swept.

  4. The pool token balance is unchanged because the transfer was a self-transfer.

  5. Moderator correction is blocked even though no external value moved.

Recommended Mitigation

Disallow the pool itself as a recovery address during initialization and updates.

+ error InvalidRecoveryAddress();
​
function setRecoveryAddress(address newRecoveryAddress) external onlyOwner {
- if (newRecoveryAddress == address(0)) revert ZeroAddress();
+ if (newRecoveryAddress == address(0) || newRecoveryAddress == address(this)) {
+ revert InvalidRecoveryAddress();
+ }
​
recoveryAddress = newRecoveryAddress;
​
emit RecoveryAddressUpdated(newRecoveryAddress);
}

Apply the same validation in initialize() before storing the initial recoveryAddress.

As defense in depth, recovery/sweep functions can also check that the pool balance decreased by the expected amount after transfer, or only update finality/accounting after confirming that funds actually left the pool.

Support

FAQs

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

Give us feedback!