BattleChain Confidence Pools

Description

• Normal behavior: createPool validates the caller's authorization against the agreement contract, then deterministically clones and initializes a new pool for that agreement, tracking it in _poolsByAgreement[agreement].

• The issue: createPool calls out to attacker-influenceable code (safeHarborRegistry.isAgreementValid(agreement), then IAgreement(agreement).owner()) before any state-changing effect, with no nonReentrant guard:

An attacker-controlled agreement whose owner() reenters createPool (same agreement) can produce two pools from one logical call, since the inner call reads _poolsByAgreement[agreement].length before the outer call has pushed its own new pool.

Risk

Likelihood:

• Requires the attacker to already be the legitimate owner of the agreement contract being registered (the owner() check must pass), and for that owner() implementation to include a reentrant callback into createPool — a self-inflicted condition rather than one exploitable against a third party's agreement.

Impact:

• The inner reentrant call reads _poolsByAgreement[agreement].length == 0, creates/initializes a pool at salt = hash(agreement, 0), and pushes (length → 1). The outer call then reads length == 1 and creates a second, independently-salted pool.

• Both pools are correctly initialized with valid parameters — no parameter corruption or fund-theft path exists — so the impact is limited to extra gas spent and confusion for off-chain indexers that assume one PoolCreated event per createPool call. The attacker must already own the agreement, so the blast radius is limited to their own bookkeeping.

Proof of Concept

1. Attacker deploys an agreement contract whose owner() function, when called, reenters factory.createPool(agreement, ...) with the same agreement address.

2. Attacker calls factory.createPool(agreement, stakeToken, expiry, minStake, recoveryAddress, scope).

3. Inside the authorization check, IAgreement(agreement).owner() is invoked; this callback reenters createPool with the same arguments before the outer call has pushed anything to _poolsByAgreement[agreement].

4. The inner call reads _poolsByAgreement[agreement].length == 0, computes salt = hash(agreement, 0), clones and initializes pool #1, and pushes it (length becomes 1).

5. Control returns to the outer call, which (having already passed its own authorization check before the reentrant call occurred) proceeds to read _poolsByAgreement[agreement].length == 1, computes salt = hash(agreement, 1), and clones and initializes a second, independently-salted pool #2.

6. Result: one logical createPool transaction silently produces two valid, correctly-initialized pool clones instead of one.

Recommended Mitigation

Add nonReentrant to createPool (requires adding ReentrancyGuardUpgradeable to the factory's inheritance and initializing it in the factory's initializer), or reorder the function so the external owner() check is the very first read of anything a reentrant call could have advanced — e.g. capture _poolsByAgreement[agreement].length into a local variable immediately upon entry, before any external call, so a reentrant call cannot silently advance the length out from under the outer call's salt computation.