BattleChain Confidence Pools

Description

createPool calls IAgreement.owner() and, via the clone's initialize(), IAgreement.isContractInScope() both on a caller-supplied agreement contract before writing any factory state. The function has no nonReentrant modifier. agreement is only checked via isAgreementValid (registration, not code safety), and agreement registration is permissionless, so a sponsor can supply an agreement whose owner() or isContractInScope() reenters createPool.

Relevant GitHub Links

https://github.com/CodeHawks-Contests/2026-07-bc-confidence-pools/blob/main/src/ConfidencePoolFactory.sol

Risk

Likelihood: Low — requires a sponsor to deliberately deploy a malicious agreement contract; there's no incentive for a third party to trigger this against someone else's pool.
Impact: None currently. Traced below: the reentrant path produces two valid pools, not a fund-loss or corruption bug. The risk is forward-looking the safety depends on an undocumented ordering property that a future refactor could break.

Vulnerability Details

agreement is a caller-supplied address whose only validation is safeHarborRegistry.isAgreementValid(agreement) i.e., that it is registered with the Safe Harbor registry, not that its bytecode is benign. Because agreement registration is permissionless by design (any sponsor can register their own Safe Harbor agreement), a sponsor can supply an agreement contract whose owner() or isContractInScope() implementation reenters createPool.

createPool has no nonReentrant modifier, and both IAgreement.owner() and isContractInScope() are called before _poolsByAgreement[agreement].push(pool) executes.

Impact

Traced impact of a successful reentrant call:

The salt keccak256(abi.encode(agreement, _poolsByAgreement[agreement].length)) is computed by reading _poolsByAgreement[agreement].length fresh at the point of computation, not cached before the external calls. A nested (reentrant) call therefore reads length = N, creates and pushes its own pool, and returns; the original (outer) call then reads length = N+1 and computes a different salt.

Consequence:

Two distinct, validly-initialized pools are created rather than a CREATE2 address collision, a double-initialized clone, or a corrupted _poolsByAgreement array. This matches the protocol's own documented design ("An agreement may back many pools — each commits to its own (locked) scope, so duplicates are allowed and curated off-chain," per docs/DESIGN.md §10/README).

Given this, the current implementation does not appear to be exploitable for fund loss, denial of service, or storage corruption. The finding is that the function's safety currently depends on an implicit ordering property (the salt being read fresh per call rather than cached) that is not documented as a security invariant anywhere in the codebase or comments. A future refactor that caches _poolsByAgreement[agreement].length into a local variable before the external calls a natural-looking gas optimization — would silently reintroduce a genuine CREATE2 collision / front-running vector without any other code change being flagged as risky.

Proof of Concept

Reasoning-based PoC (no code changes to the contract needed to demonstrate the mechanism):

1. Attacker deploys MaliciousAgreement; its owner() reenters createPool with the same agreement address on first invocation.

2. Attacker registers MaliciousAgreement with the Safe Harbor registry (permissionless), so isAgreementValid passes.

3. Attacker calls createPool(address(MaliciousAgreement), ...).
   owner() reenters createPool: reads length == 0, deploys/initializes/pushes pool_0; length becomes 1.

4. Reentrant call returns; outer call proceeds, reads length == 1, deploys/initializes/pushes pool_1 (different salt/address).

5. Two independently funded, independently resolvable pools exist for one agreement — no collision, no fund loss.

Recommended Mitigation

Add nonReentrant (from OpenZeppelin's ReentrancyGuardUpgradeable, consistent with the pattern already used in ConfidencePool.sol) to createPool:

This is defense-in-depth rather than a fix for an active exploit: it removes the function's implicit dependence on the salt being computed fresh per call, so future refactors cannot silently reintroduce a real collision/front-running vector without the reentrancy guard already closing that door.

Tools Used

Manual review, cross-referenced against docs/DESIGN.md and the project's own test suite (test/unit/ConfidencePoolFactory.t.sol) to confirm multi-pool-per-agreement is intended behavior. Additionally validated via ~154,000 fuzzed calls across two complementary Foundry invariant-test harnesses (moderator-enabled and moderator-disabled/forced-auto-resolution variants) covering stake, withdraw, contributeBonus, flagOutcome, claimSurvived, claimExpired, claimCorrupted, claimAttackerBounty, sweepUnclaimedCorrupted, and sweepUnclaimedBonus — all of which held token-conservation and staker-solvency invariants throughout, with no violations found elsewhere in the codebase.