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

sweepUnclaimedBonus reserves against a non-final outcome, letting anyone permanently divert the whole bonus pot to recoveryAddress and short-change the named whitehat

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: sweepUnclaimedBonus() is the cleanup function. protocol-readme.md:43 says what it recovers: "k=2 rounding dust, non-claimers' forfeited shares, post-resolution donations" — leftovers. It calculates what still belongs to people (reserved) and sends the excess to recoveryAddress.

  • The bug: it calculates reserved from an outcome that can still change. So it sends the entire bonus pot to recoveryAddress while the moderator is still allowed to correct the outcome. When they do correct it, the money is already gone.

// ConfidencePool.sol
function sweepUnclaimedBonus() external nonReentrant {
if (outcome != PoolStates.Outcome.SURVIVED && outcome != PoolStates.Outcome.EXPIRED) {
revert OutcomeNotEligibleForSweep();
}
​
uint256 reserved;
if (totalEligibleStake != 0) {
reserved = totalEligibleStake;
// @> The bonus is only held back if a risk window was seen. With riskWindowStart == 0
// @> this never runs, so `reserved` is principal only and the WHOLE bonus is sweepable.
if (riskWindowStart != 0) {
reserved += snapshotTotalBonus - claimedBonus;
}
}
​
uint256 freeBalance = stakeToken.balanceOf(address(this));
uint256 amount = freeBalance > reserved ? freeBalance - reserved : 0;
if (amount == 0) revert NothingToSweep();
...
// @> Real money moves, but `claimsStarted` is deliberately NOT set. So the moderator can
// @> still change the outcome afterwards -- and redirect a pot that has already left.
stakeToken.safeTransfer(recoveryAddress, amount);
}

  • The sequence:

    1. The agreement is hacked, so the registry reads CORRUPTED. But nobody interacted with the pool while it was attackable, so riskWindowStart is still 0. (docs/DESIGN.md §5 documents this exact state - "The no-risk-window CORRUPTED race".)

    2. The moderator judges the hack hit contracts outside this pool's cover and flags SURVIVED. Legal and intended — ConfidencePool.sol:359 accepts a CORRUPTED registry for SURVIVED (§8).

    3. Anyone calls sweepUnclaimedBonus(). The whole bonus pot moves to recoveryAddress. claimsStarted is still false.

    4. The moderator realises they were wrong — the hack was in scope — and re-flags to good-faith CORRUPTED, naming the whitehat. Accepted, because claimsStarted is false (ConfidencePool.sol:348).

    5. The re-snapshot reads snapshotTotalBonus = 0. The whitehat is paid principal only. The bonus is at recoveryAddress forever.

  • Why flagging SURVIVED does NOT open the risk window (the step that looks like it should save this): flagOutcome calls _observePoolState() first (line 349), which reads the registry. It's natural to assume that observation sets riskWindowStart and puts the bonus back into reserved. It doesn't — the pool has two markers driven by two non-overlapping state lists:

// ConfidencePool.sol
if (riskWindowStart == 0 && _isActiveRiskState(state)) {
_markRiskWindowStart(); // @> OPENS the window. Active-risk states only.
}
if (riskWindowEnd == 0 && _isTerminalState(state)) {
_markRiskWindowEnd(); // @> CLOSES it. Terminal states. <-- CORRUPTED lands HERE.
}
​
function _isActiveRiskState(IAttackRegistry.ContractState s) internal pure returns (bool) {
// @> CORRUPTED is NOT in this list, so _markRiskWindowStart() is never reached and
// @> riskWindowStart stays 0 -- even though the registry plainly says CORRUPTED.
return s == IAttackRegistry.ContractState.UNDER_ATTACK || s == IAttackRegistry.ContractState.PROMOTION_REQUESTED;
}
​
function _isTerminalState(IAttackRegistry.ContractState s) internal pure returns (bool) {
return s == IAttackRegistry.ContractState.PRODUCTION || s == IAttackRegistry.ContractState.CORRUPTED;
}

