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

Stake calldata does not bind the depositor's reviewed expiry or risk state

Author Revealed upon completion

Root + Impact

Description

stake(uint256 amount) can execute after the owner changes the pool's expiry or the registry enters active risk, committing the depositor under materially different terms without invalidating their pending transaction.

Stakers are expected to inspect the pool's on-chain parameters and live BattleChain registry state before depositing. The reviewed expiry defines the maximum underwriting term, while the registry state determines whether the depositor retains the pre-risk withdrawal option or immediately commits principal to resolution.

Deposits during UNDER_ATTACK are intentionally supported. A depositor who knowingly submits in that state accepts that stake() will seal riskWindowStart and withdraw() will be disabled. Similarly, the owner is intentionally allowed to edit expiry until the first successful stake, after which expiryLocked makes the deadline immutable.

The signed stake calldata contains only amount, however. It carries no expected expiry, expected registry state, active-risk consent, or transaction deadline. Both material conditions are therefore checked only at execution:

  • A transaction prepared in ATTACK_REQUESTED, where withdrawal is available, still succeeds if the ordinary DAO approveAttack() transition to UNDER_ATTACK executes first. The same call seals the risk window, and the depositor cannot withdraw after execution. A later valid CORRUPTED outcome can forfeit 100% of principal even though the submitted transaction did not authorize an active-risk deposit.

  • Before the first stake executes, the owner can order setExpiry() first and replace the displayed deadline with any valid value up to type(uint32).max. The pending stake still succeeds and permanently locks that substituted expiry. When it is also made during active risk, the depositor has neither withdrawal nor the expiry backstop at the deadline they reviewed.

Setting expiryLocked = true before safeTransferFrom() only prevents changes during the execution of stake() itself. It cannot protect against a separate owner transaction included immediately before the stake.

src/ConfidencePool.sol
​
function stake(uint256 amount) external nonReentrant whenPoolNotPaused {
if (amount == 0) revert InvalidAmount();
if (amount < minStake) revert BelowMinStake();
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
if (block.timestamp >= expiry) revert StakingClosed(); // @> Uses execution-time expiry only.
_assertDepositsAllowed(_observePoolState()); // @> Uses execution-time registry state only.
​
if (!expiryLocked) {
expiryLocked = true; // @> Locks whichever expiry won transaction ordering.
}
​
// ... transfer and accounting omitted ...
emit Staked(msg.sender, received);
}
​
function withdraw() external nonReentrant {
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
IAttackRegistry.ContractState state = _observePoolState();
if (
riskWindowStart != 0
|| (state != IAttackRegistry.ContractState.NOT_DEPLOYED
&& state != IAttackRegistry.ContractState.NEW_DEPLOYMENT
&& state != IAttackRegistry.ContractState.ATTACK_REQUESTED)
) {
revert WithdrawsDisabled(); // @> The newly accepted position cannot be undone.
}
​
// ... accounting and transfer omitted ...
stakeToken.safeTransfer(msg.sender, amount);
emit Withdrawn(msg.sender, amount);
}
​
function setExpiry(uint256 newExpiry) external onlyOwner {
if (expiryLocked) revert ExpiryLocked(); // @> False until the first stake actually executes.
if (newExpiry < block.timestamp + _MIN_EXPIRY_LEAD) revert ExpiryTooSoon();
if (newExpiry > type(uint32).max) revert ExpiryTooFar();
​
uint256 oldExpiry = expiry;
expiry = uint32(newExpiry);
​
emit ExpiryUpdated(oldExpiry, newExpiry);
}
​
function _assertDepositsAllowed(IAttackRegistry.ContractState state) private pure {
if (
state == IAttackRegistry.ContractState.PROMOTION_REQUESTED
|| state == IAttackRegistry.ContractState.PRODUCTION
|| state == IAttackRegistry.ContractState.CORRUPTED
) {
revert StakingClosed();
}
} // @> UNDER_ATTACK deliberately passes, but the caller cannot prove they consented to it.

The documented instruction to read the live registry before staking does not mitigate the issue. A read occurs before the transaction is signed or submitted; the relevant transition can be included before that pending transaction. Only a calldata-bound execution condition can preserve the value the depositor reviewed.

Risk

Likelihood:

  • ATTACK_REQUESTED -> UNDER_ATTACK is an ordinary BattleChain transition performed by the registry moderator. A pending stake only needs to be included after that transition; a compromised registry or malicious validator is not required.

  • The expiry substitution applies only to the first successful stake and requires a malicious or compromised pool owner to order setExpiry() before it. Public transaction visibility makes that ordering practical whenever the first stake is submitted through a public mempool.

  • Both cases use the normal stake(amount) interface and a standard ERC-20. No unusual token behavior, reentrancy, arithmetic edge case, or privileged action by the depositor is required.

Impact:

  • A stale pre-risk stake immediately loses its withdrawal right and becomes exposed to the complete CORRUPTED distribution. The PoC demonstrates the depositor's full principal being swept to recoveryAddress.

  • The owner can replace a reviewed 31-day term with the uint32 timestamp ceiling near February 2106. At the original maturity, claimExpired() still reverts PoolNotExpired, while an active-risk position remains non-withdrawable.

  • The first stake itself makes the substituted expiry permanent, so neither the depositor nor a later honest owner can restore the reviewed term after execution.

Proof of Concept

