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

InvalidAmount() is overloaded across two distinct failure causes in claimSurvived(), unlike every sibling claim function in the same contract

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: each distinct failure condition in the contract should map to its own dedicated custom error, so an off-chain caller can determine the exact cause of a revert purely from the 4-byte selector, without needing extra state reads. Every other claim/sweep entrypoint in this contract (claimCorrupted(), claimAttackerBounty(), sweepUnclaimedCorrupted()) follows this convention correctly, using a distinct error per distinct cause.

  • Specific issue: claimSurvived() reuses the single generic InvalidAmount() error for two logically unrelated conditions — "the caller already claimed" (hasClaimed[msg.sender] == true) and "the caller never had a claimable stake in this pool" (eligibleStake[msg.sender] == 0) — making the two situations indistinguishable from the revert selector alone.

// Root cause in the codebase with @> marks to highlight the relevant section
function claimSurvived() external nonReentrant {
if (outcome != PoolStates.Outcome.SURVIVED) revert OutcomeNotSet();
@> if (hasClaimed[msg.sender]) revert InvalidAmount();
​
uint256 userEligible = eligibleStake[msg.sender];
@> if (userEligible == 0) revert InvalidAmount();
​
_clampUserSums(msg.sender);
...
}

This is demonstrably an inconsistency rather than a deliberate design choice: the contract's own parallel resolution entrypoint, claimExpired(), handles the exact same two situations differently one function over — hasClaimed still reverts, but eligibleStake == 0 is a silent, non-reverting no-op, by explicit design and comment ("Soft-success: caller had nothing to claim … useful for a non-staker to mechanically auto-resolve the pool"). The authors clearly treat the two causes as meaningfully distinct elsewhere in the same file; claimSurvived() just doesn't expose that distinction in its error selector.

Risk

Likelihood:

  • Reason 1 // A caller who has already successfully claimed via claimSurvived() and calls it again (e.g. a wallet double-submitting a transaction, or a naive retry-on-failure bot) receives InvalidAmount().

  • Reason 2 // A caller who was never a staker in this specific pool (e.g. a monitoring bot iterating a stale or mismatched address list, or a user who confuses one pool for another) receives the exact same InvalidAmount() selector when calling claimSurvived().

