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

`setPoolScope` calls `_observePoolState()` before `scopeLocked` guard check

Author Revealed upon completion

Root + Impact

Gas is wasted on an unnecessary external registry call, and misleading state-transition events are emitted in a transaction that ultimately reverts. Off-chain indexers may incorrectly interpret these events as genuine.

Description

  • The pool owner can update the pool's BattleChain scope until the registry leaves pre-attack staging (scopeLocked == true). Once locked, scope is immutable. setPoolScope should check the lock before making any external calls or state changes.

  • setPoolScope calls _observePoolState() at line 638 BEFORE checking scopeLocked at line 639. _observePoolState makes an external registry call and emits events (ScopeLocked, RiskWindowStarted). The scopeLocked check then reverts. While Solidity revert semantics roll back all state changes (the markers do NOT persist on revert), the external registry call wastes gas on a doomed transaction and misleading events appear in the receipt, potentially confusing off-chain indexers.

Risk

Likelihood:

Reason 1 — The registry state changes from pre-attack staging to active-risk during every pool's normal lifecycle. At that moment, `setPoolScope` becomes a guaranteed revert-after-external-call for any scope update attempt.
​
Reason 2 — The owner may call `setPoolScope` unaware that the registry has transitioned, wasting gas on an external call that was doomed to fail and emitting ScopeLocked/RiskWindowStarted events that off-chain indexers may interpret as genuine state transitions.

Impact:

Impact 1 — Gas wasted on an external registry call in a transaction destined to revert. Events emitted during the reverting transaction remain in the receipt.
​
Impact 2 — The ordering asymmetry violates the principle of least action: side effects run even when the function's purpose (changing scope) is impossible.

Proof of Concept

function test_M4_SetPoolScope_EffectBeforeGuard() public {
attackRegistry.setAgreementState(IAttackRegistry.ContractState.UNDER_ATTACK);
assertFalse(pool.scopeLocked());
​
address[] memory newScope = new address[](1);
newScope[0] = address(0xBEEF);
agreementContract.setContractInScope(address(0xBEEF), true);
​
// setPoolScope calls _observePoolState first (fires ScopeLocked event),
// then checks scopeLocked → reverts. State rolled back but events remain.
vm.expectRevert(); // ScopePostLockImmutable
pool.setPoolScope(newScope);
​
assertFalse(pool.scopeLocked()); // state rolled back by revert
assertEq(pool.riskWindowStart(), 0); // state rolled back by revert
// BUT: ScopeLocked and RiskWindowStarted events ARE in the transaction receipt
}

Forge output:

[PASS] test_M4_SetPoolScope_EffectBeforeGuard()
State IS rolled back (Solidity revert semantics)
But: events emitted + external call wasted on a reverting tx

Recommended Mitigation

Check the lock before making any external calls, so the transaction fails fast without wasting gas on a registry read and emitting misleading events that will be rolled back.

function setPoolScope(address[] calldata accounts) external onlyOwner {
+ if (scopeLocked) revert ScopePostLockImmutable(); // check FIRST
_observePoolState();
if (scopeLocked) revert ScopePostLockImmutable();
_replaceScope(accounts);
}

Support

FAQs

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

Give us feedback!