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

`sweepUnclaimedBonus` permanently diverts the whitehat's bonus entitlement to `recoveryAddress`

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: when a pool resolves SURVIVED/EXPIRED with riskWindowStart == 0 ("no observed risk"; the registry skipped the active-risk states entirely), _bonusShare owes the bonus to no staker, so sweepUnclaimedBonus() is permissionlessly allowed to recover the unowed bonus to recoveryAddress. Separately, because flagOutcome only closes its re-flag window once claimsStarted is true, and sweepUnclaimedBonus() deliberately does not set claimsStarted, a moderator can still re-flag the pool as good-faith CORRUPTED afterward if the agreement is genuinely breached later, naming the real whitehat attacker and re-snapshotting their bountyEntitlement off the live totalBonus/totalEligibleStake.

  • The problem: sweepUnclaimedBonus() physically transfers the bonus out and zeroes totalBonus when it decides the bonus is currently unowed, but it does this irreversibly, even though it intentionally keeps the re-flag window open for exactly the scenario where that bonus becomes owed to someone else moments later. When the moderator subsequently re-flags good-faith CORRUPTED, flagOutcome re-snapshots snapshotTotalBonus = totalBonus, which is now 0. The named whitehat's bountyEntitlement silently excludes the bonus, which is unrecoverably sitting at recoveryAddress instead of following the attacker's bounty as the CORRUPTED/good-faith payout rule intends.

function sweepUnclaimedBonus() external nonReentrant {
if (outcome != PoolStates.Outcome.SURVIVED && outcome != PoolStates.Outcome.EXPIRED) {
revert OutcomeNotEligibleForSweep();
}
uint256 reserved;
if (totalEligibleStake != 0) {
reserved = totalEligibleStake;
if (riskWindowStart != 0) {
reserved += snapshotTotalBonus - claimedBonus;
}
}
uint256 freeBalance = stakeToken.balanceOf(address(this));
uint256 amount = freeBalance > reserved ? freeBalance - reserved : 0;
if (amount == 0) revert NothingToSweep();
if (totalEligibleStake == 0 || riskWindowStart == 0) {
//> totalBonus -= amount <= totalBonus ? amount : totalBonus; // @> bonus zeroed out permanently
}
// @> Intentionally does NOT set claimsStarted -- re-flag window (flagOutcome) stays open
//> stakeToken.safeTransfer(recoveryAddress, amount); // @> bonus irreversibly leaves the pool
emit BonusSwept(msg.sender, recoveryAddress, amount);
}
function flagOutcome(PoolStates.Outcome newOutcome, bool goodFaith_, address attacker_) external onlyModerator {
// @> re-flag still allowed here because sweepUnclaimedBonus never set claimsStarted
if (outcome != PoolStates.Outcome.UNRESOLVED && claimsStarted) revert OutcomeAlreadySet();
...
outcome = newOutcome;
goodFaith = goodFaith_;
attacker = attacker_;
snapshotTotalStaked = totalEligibleStake;
//> snapshotTotalBonus = totalBonus; // @> re-snapshots off the ALREADY-SWEPT (zeroed) totalBonus
...
bountyEntitlement = willBeGoodFaithCorrupted ? snapshotTotalStaked + snapshotTotalBonus : 0;
// @> bountyEntitlement excludes the bonus that already left for recoveryAddress
}

Risk

