Description
Normal Behavior
The ConfidencePoolFactory is intended to completely validate input parameters before deploying and initializing a new clone of ConfidencePool. This ensures the factory cleanly rejects invalid configurations rather than spending gas deploying a clone that will immediately revert during its initialize call.
Specific Issue
The factory's createPool function checks the lower bound of expiry (if (expiry < block.timestamp + _MIN_EXPIRY_LEAD) revert ExpiryTooSoon();) but forgets to check the upper bound (expiry > type(uint32).max). Because initialize() in ConfidencePool does check this, the clone initialization reverts with ExpiryTooFar(). The transaction fails, but it fails ungracefully after deploying the proxy, rather than validating at the factory level.
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();
Risk
Likelihood:
The sponsor mistakenly enters an expiry timestamp exceeding the uint32.max threshold (Year 2106).
The frontend incorrectly formats the expiry as milliseconds instead of seconds, trivially exceeding the bound.
Impact:
The transaction reverts from the inner clone's initialize call rather than cleanly at the factory input validation stage.
Minor gas inefficiency on revert due to performing the Clones.cloneDeterministic deployment before catching the error.
Proof of Concept
Create a new test file test/unit/POCTest.t.sol inheriting from BaseConfidencePoolTest, paste the following code inside the contract body, and run it using the command forge test --match-test test_POC_FactoryExpiryValidation -vvv.
pragma solidity 0.8.26;
import {Test, console} from "forge-std/Test.sol";
import {ConfidencePool} from "src/ConfidencePool.sol";
import {ConfidencePoolFactory} from "src/ConfidencePoolFactory.sol";
import {PoolStates} from "src/libraries/PoolStates.sol";
import {BaseConfidencePoolTest} from "test/helpers/BaseConfidencePoolTest.sol";
import {MockAgreement} from "test/mocks/MockAgreement.sol";
import {IAttackRegistry} from "@battlechain/interface/IAttackRegistry.sol";
import {IAgreement} from "@battlechain/interface/IAgreement.sol";
import {IConfidencePool} from "src/interfaces/IConfidencePool.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
contract POCTest is BaseConfidencePoolTest {
function test_POC_FactoryExpiryValidation() external {
console.log("--- POC: Factory Expiry Validation ---");
ConfidencePoolFactory factoryImpl = new ConfidencePoolFactory();
ERC1967Proxy proxy = new ERC1967Proxy(
address(factoryImpl),
abi.encodeWithSelector(
ConfidencePoolFactory.initialize.selector,
address(safeHarborRegistry),
address(new ConfidencePool()),
moderator
)
);
ConfidencePoolFactory testFactory = ConfidencePoolFactory(address(proxy));
testFactory.setStakeTokenAllowed(address(token), true);
address[] memory accs = new address[](1);
accs[0] = DEFAULT_SCOPE_ACCOUNT;
uint256 invalidExpiry = uint256(type(uint32).max) + 1;
console.log("Attempting to create pool with expiry > uint32.max :", invalidExpiry);
vm.expectRevert(IConfidencePool.ExpiryTooFar.selector);
vm.prank(address(this));
testFactory.createPool(
address(agreement),
address(token),
invalidExpiry,
1 * ONE,
recovery,
accs
);
console.log("Successfully caught ExpiryTooFar() bubbling up from the clone's initialize() rather than factory.");
}
}
Recommended Mitigation
To gracefully reject invalid expiry inputs at the factory level and save unnecessary proxy deployment gas, add a type(uint32).max upper bound check alongside the existing lower bound check.
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();
+ if (expiry > type(uint32).max) revert ExpiryTooFar();