riskWindowStart means "we saw this protocol while it was under attack". CORRUPTED doesn't say that — it says "it's over, and it got hacked". The contract's own comment agrees: "Terminal states do not count as risk observations." So flagging SURVIVED on a CORRUPTED registry seals riskWindowEnd and leaves riskWindowStart at 0. It can never be opened later either, because terminal states are sticky — the registry cannot go back to UNDER_ATTACK. The PoC asserts both directly after flagOutcome runs.

  • The docs say the opposite, and name the only thing that can block this. protocol-readme.md:45:

    "the moderator may still flag it on their off-chain in-scope judgement (sweeping the pool whole, bonus included), provided no claimExpired call has resolved the pool first."

    The PoC never calls claimExpired — it meets the team's stated condition in full, and the bonus still vanishes. Their list of blockers is missing one: sweepUnclaimedBonus(). And "sweeping the pool whole, bonus included" is simply not what happens.

  • The code comment defending this is wrong. ConfidencePool.sol:558-560 explains that the sweep skips claimsStarted so nobody can donate 1 wei to grief the moderator's correction window. Fair for donations. But it ends with "Genuine reliance only comes from claim entrypoints" — false, because this sweep moves snapshotTotalBonus, real accounted money. docs/DESIGN.md §4 states the rule being broken: "Once value has left the contract, a corrective re-flag cannot be honored without breaking balance accounting."

  • Being upfront: the sweep's maths are correct at the moment it runs — under a finished SURVIVED with no risk window, the bonus does belong to recoveryAddress (§5). The defect is that it computes against an outcome that isn't finished. Nobody reserves for the whitehat, because at that moment the contract doesn't think one exists. The re-flag creates them afterwards, against a pot that's gone.

Risk

Likelihood: Low

  • Needs riskWindowStart == 0 on a CORRUPTED registry (a real state — docs/DESIGN.md §5 has a section on it), and the moderator to correct their own outcome (docs/DESIGN.md §4: the re-flag window exists "so the moderator can fix a typo'd outcome/attacker"). Both are documented as expected, but they must coincide.

  • The damaging step itself is free, permissionless, unprivileged, and irreversible — any address can call the sweep the moment the first flag lands.

Impact: Medium

  • The named whitehat loses the entire bonus pot — 100% of what docs/DESIGN.md §12 and protocol-readme.md:39 promise them ("the entire pool").

  • It lands at recoveryAddress — the sponsor's wallet — the exact opposite of where good-faith CORRUPTED sends funds. The sponsor keeps the reward meant for the person who found the hack.

  • No recovery path. Nothing can pull it back, and re-flagging just re-reads a balance that is now zero. Staker principal is unaffected.

Proof of Concept

Paste both functions into test/unit/ConfidencePool.t.sol (anywhere inside the existing contract ConfidencePoolTest block) and run:

forge test --match-path test/unit/ConfidencePool.t.sol --match-test "test_PoC" -vv

No new imports or setup needed — that file already imports IConfidencePool, IAttackRegistry and PoolStates, and extends BaseConfidencePoolTest.

Both functions matter. The first proves the theft. The second is the control: it changes one line (_passThroughUnderAttack()) and the attack becomes impossible — the bonus lands in reserved and the sweep reverts. That proves riskWindowStart == 0 is the sole cause, and rules out "the sweep just always takes the bonus, working as designed". The sweep is meant to protect the bonus; this state slips past the guard.

