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

Lacking documentation for setPoolImplementation

Author Revealed upon completion

setPoolImplementation only affects pools deployed after the change is not documented at the call leading to operational confusion

Description

  • createPool uses Clones.cloneDeterministic(poolImplementation, salt) to deploy each pool as an EIP-1167 minimal proxy pointed permanently at whatever poolImplementation was at creation time. This is correct and expected clone-factory behavior — updating poolImplementation has no effect on already-deployed pools, since each clone's DELEGATECALL target is baked into its immutable bytecode

  • The behavior itself is fine; the only gap is that nothing in the natspec or event states this explicitly, so an operator reading PoolImplementationUpdated in isolation (e.g. from a block explorer or monitoring dashboard) could mistakenly believe the update is a live, retroactive upgrade for all existing pools — which would be a reasonable assumption coming from UUPS-style upgradeable systems (which this factory itself uses for its own logic, per UUPSUpgradeable). That mismatch in upgrade model between the factory (upgradeable) and its pools (immutable per-clone) is worth calling out explicitly so operators don't misjudge the blast radius of a bad poolImplementation value — a buggy implementation set via this function only affects pools created going forward, not the ones already live.

function setPoolImplementation(address newPoolImplementation) external onlyOwner {
if (newPoolImplementation == address(0)) revert ZeroAddress();
address old = poolImplementation;
poolImplementation = newPoolImplementation;
emit PoolImplementationUpdated(old, newPoolImplementation);
}

Risk

Likelihood:

  • Not exploitable condition, purely a documentation/operational-clarity gap.

Impact:

  • Informational. Could lead to operator confusion (e.g. believing a hotfix has propagated to existing pools when it hasn't), but has no direct fund-safety or correctness consequence.

Proof of Concept

Not applicable — this is a documentation gap, not an exploitable code path. Illustrative behavior check:

function test_setPoolImplementation_doesNotAffectExistingClones() public {
address poolA = factory.createPool(agreement, token, expiry, minStake, recovery, accounts);
​
factory.setPoolImplementation(address(newImplementation));
​
address poolB = factory.createPool(agreement2, token, expiry, minStake, recovery, accounts);
​
// poolA keeps delegating to the old implementation; poolB uses the new one.
// Both are legitimate, expected outcomes — the point being purely that this
// is easy to misread from the emitted event alone.
assertTrue(_cloneImplementation(poolA) != _cloneImplementation(poolB));
}

Recommended Mitigation
Add an explicit natspec note on setPoolImplementation (and ideally on PoolImplementationUpdated) stating that the change applies prospectively only, e.g.:

/// @notice Updates the implementation address used for future `createPool` clones.
/// @dev Does NOT retroactively affect already-deployed pools — each is an EIP-1167 clone
/// with its implementation address fixed at deployment time. To patch a bug in an existing
/// pool's logic, a new pool must be created (or a migration path built separately).
function setPoolImplementation(address newPoolImplementation) external onlyOwner { ... }

Support

FAQs

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

Give us feedback!