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

claimAttackerBounty() unconditionally reverts with the semantically-incorrect BountyAlreadyClaimed() selector on bad-faith CORRUPTED pools, making the InvalidGoodFaithParams() guard permanently unreachable through any real transaction

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: each distinct failure condition in claimAttackerBounty() should map to a diagnostic custom error, so a caller (or off-chain tooling decoding the revert) can determine the actual root cause of the rejection. InvalidGoodFaithParams() exists specifically to signal "this pool has no bounty because it was flagged bad-faith."

  • Specific issue: for a bad-faith CORRUPTED pool, bountyEntitlement is 0 and bountyClaimed starts at its zero-initialized default, so the bountyClaimed == bountyEntitlement check (0 == 0) is always true and fires first — before the interpreter ever reaches the !goodFaith check. Every caller receives BountyAlreadyClaimed(), a selector whose plain meaning ("someone already claimed this bounty") is factually false, since no bounty ever existed to claim.

// 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();
...
}

First-party confirmation that this branch is dead code on the honest path: the project's own regression test, testRevert_claimAttackerBounty_badFaith (test/unit/ConfidencePool.branches.t.sol), has to use the stdstore cheatcode to forcibly overwrite bountyEntitlement storage just to reach the InvalidGoodFaithParams() branch — a facility that exists only inside the Foundry test environment and has no equivalent in any real, permissionless transaction.

Risk

Likelihood:

  • Reason 1 // Any address calling claimAttackerBounty() against a bad-faith CORRUPTED pool receives BountyAlreadyClaimed() — this is the only reachable outcome for every such call, with no special preconditions beyond the pool having been flagged bad-faith by the moderator, which is an ordinary, expected step of the protocol's own resolution flow.

  • Reason 2 // The correct, semantically-accurate selector InvalidGoodFaithParams() is declared in IConfidencePool.sol and referenced by the code, yet is structurally unreachable through any real caller for this specific pool state.

Impact:

  • Impact 1 // Off-chain automation that decodes revert selectors to drive behavior (whitehat bounty-monitoring bots, claim-status indexers, wallet UIs surfacing human-readable failure reasons) is unable to distinguish "this pool never had a bounty" from "someone already took the bounty" using the interface's own error surface, defeating the purpose of having a dedicated selector for this case.

  • Impact 2 // No funds move incorrectly and no state is corrupted as a result of this issue — the function still correctly and safely rejects every bad-faith bounty claim attempt; the defect is confined to which diagnostic selector is returned, not whether the call is rejected.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {ConfidencePool} from "src/ConfidencePool.sol";
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";
import {stdStorage, StdStorage} from "forge-std/StdStorage.sol";
​
/// @notice Standalone, drop-in PoC for Finding: "claimAttackerBounty() always reverts with
/// BountyAlreadyClaimed() instead of InvalidGoodFaithParams() for bad-faith CORRUPTED pools,
/// making the InvalidGoodFaithParams guard unreachable through any real, unprivileged call path."
///
/// Run with:
/// forge test --match-contract PoC_MisleadingBountySelector -vvv
contract PoC_MisleadingBountySelector is BaseConfidencePoolTest {
using stdStorage for StdStorage;
​
/// @dev Demonstrates that ANY caller attempting to claim a bounty on a natural (i.e. not
/// storage-cheated) bad-faith CORRUPTED pool receives BountyAlreadyClaimed() — a selector
/// whose plain-English meaning ("the bounty has already been claimed") is factually false;
/// no bounty was ever claimed, because no bounty ever existed for a bad-faith pool.
function testPoC_badFaithPool_revertsWithWrongSelector() external {
_stake(alice, 100 * ONE);
​
_passThroughUnderAttack();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
// Moderator flags bad faith: goodFaith_ = false, attacker_ = address(0).
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
​
// Ground truth: this is a bad-faith pool, no attacker was ever named, no bounty was
// ever claimed by anyone.
assertEq(pool.goodFaith(), false);
assertEq(pool.bountyEntitlement(), 0);
assertEq(pool.bountyClaimed(), 0);
​
// ANY caller (does not even need to be a real attacker) attempting to claim a bounty
// receives BountyAlreadyClaimed() — even though bountyClaimed == bountyEntitlement == 0
// purely because both are the zero-initialized default for a bad-faith pool, NOT because
// any claim ever happened.
vm.prank(alice);
vm.expectRevert(IConfidencePool.BountyAlreadyClaimed.selector);
pool.claimAttackerBounty();
​
vm.prank(bob);
vm.expectRevert(IConfidencePool.BountyAlreadyClaimed.selector);
pool.claimAttackerBounty();
​
// The semantically-correct selector for this situation, InvalidGoodFaithParams(),
// is declared and exists in the interface but is NEVER reachable here.
}
​
/// @dev Proves the claim above by construction: the ONLY way to observe
/// InvalidGoodFaithParams() being thrown from claimAttackerBounty() on a bad-faith pool is
/// to artificially overwrite `bountyEntitlement` storage with a test-only cheatcode
/// (`stdstore`), which has no equivalent in a real, permissionless call. This mirrors the
/// project's own regression test (test/unit/ConfidencePool.branches.t.sol,
/// `testRevert_claimAttackerBounty_badFaith`), reproduced here standalone to make the point
/// explicit: the maintainers themselves had to reach for a storage cheat to exercise this
/// branch, which is independent confirmation that it is unreachable through any real,
/// unprivileged transaction.
function testPoC_onlyReachableViaStorageCheat_confirmsDeadBranch() external {
_stake(alice, 10 * ONE);
_passThroughUnderAttack();
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
​
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
​
// Only reachable via a test-only storage cheat -- not reachable by any real caller.
stdstore.target(address(pool)).sig("bountyEntitlement()").checked_write(uint256(1));
​
vm.prank(bob);
vm.expectRevert(IConfidencePool.InvalidGoodFaithParams.selector);
pool.claimAttackerBounty();
}
}

Recommended Mitigation

function claimAttackerBounty() external nonReentrant {
if (outcome != PoolStates.Outcome.CORRUPTED) revert OutcomeNotSet();
- if (bountyClaimed == bountyEntitlement) revert BountyAlreadyClaimed();
- if (!goodFaith) revert InvalidGoodFaithParams();
+ if (!goodFaith) revert InvalidGoodFaithParams();
+ if (bountyClaimed == bountyEntitlement) revert BountyAlreadyClaimed();
if (msg.sender != attacker) revert NotAttacker();
if (block.timestamp > corruptedClaimDeadline) revert ClaimWindowExpired();
...
}

Swapping the order of these two independent checks makes InvalidGoodFaithParams() fire first for bad-faith pools (correctly identifying the root cause), while preserving BountyAlreadyClaimed()'s correct, reachable behavior for good-faith pools whose bounty has genuinely been fully claimed. This is a pure reordering with no change to any fund-moving logic.

Updates

Lead Judging Commences

inallhonesty Lead Judge
16 days ago
inallhonesty Lead Judge 2 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

QA / Informational / Gas

inallhonesty Lead Judge 2 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

QA / Informational / Gas

Support

FAQs

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

Give us feedback!