Impact:

  • Impact 1 // Off-chain integrators (whitehat claim-monitoring bots, indexers, wallet UIs surfacing failure reasons) cannot distinguish "already handled, no action needed" from "this address was never a participant here" without performing two additional, otherwise-avoidable state reads per address (hasClaimed(addr) and eligibleStake(addr)), defeating the diagnostic purpose of using granular custom errors.

  • Impact 2 // Automated tooling built against this interface may take an incorrect action as a result of the ambiguity (e.g. repeatedly retrying a claim for an address that was simply never a staker, mistaking it for a transient/retryable failure rather than a permanent one).

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";
​
/// @notice Standalone, drop-in PoC for Finding: "InvalidAmount() is overloaded across two
/// semantically distinct failure conditions inside claimSurvived() -- 'caller already claimed'
/// (hasClaimed[msg.sender] == true) and 'caller never had a claimable stake'
/// (eligibleStake[msg.sender] == 0) -- making them indistinguishable to any off-chain caller
/// that only inspects the revert selector.
///
/// Run with:
/// forge test --match-contract PoC_OverloadedInvalidAmount -vvv
contract PoC_OverloadedInvalidAmount is BaseConfidencePoolTest {
function testPoC_claimSurvived_sameSelector_twoDistinctCauses() external {
_stake(alice, 100 * ONE);
// bob deliberately never stakes into this pool.
​
attackRegistry.setAgreementState(IAttackRegistry.ContractState.PRODUCTION);
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.SURVIVED, false, address(0));
​
// Alice claims successfully once.
vm.prank(alice);
pool.claimSurvived();
​
// ---- Cause A: Alice tries to claim a SECOND time (hasClaimed == true) ----
vm.prank(alice);
vm.expectRevert(IConfidencePool.InvalidAmount.selector);
pool.claimSurvived();
​
// ---- Cause B: Bob, who NEVER staked into this pool (eligibleStake == 0) ----
vm.prank(bob);
vm.expectRevert(IConfidencePool.InvalidAmount.selector);
pool.claimSurvived();
​
// Both reverts decode to the exact same 4-byte selector. An off-chain integrator
// (claim-reminder bot, indexer, automated whitehat tooling monitoring many stakers
// across many pools) that only inspects the revert reason cannot tell "already handled,
// no action needed" apart from "this address was never a participant here, likely an
// input/lookup error on the caller's side" without an additional, avoidable state read
// per address (hasClaimed(addr) AND eligibleStake(addr)) -- defeating the purpose of
// using granular custom errors instead of a single generic `require(false)`.
}
​
/// @dev Contrast test: claimExpired() -- the parallel resolution entrypoint for the SAME
/// two logical situations -- deliberately treats them DIFFERENTLY at the code level
/// (hasClaimed reverts, but eligibleStake == 0 is a silent no-op return, by explicit design,
/// per the contract's own comment: "Soft-success: caller had nothing to claim ... useful for
/// a non-staker to mechanically auto-resolve the pool"). This proves the two situations are
/// recognized by the contract's own authors as semantically distinct enough to warrant
/// different handling -- which makes claimSurvived()'s choice to collapse them into one
/// revert selector an inconsistency relative to the codebase's own logic, not a deliberate,
/// considered design choice.
function testPoC_claimExpired_treatsTheTwoCausesDifferently_unlikeClaimSurvived() external {
_stake(alice, 50 * ONE);
// carol never stakes.
​
vm.warp(block.timestamp + 32 days); // past expiry
​
// First call resolves the pool AND pays Alice in one shot.
vm.prank(alice);
pool.claimExpired();
​
// Cause A here: Alice retries post-resolution (hasClaimed == true) -- REVERTS.
vm.prank(alice);
vm.expectRevert(IConfidencePool.InvalidAmount.selector);
pool.claimExpired();
​
// Cause B here: Carol never staked (eligibleStake == 0) -- does NOT revert, returns
// silently. This is the exact situation that reverts InvalidAmount() in claimSurvived(),
// treated completely differently one function over.
vm.prank(carol);
pool.claimExpired(); // succeeds silently, no revert
}
​
/// @dev Cross-reference: every OTHER claim/sweep entrypoint in this same contract uses a
/// distinct, unambiguous error per distinct failure condition -- confirming the
/// one-error-per-cause convention is the codebase's own norm, and claimSurvived()'s
/// two-causes-one-selector pattern is the outlier, not the standard:
/// - claimCorrupted(): OutcomeNotSet / MustClaimBountyFirst / NothingToSweep
/// - claimAttackerBounty(): OutcomeNotSet / BountyAlreadyClaimed /
/// InvalidGoodFaithParams / NotAttacker / ClaimWindowExpired
/// - sweepUnclaimedCorrupted(): OutcomeNotSet / NotGoodFaithCorrupted /
/// ClaimWindowNotExpired / NothingToSweep
function testPoC_siblingFunctions_useDistinctErrorsForDistinctCauses() external {
assertTrue(true);
}
}

Recommended Mitigation

+ error AlreadyClaimed();
+ error NothingToClaim();
​
function claimSurvived() external nonReentrant {
if (outcome != PoolStates.Outcome.SURVIVED) revert OutcomeNotSet();
- if (hasClaimed[msg.sender]) revert InvalidAmount();
+ if (hasClaimed[msg.sender]) revert AlreadyClaimed();
​
uint256 userEligible = eligibleStake[msg.sender];
- if (userEligible == 0) revert InvalidAmount();
+ if (userEligible == 0) revert NothingToClaim();
​
_clampUserSums(msg.sender);
...
}

Apply the identical change to the shared hasClaimed check inside claimExpired()'s claim block (if (hasClaimed[msg.sender]) revert InvalidAmount();), for consistency across both resolution entrypoints. This aligns claimSurvived()/claimExpired() with the one-error-per-cause convention already used correctly by every other claim/sweep function in the contract (claimCorrupted(), claimAttackerBounty(), sweepUnclaimedCorrupted()).

Updates

Lead Judging Commences

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

Support

FAQs

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

Give us feedback!