PanopticFactory#deployNewPool() can be called by anyone if initialize() was front-run.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticFactory.sol#L134-L139
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticFactory.sol#L210-L276

Vulnerability details

Description

1. PanopticFactory#initialize() can be called with any _owner value even if it is zero value.

function initialize(address _owner) public {
	if (!s_initialized) {
		s_owner = _owner;   //<--------- @audit
		s_initialized = true;
	}
}

2. s_owner can't be changed with Panoptic#transferOwnership()
   • If s_owner was initialized with some invalid value first.
   • If the attacker did front-run PanopticFactory#initialize() transaction with his address.

function transferOwnership(address newOwner) external {	
	address currentOwner = s_owner;
	if (msg.sender != currentOwner)   //<----- @audit
		revert Errors.NotOwner();
	...
}

3. If s_owner was assigned to zero address,
   • the caller check logic of PanopticFactory#deployNewPool() is skipped (L224) and so PanopticFactory#deployNewPool() can be called anyone.
   • the normal user can't deploy new pool any more if attacker called PanopticFactory#deployNewPool() with the all available token0, token1, fee pairs.

function deployNewPool(
	address token0,
	address token1,
	uint24 fee,
	bytes32 salt
) external returns (PanopticPool newPoolContract) {
	...
	address _owner = s_owner;
	if (_owner != address(0) && _owner != msg.sender)  //<------ @audit
		revert Errors.NotOwner();
	
	IUniswapV3Pool v3Pool = 
		IUniswapV3Pool(UNIV3_FACTORY.getPool(token0, token1, fee)); //<----- @audit
	...
	if (address(s_getPanopticPool[v3Pool]) != address(0))  //<------ @audit
		revert Errors.PoolAlreadyInitialized();
	...
	s_getPanopticPool[v3Pool] = newPoolContract;    //<------ @audit
}

Impact

1. If s_owner was initialized with invalid value, the ownership of PanopticFactory can't be transferred.
2. If s_owner was assigned to zero addres and PanopticFactory#deployNewPool() was called with the all available token0, token1, fee pairs by attacker, the normal user can't deploy new pool any more.

Proof of Concept

1. Attacker, Bob, front runs the normal owner, Alice, 's Panoptic#initialize() transaction with zero address.
2. Bob' calls PanopticFactory#deployNewPool() with the all available token0, token1, fee pairs and salt same as his address.
3. Alice can't transfer ownership of factory with Panoptic#transferOwnership().
4. Alice can't deploy new pool since Bob already filled s_getPanopticPool mapping variable with all available value.

function deployNewPool(
	address token0,
	address token1,
	uint24 fee,
	bytes32 salt
) external returns (PanopticPool newPoolContract) {
	...		
	IUniswapV3Pool v3Pool = 
		IUniswapV3Pool(UNIV3_FACTORY.getPool(token0, token1, fee)); //<----- @audit
	...
	if (address(s_getPanopticPool[v3Pool]) != address(0))  //<------ @audit
		revert Errors.PoolAlreadyInitialized();
	...
	s_getPanopticPool[v3Pool] = newPoolContract;    //<------ @audit
}

Tools Used

Manual Review

Recommended Mitigation Steps

1. Since PanopticFactory is a non-proxied contract, please consider to use constructor for initializing s_owner instead of using PanopticFactory#initialize().

constructor(
	address _WETH9,
	SemiFungiblePositionManager _SFPM,
	IUniswapV3Factory _univ3Factory,
	IDonorNFT _donorNFT,
	address _poolReference,
	address _collateralReference
) {
	WETH = _WETH9;	
	...
	COLLATERAL_REFERENCE = _collateralReference;
++	s_owner = msg.sender;
++	s_initialized = true;	
}

2. Please update the caller check logic of PanopticFactory#deployNewPool() as following.

function deployNewPool(
	address token0,
	address token1,
	uint24 fee,
	bytes32 salt
) external returns (PanopticPool newPoolContract) {
	...
	address _owner = s_owner;
--	if (_owner != address(0) && _owner != msg.sender)
++	if (_owner != msg.sender)
		revert Errors.NotOwner();
	...
}

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #523

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)