/// @notice PoC — `sweepUnclaimedBonus` permanently diverts the entire bonus pot while the
/// moderator's correction window is still open, so a later good-faith CORRUPTED flag pays the
/// named whitehat principal only instead of "the entire pool".
function test_PoC_sweepStealsAttackerBounty() external {
uint256 stakeAmt = 1000 * ONE;
uint256 bonusAmt = 500 * ONE;
​
_stake(alice, stakeAmt);
_contributeBonus(bob, bonusAmt);
​
// Breached, but the pool NEVER observed an active-risk state: no poke, no interaction
// while UNDER_ATTACK, so riskWindowStart stays 0. docs/DESIGN.md section 5.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
assertEq(pool.riskWindowStart(), 0, "precondition: no risk window observed");
​
// Step 1: moderator judges the breach out-of-scope and flags SURVIVED (legal, :359).
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
assertEq(pool.snapshotTotalBonus(), bonusAmt, "bonus is snapshot-accounted");
assertFalse(pool.claimsStarted(), "correction window is still open");
​
// flagOutcome ran _observePoolState() at :349 but did NOT open the risk window:
// _isActiveRiskState is UNDER_ATTACK/PROMOTION_REQUESTED only (:897), and CORRUPTED is
// TERMINAL -- so it seals riskWindowEnd and leaves riskWindowStart at 0. That is what
// keeps the bonus out of `reserved` below.
assertEq(pool.riskWindowStart(), 0, "CORRUPTED is terminal, not active-risk: window still NOT open");
assertGt(pool.riskWindowEnd(), 0, "flagOutcome sealed riskWindowEnd only");
​
// Step 2: ANY unprivileged caller sweeps. riskWindowStart == 0 keeps the bonus out of
// `reserved` (:540), and the sweep deliberately does not latch claimsStarted (:558-560).
vm.prank(carol);
pool.sweepUnclaimedBonus();
​
assertEq(token.balanceOf(recovery), bonusAmt, "entire bonus pot left to recoveryAddress");
assertEq(pool.totalBonus(), 0, "totalBonus zeroed");
assertFalse(pool.claimsStarted(), "VULN: real value moved but the outcome is still not final");
​
// Step 3: moderator corrects -- the breach WAS in scope. Accepted, claimsStarted false (:348).
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
​
assertEq(pool.snapshotTotalBonus(), 0, "re-snapshot sees the drained bonus");
assertEq(pool.bountyEntitlement(), stakeAmt, "VULN: entitlement is principal only, not stake + bonus");
​
// Step 4: the named whitehat claims and is short by the full bonus pot.
vm.prank(attacker);
pool.claimAttackerBounty();
​
assertEq(token.balanceOf(attacker), stakeAmt, "attacker paid principal only");
assertLt(token.balanceOf(attacker), stakeAmt + bonusAmt, "VULN: attacker did not receive the entire pool");
assertEq(stakeAmt + bonusAmt - token.balanceOf(attacker), bonusAmt, "loss to whitehat == the whole bonus pot");
}
​
/// @notice Control. Identical setup, ONE line different (`_passThroughUnderAttack`): with an
/// observed risk window the bonus IS reserved, the sweep reverts, and the attack is
/// impossible. Isolates `riskWindowStart == 0` as the sole cause.
function test_PoC_control_observedRiskWindowBlocksSweep() external {
uint256 stakeAmt = 1000 * ONE;
uint256 bonusAmt = 500 * ONE;
​
_stake(alice, stakeAmt);
_contributeBonus(bob, bonusAmt);
​
// The ONLY difference: the pool observes the active-risk state before it goes terminal.
_passThroughUnderAttack();
assertGt(pool.riskWindowStart(), 0, "risk window observed");
​
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
// reserved == the whole balance, so there is nothing above the reserve to sweep.
vm.prank(carol);
vm.expectRevert(IConfidencePool.NothingToSweep.selector);
pool.sweepUnclaimedBonus();
​
assertEq(token.balanceOf(recovery), 0, "bonus stays in the pool and reaches the attacker");
}

Result:

Ran 2 tests for test/unit/ConfidencePool.t.sol:ConfidencePoolTest
[PASS] test_PoC_control_observedRiskWindowBlocksSweep() (gas: 515301)
[PASS] test_PoC_sweepStealsAttackerBounty() (gas: 601779)
Suite result: ok. 2 passed; 0 failed; 0 skipped

The two runs differ by one line and nothing else:


 PoC (riskWindowStart == 0) Control (window observed)
reserved 1000e18 — bonus excluded 1500e18 — bonus included
pool balance 1500e18 1500e18
amount = balance - reserved 500e18 → swept 0 → reverts NothingToSweep
recoveryAddress receives 500e18 0
whitehat receives 1000e18 (short by the bonus) full pool

Recommended Mitigation

While the outcome can still change (claimsStarted == false), hold the bonus back unconditionally. Donations and dust stay sweepable, so the "1 wei grief" concern in the existing comment is still handled — but accounted bonus stays in the pool until the outcome is final.

--- a/src/ConfidencePool.sol
+++ b/src/ConfidencePool.sol
@@ function sweepUnclaimedBonus() external nonReentrant {
uint256 reserved;
if (totalEligibleStake != 0) {
reserved = totalEligibleStake;
- if (riskWindowStart != 0) {
+ // Also hold the bonus back while the outcome can still be re-flagged. Otherwise a
+ // sweep under a to-be-corrected SURVIVED permanently diverts the pot that a later
+ // good-faith CORRUPTED owes the named attacker.
+ if (riskWindowStart != 0 || !claimsStarted) {
reserved += snapshotTotalBonus - claimedBonus;
}
}

Trade-off, stated upfront: under a moderator-flagged SURVIVED where no staker ever claims, claimsStarted never flips, so the bonus stays reserved instead of sweeping. No funds are lost — it sits alongside the principal, and the first staker claiming unlocks it. If that delay is unacceptable, the alternative is to have sweepUnclaimedBonus set claimsStarted = true whenever the amount it moves includes accounted bonus (rather than only donations/dust) — keeping the sweep immediate while still making value-movement final, which is the rule docs/DESIGN.md §4 says should hold anyway.

Support

FAQs

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

Give us feedback!