Create test/audit/CP003StakeTermsNotBound.t.sol with the following contents:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {BaseConfidencePoolTest} from "test/helpers/BaseConfidencePoolTest.sol";
​
contract CP003StakeTermsNotBoundTest is BaseConfidencePoolTest {
function test_PendingPreRiskStakeExecutesAfterRiskTransitionAndCanLosePrincipal() external {
uint256 amount = 100 * ONE;
​
// Alice reviews a withdrawable pre-risk pool and prepares stake(amount).
attackRegistry.setAgreementState(IAttackRegistry.ContractState.ATTACK_REQUESTED);
token.mint(alice, amount);
vm.prank(alice);
token.approve(address(pool), amount);
​
// The ordinary DAO approval is ordered before Alice's already-prepared transaction.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.UNDER_ATTACK);
​
// The identical stake(amount) calldata still succeeds, seals risk, and self-locks.
vm.prank(alice);
pool.stake(amount);
assertEq(pool.eligibleStake(alice), amount);
assertEq(pool.riskWindowStart(), block.timestamp);
​
vm.prank(alice);
vm.expectRevert(IConfidencePool.WithdrawsDisabled.selector);
pool.withdraw();
​
// A subsequent valid corruption now forfeits principal that Alice submitted while the
// registry was still pre-risk and withdrawable from her point of view.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.CORRUPTED);
vm.prank(moderator);
pool.flagOutcome(PoolStates.Outcome.CORRUPTED, false, address(0));
​
uint256 recoveryBefore = token.balanceOf(recovery);
pool.claimCorrupted();
​
assertEq(token.balanceOf(recovery) - recoveryBefore, amount);
assertEq(token.balanceOf(alice), 0);
assertEq(token.balanceOf(address(pool)), 0);
}
​
function test_OwnerCanReplaceFirstStakersReviewedExpiry() external {
uint256 amount = 100 * ONE;
​
// Alice knowingly prepares a first stake during UNDER_ATTACK against the displayed term.
attackRegistry.setAgreementState(IAttackRegistry.ContractState.UNDER_ATTACK);
token.mint(alice, amount);
vm.prank(alice);
token.approve(address(pool), amount);
uint256 reviewedExpiry = pool.expiry();
​
// The owner is ordered first and extends the term close to February 2106.
pool.setExpiry(type(uint32).max);
​
// stake(amount) carries no expected expiry, so it succeeds and permanently locks the
// substituted deadline instead of the value Alice reviewed.
vm.prank(alice);
pool.stake(amount);
​
assertEq(pool.expiry(), type(uint32).max);
assertGt(pool.expiry(), reviewedExpiry);
assertTrue(pool.expiryLocked());
assertEq(pool.eligibleStake(alice), amount);
​
// At Alice's reviewed maturity, neither the expiry backstop nor withdrawal is available.
vm.warp(reviewedExpiry);
vm.prank(alice);
vm.expectRevert(IConfidencePool.PoolNotExpired.selector);
pool.claimExpired();
​
vm.prank(alice);
vm.expectRevert(IConfidencePool.WithdrawsDisabled.selector);
pool.withdraw();
}
}

Run the PoC from the repository root:

  1. Execute forge test --offline --match-path test/audit/CP003StakeTermsNotBound.t.sol -vv.

  2. Confirm that both tests pass and the suite reports two tests passed with zero failures.

Recommended Mitigation

Replace the unguarded stake entrypoint with one that binds all material execution conditions chosen by the depositor. Exact equality for expiry and registry state provides the clearest semantics; a deadline bounds how long the authorization remains live.

src/interfaces/IConfidencePool.sol
​
+import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
​
+error StakeDeadlineExpired(uint256 deadline, uint256 currentTimestamp);
+error ExpiryChanged(uint32 expected, uint32 actual);
+error AgreementStateChanged(
+ IAttackRegistry.ContractState expected,
+ IAttackRegistry.ContractState actual
+);
​
-function stake(uint256 amount) external;
+function stake(
+ uint256 amount,
+ uint32 expectedExpiry,
+ IAttackRegistry.ContractState expectedState,
+ uint256 deadline
+) external;
​
src/ConfidencePool.sol
​
-function stake(uint256 amount) external nonReentrant whenPoolNotPaused {
+function stake(
+ uint256 amount,
+ uint32 expectedExpiry,
+ IAttackRegistry.ContractState expectedState,
+ uint256 deadline
+) external nonReentrant whenPoolNotPaused {
if (amount == 0) revert InvalidAmount();
if (amount < minStake) revert BelowMinStake();
+ if (block.timestamp > deadline) revert StakeDeadlineExpired(deadline, block.timestamp);
+ if (expiry != expectedExpiry) revert ExpiryChanged(expectedExpiry, expiry);
if (outcome != PoolStates.Outcome.UNRESOLVED) revert OutcomeAlreadySet();
if (block.timestamp >= expiry) revert StakingClosed();
- _assertDepositsAllowed(_observePoolState());
+ IAttackRegistry.ContractState state = _observePoolState();
+ if (state != expectedState) revert AgreementStateChanged(expectedState, state);
+ _assertDepositsAllowed(state);
​
if (!expiryLocked) {
expiryLocked = true;
}
​
// ... unchanged transfer and accounting ...
emit Staked(msg.sender, received);
}

The reverted state comparison also rolls back any lifecycle markers written by _observePoolState(), so a mismatch cannot accidentally commit the deposit or its observations. Update all integrations to pass values obtained during quote/review, and do not retain an unguarded public wrapper that silently supplies execution-time values.

If exact state equality is too restrictive for integrations, replace expectedState with an explicit caller policy such as acceptActiveRisk; the critical property is that a transition which removes withdrawal must require affirmative calldata consent. A pre-launch lockExpiry() requirement can additionally remove the owner-ordering race, but it does not replace the need to bind registry risk state.

Support

FAQs

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

Give us feedback!