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

Named attacker receives a misleading BountyAlreadyClaimed error instead of the accurate reason for their failed claim

Author Revealed upon completion

Root + Impact

Description

  • laimAttackerBounty checks bountyClaimed == bountyEntitlement first, and reverts BountyAlreadyClaimed if that's true. The name of that error only makes sense in the one case it was written for, the attacker genuinely already received their payout. The check itself doesn't actually verify that, it just compares two numbers, and there are two real paths where those numbers end up equal without the attacker ever having received a cent.

    The first path: sweepUnclaimedCorrupted sets bountyClaimed = bountyEntitlement once the 180 day window expires unclaimed, purely as bookkeeping so the pool reads as resolved. If the attacker tries to claim after that point, they hit the same error a genuinely-paid attacker would see. The second path connects directly to a separate finding in this audit, when nobody staked or contributed bonus before a good-faith CORRUPTED flag, bountyEntitlement is 0 from the moment of the flag itself, so 0 == 0 is already true on the attacker's very first ever attempt, no prior sweep, no prior anything.

// Root cause in the codebase with @> marks to highlight the relevant section
​
function claimAttackerBounty() external nonReentrant {
if (outcome != PoolStates.Outcome.CORRUPTED) revert OutcomeNotSet();
@> if (bountyClaimed == bountyEntitlement) revert BountyAlreadyClaimed();
if (!goodFaith) revert InvalidGoodFaithParams();
if (msg.sender != attacker) revert NotAttacker();
if (block.timestamp > corruptedClaimDeadline) revert ClaimWindowExpired();
​
function sweepUnclaimedCorrupted() external nonReentrant {
if (outcome != PoolStates.Outcome.CORRUPTED) revert OutcomeNotSet();
if (!goodFaith) revert NotGoodFaithCorrupted();
if (block.timestamp <= corruptedClaimDeadline) revert ClaimWindowNotExpired();
...
@> bountyClaimed = bountyEntitlement;

Risk

Likelihood:

  • Whenever either underlying scenario happens, a missed 180 day deadline or an unstaked pool at flag time, this error fires every single time, not occasionally. The likelihood here tracks how often those two scenarios occur, and once they do, the misleading error is guaranteed, not a rare coincidence layered on top.

Impact:

  • Impact 1

  • Impact 2

Proof of Concept

The first test proves the error is misleading, not just imprecise, by showing the deadline check would have produced the same outcome on its own, this is purely about which message the user actually sees. The second test confirms a real double-claim attempt still shows the same error correctly, so this isn't a general break of the message, just these two specific triggers. The third test is the sharper one, it shows the same wrong error firing on a completely fresh pool, first interaction ever, while the window is still wide open.
​
​
function testSweepThenLateClaimRevertsWithMisleadingBountyAlreadyClaimedNotDeadlineExpired() external {
_stake(alice, 100 * ONE);
_passThroughUnderAttack();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
​
// Attacker never claims. 180 days pass; anyone sweeps the untouched pool.
vm.warp(pool.corruptedClaimDeadline() + 1);
pool.sweepUnclaimedCorrupted();
​
assertEq(pool.bountyClaimed(), pool.bountyEntitlement(), "sweep marks bountyClaimed as if fully paid");
assertEq(token.balanceOf(attacker), 0, "attacker received literally nothing");
​
vm.prank(attacker);
vm.expectRevert(IConfidencePool.BountyAlreadyClaimed.selector);
pool.claimAttackerBounty();
}
​
function testZeroEntitlementCaseShowsSameMisleadingErrorOnFirstEverAttempt() external {
// Nobody stakes, nobody contributes bonus. A real breach still happens.
_passThroughUnderAttack();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, true, attacker);
​
assertEq(pool.bountyEntitlement(), 0);
assertEq(pool.bountyClaimed(), 0, "attacker has made zero attempts so far");
​
// First ever interaction with this pool -- still shows BountyAlreadyClaimed.
vm.prank(attacker);
vm.expectRevert(IConfidencePool.BountyAlreadyClaimed.selector);
pool.claimAttackerBounty();
}
The token.balanceOf(attacker), 0 assertion in the first test is the key proof, the error name and reality directly contradict each other. The third test needs no balance assertion at all to make its point, the deadline in that run hasn't even started to matter yet, and the message still tells the attacker they're too late.

Recommended Mitigation

Check the deadline before checking whether the bounty was already claimed, so a caller who's actually out of time sees that instead:
​
​
function claimAttackerBounty() external nonReentrant {
if (outcome != PoolStates.Outcome.CORRUPTED) revert OutcomeNotSet();
- if (bountyClaimed == bountyEntitlement) revert BountyAlreadyClaimed();
if (!goodFaith) revert InvalidGoodFaithParams();
if (msg.sender != attacker) revert NotAttacker();
if (block.timestamp > corruptedClaimDeadline) revert ClaimWindowExpired();
+ if (bountyClaimed == bountyEntitlement) revert BountyAlreadyClaimed();
This doesn't fix the zero-entitlement case on its own, that one fires before the deadline even matters, and fixing it properly means addressing how bountyEntitlement gets computed in the first place, which is the separate finding this connects to. What this reordering does fix cleanly is the post-sweep case, and it costs nothing, a genuine double claim within the window still falls through to the correct BountyAlreadyClaimed exactly as it does today, verified directly by the second test above.

Support

FAQs

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

Give us feedback!