Likelihood:

  • Occurs whenever a pool resolves SURVIVED/EXPIRED with riskWindowStart == 0 (registry transitioned straight from pre-attack staging to a terminal state, e.g. NEW_DEPLOYMENT PRODUCTION) a state the protocol's own docs treat as a normal, expected edge case, not a rare fault.

  • sweepUnclaimedBonus() is permissionless and requires no staker action first, so any observer (a bot, a competing sponsor, or recoveryAddress's own owner) will sweep the bonus as soon as it becomes sweepable, well before the pool's underlying agreement's real fate is known.

  • The re-flag path is a documented, intended moderator workflow (correcting an outcome, naming a whitehat after the fact when a real attack surfaces post-"resolution"); it is not a contrived edge case; it is the exact mechanism sweepUnclaimedBonus was written to stay compatible with.

Impact:

  • The bonus (the sponsor-funded incentive specifically meant to reward whoever ends up entitled to it under a CORRUPTED outcome) settles at recoveryAddress controlled by the sponsor/pool owner instead of the whitehat attacker the moderator names in good faith.

  • Breaks the documented CORRUPTED/good-faith payout guarantee that the full pool (stake + bonus) is owed to the named attacker, silently reducing it to stake-only with no on-chain signal that anything went wrong (the transaction succeeds, bountyEntitlement is simply smaller than expected).

Proof of Concept

Add this file to the mock folder

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {IBattleChainSafeHarborRegistry} from "@battlechain/interface/IBattleChainSafeHarborRegistry.sol";
import {IAgreement} from "@battlechain/interface/IAgreement.sol";
​
/// @notice Minimal mock of the on-chain attack registry. State is set directly by the test so we
/// can drive the exact registry-state sequence the PoC needs. Does NOT inherit IAttackRegistry —
/// matching selectors is sufficient for an address-cast call site, and inheriting the full
/// interface would force struct-typed stubs that clash under via_ir.
contract MockAttackRegistry {
mapping(address => IAttackRegistry.ContractState) public state;
​
function setState(address agreement, IAttackRegistry.ContractState s) external {
state[agreement] = s;
}
​
function getAgreementState(address agreement) external view returns (IAttackRegistry.ContractState) {
return state[agreement];
}
}
​
/// @notice Minimal mock of the Safe Harbor registry singleton. Does NOT inherit
/// IBattleChainSafeHarborRegistry — matching selectors is sufficient for an address-cast call site.
contract MockSafeHarborRegistry {
address public attackRegistry;
mapping(address => bool) public validAgreements;
​
constructor(address attackRegistry_) {
attackRegistry = attackRegistry_;
}
​
function setAgreementValid(address agreement, bool valid_) external {
validAgreements[agreement] = valid_;
}
​
function getAttackRegistry() external view returns (address) {
return attackRegistry;
}
​
function isAgreementValid(address agreement) external view returns (bool) {
return validAgreements[agreement];
}
}
​
/// @notice Minimal mock of the underlying Safe Harbor agreement. Every address is in-scope so the
/// PoC doesn't need to fuss with scope bookkeeping. Does NOT inherit IAgreement — matching
/// selectors is sufficient for an address-cast call site.
contract MockAgreement {
function isContractInScope(address) external pure returns (bool) {
return true;
}
}
​
/// @notice Plain mintable ERC20 used as the pool's stake/bonus token.
contract MockStakeToken is ERC20 {
constructor() ERC20("Mock Stake Token", "MST") {}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
​
​

Add this POC to the unit test folder.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {Test, console2} from "forge-std/Test.sol";
import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
​
import {ConfidencePool} from "src/ConfidencePool.sol";
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
​
import {MockAttackRegistry, MockSafeHarborRegistry, MockAgreement, MockStakeToken} from "../mocks/PoCMocks.sol";
​
/// @title PoC: sweepUnclaimedBonus permanently diverts the whitehat's bounty
/// @notice Demonstrates the finding:
///
/// 1. The registry skips the active-risk states entirely (NEW_DEPLOYMENT -> PRODUCTION), so
/// `riskWindowStart` is never sealed ("no observed risk").
/// 2. `claimExpired()` auto-resolves the pool to SURVIVED off the observed PRODUCTION state.
/// 3. Because `riskWindowStart == 0`, nobody is owed any bonus share, so anyone can call
/// `sweepUnclaimedBonus()` to send the *entire* bonus pool to `recoveryAddress`. This
/// intentionally does NOT set `claimsStarted`, per the contract's own comment, so the
/// moderator's pre-claim re-flag window stays open.
/// 4. The agreement is later actually attacked and the registry moves to CORRUPTED. The
/// moderator (correctly, per the docs) re-flags the pool as good-faith CORRUPTED naming the
/// whitehat attacker.
/// 5. `flagOutcome` re-snapshots `snapshotTotalBonus = totalBonus`, but `totalBonus` was already
/// zeroed out (and the tokens physically moved to `recoveryAddress`) by step 3. The
/// attacker's `bountyEntitlement` therefore excludes the bonus entirely, and
/// `claimAttackerBounty()` only pays out stake, not stake + bonus. The bonus is
/// unrecoverably sitting at `recoveryAddress` instead.
contract BonusDivertedOnReflagTest is Test {
ConfidencePool internal pool;
MockAttackRegistry internal attackRegistry;
MockSafeHarborRegistry internal safeHarborRegistry;
MockAgreement internal agreement;
MockStakeToken internal token;
​
address internal owner = makeAddr("owner");
address internal moderator = makeAddr("moderator");
address internal recoveryAddress = makeAddr("recoveryAddress");
address internal staker = makeAddr("staker");
address internal whitehat = makeAddr("whitehat");
address internal randomCaller = makeAddr("randomCaller");
​
address internal scopedAccount = makeAddr("scopedAccount");
​
uint256 internal constant STAKE_AMOUNT = 100 ether;
uint256 internal constant BONUS_AMOUNT = 40 ether;
uint256 internal expiry;
​
function setUp() public {
attackRegistry = new MockAttackRegistry();
agreement = new MockAgreement();
safeHarborRegistry = new MockSafeHarborRegistry(address(attackRegistry));
safeHarborRegistry.setAgreementValid(address(agreement), true);
token = new MockStakeToken();
​
// Registry starts in pre-attack staging.
attackRegistry.setState(address(agreement), IAttackRegistry.ContractState.NEW_DEPLOYMENT);
​
ConfidencePool implementation = new ConfidencePool();
pool = ConfidencePool(Clones.clone(address(implementation)));
​
expiry = block.timestamp + 31 days;
​
address[] memory accounts = new address[](1);
accounts[0] = scopedAccount;
​
pool.initialize(
address(agreement),
address(token),
address(safeHarborRegistry),
moderator,
expiry,
1 ether, // minStake
recoveryAddress,
owner,
accounts
);
​
token.mint(staker, STAKE_AMOUNT);
token.mint(owner, BONUS_AMOUNT); // owner acts as bonus contributor for simplicity
​
vm.prank(staker);
token.approve(address(pool), STAKE_AMOUNT);
vm.prank(owner);
token.approve(address(pool), BONUS_AMOUNT);
​
vm.prank(staker);
pool.stake(STAKE_AMOUNT);
​
vm.prank(owner);
pool.contributeBonus(BONUS_AMOUNT);
}
​
function test_sweepUnclaimedBonus_divertsWhitehatBountyOnReflag() public {
// --- Step 1: registry skips active-risk states entirely and jumps straight to
// PRODUCTION. `riskWindowStart` never seals -- this is the documented "no observed risk"
// scenario.
attackRegistry.setState(address(agreement), IAttackRegistry.ContractState.PRODUCTION);
assertEq(pool.riskWindowStart(), 0, "riskWindowStart should never have sealed");
​
// --- Step 2: moderator flags SURVIVED off the observed PRODUCTION state via the
// moderator-driven `flagOutcome` path (not the permissionless `claimExpired`
// auto-resolution). `flagOutcome` does NOT set `claimsStarted` by itself, matching its
// own documented "re-flag allowed pre-claim" window.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
assertEq(uint256(pool.outcome()), uint256(PoolStates.Outcome.SURVIVED));
assertEq(pool.snapshotTotalBonus(), BONUS_AMOUNT, "bonus snapshot should hold the full bonus");
assertFalse(pool.claimsStarted(), "no claim entrypoint has run yet");
assertEq(token.balanceOf(address(pool)), STAKE_AMOUNT + BONUS_AMOUNT);
​
// --- Step 3: because riskWindowStart == 0, nobody is owed any bonus share, so the whole
// bonus is "sweepable". Anyone can call this permissionlessly.
vm.prank(randomCaller);
pool.sweepUnclaimedBonus();
​
assertEq(token.balanceOf(recoveryAddress), BONUS_AMOUNT, "entire bonus already sent to recoveryAddress");
assertEq(pool.totalBonus(), 0, "totalBonus zeroed out by the sweep");
assertFalse(pool.claimsStarted(), "sweep intentionally leaves claimsStarted false");
assertEq(token.balanceOf(address(pool)), STAKE_AMOUNT, "only stake principal remains in the pool");
​
// --- Step 4: the agreement is genuinely attacked after the fact. The registry moves to
// CORRUPTED, and the moderator does exactly what the docs say a moderator should do: names
// the whitehat attacker in a good-faith CORRUPTED re-flag. This is allowed because
// claimsStarted is still false.
attackRegistry.setState(address(agreement), IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, whitehat);
​
assertEq(uint256(pool.outcome()), uint256(PoolStates.Outcome.CORRUPTED));
assertTrue(pool.goodFaith());
assertEq(pool.attacker(), whitehat);
​
// --- Step 5: the re-snapshot at flagOutcome captured totalBonus AFTER the sweep, i.e.
// zero. The whitehat's entitlement excludes the bonus that DESIGN.md says should have
// gone to them (or at worst waited for the moderator, never been auto-swept to
// recoveryAddress while a real CORRUPTED outcome was still possible).
assertEq(
pool.bountyEntitlement(),
STAKE_AMOUNT,
"bountyEntitlement excludes the bonus - it was already swept away"
);
​
vm.prank(whitehat);
pool.claimAttackerBounty();
​
assertEq(token.balanceOf(whitehat), STAKE_AMOUNT, "whitehat only recovers stake, never the bonus");
assertEq(
token.balanceOf(recoveryAddress),
BONUS_AMOUNT,
"the full bonus stays stuck at recoveryAddress instead of the whitehat bounty"
);
​
console2.log("whitehat payout: ", token.balanceOf(whitehat));
console2.log("recoveryAddress payout: ", token.balanceOf(recoveryAddress));
console2.log("bonus that should have followed the CORRUPTED bounty:", BONUS_AMOUNT);
}
}

Recommended Mitigation

function sweepUnclaimedBonus() external nonReentrant {
if (outcome != PoolStates.Outcome.SURVIVED && outcome != PoolStates.Outcome.EXPIRED) {
revert OutcomeNotEligibleForSweep();
}
uint256 reserved;
if (totalEligibleStake != 0) {
reserved = totalEligibleStake;
if (riskWindowStart != 0) {
reserved += snapshotTotalBonus - claimedBonus;
}
}
uint256 freeBalance = stakeToken.balanceOf(address(this));
uint256 amount = freeBalance > reserved ? freeBalance - reserved : 0;
if (amount == 0) revert NothingToSweep();
if (totalEligibleStake == 0 || riskWindowStart == 0) {
totalBonus -= amount <= totalBonus ? amount : totalBonus;
}
- // Intentionally does NOT set claimsStarted. A direct-transfer donation of as little as 1
- // wei would otherwise let anyone flip the flag post-flagOutcome and block the moderator's
- // documented pre-claim re-flag window. Genuine reliance only comes from claim entrypoints.
+ // Moving real bonus funds out of the pool IS genuine reliance -- it must close the
+ // re-flag window the same way every other value-moving entrypoint does, otherwise a
+ // later good-faith CORRUPTED re-flag re-snapshots off an already-emptied totalBonus and
+ // silently strips the bonus from the whitehat's bountyEntitlement.
+ claimsStarted = true;
stakeToken.safeTransfer(recoveryAddress, amount);
emit BonusSwept(msg.sender, recoveryAddress, amount);
}

This closes the re-flag window as soon as real value moves, matching every other claim entrypoint. If a 1-wei donation griefing a moderator's re-flag is a concern worth preserving against, prefer gating sweepUnclaimedBonus() behind a minimum post-resolution delay (mirroring MODERATOR_CORRUPTED_GRACE) long enough for a moderator to act on a freshly observed CORRUPTED state first, rather than leaving the sweep able to permanently strip funds out from under a still-open re-flag window.

Support

FAQs

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

Give us feedback!