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

Accounted bonus can be swept before moderator re-flag finality

Author Revealed upon completion

Summary

sweepUnclaimedBonus() can move accounted bonus to recoveryAddress while the moderator’s pre-claim re-flag window is still open. If the moderator later corrects SURVIVED to good-faith CORRUPTED, the new snapshot uses the already-reduced totalBonus, so the attacker bounty excludes bonus that was part of the pool before the correction.

Vulnerability Details

Vulnerable code:

flagOutcome() intentionally allows the moderator to re-flag before any claim starts:

322: function flagOutcome(PoolStates.Outcome newOutcome, bool goodFaith_, address attacker_) external onlyModerator {
323: // Re-flag allowed pre-claim so the moderator can fix a typo'd outcome / attacker before
324: // any participant locks in the wrong distribution.
327: @> if (outcome != PoolStates.Outcome.UNRESOLVED && claimsStarted) revert OutcomeAlreadySet();
...
354: outcome = newOutcome;
357: snapshotTotalStaked = totalEligibleStake;
358: @> snapshotTotalBonus = totalBonus;
361: corruptedReserve = newOutcome == PoolStates.Outcome.CORRUPTED ? snapshotTotalStaked + snapshotTotalBonus : 0;
362: @> bountyEntitlement = willBeGoodFaithCorrupted ? snapshotTotalStaked + snapshotTotalBonus : 0;
​

The problem is that sweepUnclaimedBonus() can move real accounted bonus before that re-flag window is closed:

474: function sweepUnclaimedBonus() external nonReentrant {
475: if (outcome != PoolStates.Outcome.SURVIVED && outcome != PoolStates.Outcome.EXPIRED) {
476: revert OutcomeNotEligibleForSweep();
477: }
...
491: uint256 freeBalance = stakeToken.balanceOf(address(this));
492: uint256 amount = freeBalance > reserved ? freeBalance - reserved : 0;
...
499: @> if (totalEligibleStake == 0 || riskWindowStart == 0) {
500: @> totalBonus -= amount <= totalBonus ? amount : totalBonus;
501: }
...
503: // Intentionally does NOT set claimsStarted.
506: @> stakeToken.safeTransfer(recoveryAddress, amount);

When riskWindowStart == 0, the function treats the bonus as sweepable, transfers it to recoveryAddress, and reduces totalBonus. But it does not set claimsStarted, so the moderator can still re-flag afterward. A later good-faith CORRUPTED flag snapshots the already-drained totalBonus, reducing the attacker’s bounty.

Why this is not intended

The code comments show that the moderator is allowed to fix an outcome only before users start relying on the current distribution.

sweepUnclaimedBonus() does not set claimsStarted because otherwise someone could send a tiny amount of tokens directly to the pool and use that donation to block the moderator from correcting a mistake. That part makes sense.

The problem is that the function does not separate a direct donation from the pool’s real recorded bonus.

In the second test, the pool has 100 tokens of stake, 50 tokens of bonus, and a 25-token direct donation. The sweep sends 75 tokens to recoveryAddress, which includes both the donation and the full 50-token bonus. Even after that, the moderator can still change the outcome. When the outcome is corrected to good-faith CORRUPTED, the attacker receives only the 100-token stake instead of 150 tokens.

The third test makes the issue even clearer because there is no donation at all. The pool has no stake and only a 50-token bonus. That bonus is fully swept, but the moderator can still correct the outcome afterward. Since the new snapshot sees zero stake and zero bonus, the attacker bounty becomes zero.

So this is not only about safely removing random tokens sent by outsiders. The same logic also lets the pool’s recorded bonus leave before the outcome is final. Once that bonus has been moved, a later correction should not be allowed to calculate the bounty from the reduced amount.

The comments in flagOutcome() say re-flagging is allowed only before value movement creates reliance:
323: // Re-flag allowed pre-claim so the moderator can fix a typo'd outcome / attacker before
324: // any participant locks in the wrong distribution.
325: // The window closing on the FIRST claim (`claimsStarted`) is by design
326: // — a value-movement finality latch

So the intended rule is not “re-flagging remains open after pool value moves.” The intended rule is that re-flagging stays open before claims lock in a distribution.
sweepUnclaimedBonus() avoids setting claimsStarted to prevent a tiny donation from griefing the moderator’s correction window:

503: // Intentionally does NOT set claimsStarted. A direct-transfer donation of as little as 1
504: // wei would otherwise let anyone flip the flag post-flagOutcome and block the moderator's
505: // documented pre-claim re-flag window.

That makes sense for dust or unaccounted donations. It does not cover the PoC case, where the swept amount is the real 50 * ONE bonus added through contributeBonus(). Once that accounted bonus leaves the pool, a later corrected snapshot should not silently lose it.

Likelihood: Medium

This requires a particular order of events: the moderator first flags the pool as SURVIVED or EXPIRED, someone calls sweepUnclaimedBonus() before any claim, and the moderator later corrects the result to good-faith CORRUPTED.

The sequence is specific, but every step is allowed by the current code. The sweep is permissionless, and it does not close the moderator’s correction window.

Impact: Medium

The attacker receives less than the bounty that would have been available before the bonus was swept.

In the second test, the attacker should receive the 100-token stake plus the 50-token bonus. Instead, the bonus is sent to recoveryAddress, and the attacker receives only 100 tokens. The attacker loses 50 tokens from the corrected bounty.

In the third test, the pool contains only a 50-token bonus and no stake. After the bonus is swept, the corrected CORRUPTED outcome gives the attacker nothing. The entire bounty is removed.

The tokens are not burned or stuck. They are sent to recoveryAddress. The issue is that they go to the wrong side of the final distribution, which reduces or completely removes the attacker’s bounty..

test/unit/AuditReflagSweepBonusTest.t.sol

Proof Of Concept

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {BaseConfidencePoolTest} from "test/helpers/BaseConfidencePoolTest.sol";
​
contract AuditReflagSweepBonusTest is BaseConfidencePoolTest {
function testAccountedBonusSweepReducesLaterGoodFaithCorruptedBounty() external {
_stake(alice, 100 * ONE);
_contributeBonus(carol, 50 * ONE);
​
// Registry becomes CORRUPTED without an observed active-risk window.
// This keeps riskWindowStart == 0, so SURVIVED/EXPIRED bonus share is zero
// and the accounted bonus is sweepable.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
assertEq(pool.riskWindowStart(), 0);
​
// Moderator first flags SURVIVED.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
assertEq(pool.snapshotTotalStaked(), 100 * ONE);
assertEq(pool.snapshotTotalBonus(), 50 * ONE);
assertFalse(pool.claimsStarted());
​
// The accounted bonus is swept to recovery while the re-flag window stays open.
uint256 recoveryBefore = token.balanceOf(recovery);
pool.sweepUnclaimedBonus();
​
assertEq(token.balanceOf(recovery) - recoveryBefore, 50 * ONE);
assertEq(pool.totalBonus(), 0);
assertFalse(pool.claimsStarted());
​
// Moderator corrects to good-faith CORRUPTED before any claim.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
​
// The attacker bounty is now only the remaining stake, not stake + original bonus.
assertEq(pool.bountyEntitlement(), 100 * ONE);
assertEq(pool.snapshotTotalBonus(), 0);
​
uint256 attackerBefore = token.balanceOf(attacker);
vm.prank(attacker);
pool.claimAttackerBounty();
​
assertEq(token.balanceOf(attacker) - attackerBefore, 100 * ONE);
assertEq(token.balanceOf(recovery) - recoveryBefore, 50 * ONE);
}
​
function testDonationPlusAccountedBonusSweepStillReducesCorrectedBounty() external {
// Alice deposits 100 tokens as stake, while Carol adds another
// 50 tokens specifically as the pool's bonus.
_stake(alice, 100 * ONE);
_contributeBonus(carol, 50 * ONE);
​
// The registry already considers the agreement corrupted.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
// Despite that, the moderator temporarily records the pool as SURVIVED.
// Claims have not started yet, so the moderator can still correct this outcome.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
// Dave directly sends 25 extra tokens to the pool.
// These tokens are not recorded as stake or bonus; they are just an
// unaccounted donation sitting in the contract balance.
token.mint(dave, 25 * ONE);
vm.prank(dave);
token.transfer(address(pool), 25 * ONE);
​
uint256 recoveryBefore = token.balanceOf(recovery);
​
// Because the current outcome is SURVIVED and no claims have started,
// sweepUnclaimedBonus() sends both:
// - the recorded 50-token bonus, and
// - Dave's unaccounted 25-token donation
// to the recovery address.
pool.sweepUnclaimedBonus();
​
assertEq(token.balanceOf(recovery) - recoveryBefore, 75 * ONE);
assertEq(pool.totalBonus(), 0);
assertFalse(pool.claimsStarted());
​
// The moderator now fixes the earlier result and marks the pool as
// good-faith CORRUPTED, naming the attacker as the bounty recipient.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
​
// The correction does not bring back the 50-token bonus that was already swept.
// The attacker therefore receives only Alice's 100-token stake, even though
// the original corrupted-pool bounty would also have included the bonus.
// Dave's 25-token donation is also permanently sitting with recovery.
assertEq(pool.bountyEntitlement(), 100 * ONE);
assertEq(pool.snapshotTotalBonus(), 0);
​
uint256 attackerBefore = token.balanceOf(attacker);
vm.prank(attacker);
pool.claimAttackerBounty();
​
assertEq(token.balanceOf(attacker) - attackerBefore, 100 * ONE);
assertEq(token.balanceOf(recovery) - recoveryBefore, 75 * ONE);
}
​
function testNoStakerAccountedBonusSweepRemovesCorrectedBounty() external {
// This pool has no stakers. Carol only contributes a 50-token bonus.
// Therefore, if the pool is later correctly marked CORRUPTED,
// this bonus would be the attacker's entire possible bounty.
_contributeBonus(carol, 50 * ONE);
​
// The registry already reports that the agreement was corrupted.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
// The moderator first records the wrong outcome: SURVIVED.
// Claims still have not started, so this result remains changeable.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
assertEq(pool.snapshotTotalStaked(), 0);
assertEq(pool.snapshotTotalBonus(), 50 * ONE);
assertFalse(pool.claimsStarted());
​
uint256 recoveryBefore = token.balanceOf(recovery);
​
// While the incorrect SURVIVED result is active, anyone can sweep
// the full 50-token bonus to the recovery address.
pool.sweepUnclaimedBonus();
​
assertEq(token.balanceOf(recovery) - recoveryBefore, 50 * ONE);
assertEq(pool.totalBonus(), 0);
assertFalse(pool.claimsStarted());
​
// The moderator then corrects the result to good-faith CORRUPTED.
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
​
// Since there was no stake and the complete bonus was already swept,
// nothing remains for the attacker. A valid corrected CORRUPTED outcome
// ends with a zero bounty, even though 50 bonus tokens were originally
// available before the temporary SURVIVED flag.
assertEq(pool.bountyEntitlement(), 0);
assertEq(pool.snapshotTotalBonus(), 0);
}
}
forge test --match-path test/unit/AuditReflagSweepBonusTest.t.sol -vvv
Ran 3 test for test/unit/AuditReflagSweepBonusTest.t.sol:AuditReflagSweepBonusTest
​
Suite result: ok. 3 passed; 0 failed; 0 skipped
​

Tools Used

Manual review, Foundry test.

Recommendations

Close the re-flag window when sweepUnclaimedBonus() moves accounted bonus, while still allowing harmless dust/donation sweeps to avoid blocking moderator correction.

diff --git a/src/ConfidencePool.sol b/src/ConfidencePool.sol
@@
- if (totalEligibleStake == 0 || riskWindowStart == 0) {
- totalBonus -= amount <= totalBonus ? amount : totalBonus;
+ if (totalEligibleStake == 0 || riskWindowStart == 0) {
+ uint256 accountedBonusSwept = amount <= totalBonus ? amount : totalBonus;
+ totalBonus -= accountedBonusSwept;
+ if (accountedBonusSwept != 0 && !claimsStarted) {
+ claimsStarted = true;
+ }
}

This keeps the intended donation/dust behavior intact, but once real accounted bonus leaves the pool, the outcome becomes final just like other value-moving paths

Support

FAQs

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

Give us feedback!