BattleChain Confidence Pools

Description

• Normal behavior: setPoolScope (owner-only, pre-lock) replaces the pool's scope list. The contract's own natspec on ScopeUpdated states this event carries the full account list so indexers can reconstruct scope without a follow-up view call — i.e. the event is meant to be a faithful, standalone record of the committed on-chain scope.

• The issue: _replaceScope is not reentrancy-guarded, and it makes an external call (IAgreement(agreement).isContractInScope(account)) inside the per-account loop, before that iteration's own effects are applied and before the event is emitted:

Because agreement is the only external contract called for both pool-creation identity (owner()) and scope membership (isContractInScope), and the factory's own _poolsByAgreement[agreement] explicitly supports many pools per agreement, a single malicious agreement contract can affect the scope commitment of every pool built on it — including pools created later by other sponsors who trusted the agreement's registration but did not author its bytecode.

Risk

Likelihood:

• Requires an attacker to control the agreement contract's bytecode (deploy AgreementAndOwner implementing owner()/isContractInScope()), then become the pool's owner by calling factory.createPool from that same contract so owner() == msg.sender, satisfying the factory's authorization check.

• Once owner, the attacker simply calls setPoolScope on their own pool — no special timing or race against another party is needed, since the reentrancy is self-triggered from within their own controlled agreement callback.

Impact:

• The final on-chain _scopeAccounts array silently contains extra addresses ([X, Y, A, B, C]) beyond what the last-emitted ScopeUpdated([A, B, C]) event shows, desyncing the "binding audit trail" the moderator is documented to judge CORRUPTED breaches against from the actual committed scope.

• Because one malicious agreement can serve many pools (per the factory's own one-to-many _poolsByAgreement mapping), the blast radius extends beyond the attacker's own pool to any other sponsor's pool built on the same compromised agreement.

Proof of Concept

1. Attacker deploys AgreementAndOwner implementing owner() → address(this) and isContractInScope(address) with a reentrant callback.

2. Attacker calls factory.createPool(address(AgreementAndOwner), stakeToken, expiry, minStake, recoveryAddress, [A,B,C]) from AgreementAndOwner itself, so msg.sender == agreement; the factory sets owner_ = msg.sender, making pool.owner() == address(AgreementAndOwner) once initialize() completes.

3. As legitimate owner, attacker calls pool.setPoolScope([A,B,C]). Inside _replaceScope's loop, isContractInScope(A) calls back into AgreementAndOwner, which reenters pool.setPoolScope([X,Y]).

4. The reentrant call's msg.sender (from the pool's view) is AgreementAndOwner, which is pool.owner(), so onlyOwner passes. The inner _replaceScope([X,Y]) runs to completion first: old = _scopeAccounts is still empty (outer loop hasn't pushed A yet), so it pushes X, Y and emits ScopeUpdated([X, Y]).

5. Control returns to the outer loop at A: it pushes A on top of what the inner call left, giving _scopeAccounts = [X, Y, A]; remaining iterations append B, C, giving a final _scopeAccounts = [X, Y, A, B, C].

6. The outer call emits ScopeUpdated([A, B, C]) as the last log entry, while the actual committed scope silently also contains X and Y.

Recommended Mitigation

Additionally, restructure _replaceScope to strict CEI: resolve and validate the entire new list into memory first (all isContractInScope calls up front, before any storage write), then apply storage writes and emit the event as one atomic, non-interleaveable block. Adding nonReentrant is trivial since ReentrancyGuard is already inherited and used elsewhere in the contract.