deploy() does not revert if contract deployment failed

Summary

Improper Validation of Create2 return value results in no deployment for that basepool ever again.

Vulnerability Detail

In PoolFactory.sol , the deploy() function, which is used to deploy contracts with the CREATE2 opcode, is as shown

PoolFactory.sol#L-50-L92

function deploy(address basePool, address underlying, PoolConfig calldata poolConfig)
        external
        override
        onlyOwner
        returns (address)
    {
        if (poolConfig.lnFeeRateRoot > MAX_LN_FEE_RATE_ROOT) revert Errors.LnFeeRateRootTooHigh();
        if (poolConfig.protocolFeePercent > MAX_PROTOCOL_FEE_PERCENT) revert Errors.ProtocolFeePercentTooHigh();
        if (poolConfig.initialAnchor < MIN_INITIAL_ANCHOR) revert Errors.InitialAnchorTooLow();

        address computedAddr = poolFor(basePool, underlying);
        if (_pools[computedAddr].underlying != address(0)) revert Errors.FactoryPoolAlreadyExists();

        address[3] memory pts = curveTricryptoFactory.get_coins(basePool);

        // Checklist:
        // 1. Base pool must be deployed by `CurveTricryptoFactory`.
        // 2. Underlying asset must be the same as the underlying asset of the principal tokens.
        // 3. Maturity of the principal tokens must be the same.
        uint256 maturity = ITranche(pts[0]).maturity();
        if (maturity != ITranche(pts[1]).maturity() || maturity != ITranche(pts[2]).maturity()) {
            revert Errors.FactoryMaturityMismatch();
        }
        if (
            ITranche(pts[0]).underlying() != underlying || ITranche(pts[1]).underlying() != underlying
                || ITranche(pts[2]).underlying() != underlying
        ) revert Errors.FactoryUnderlyingMismatch();

        // Set temporary variable
        _tempArgs = InitArgs({
            assets: PoolAssets({basePool: basePool, underlying: underlying, principalTokens: pts}),
            configs: poolConfig
        });
        // Deploy pool and temporary variable is read by callback from the pool
        address pool = address(Create2PoolLib.deploy(basePool, underlying));
        _pools[pool] = _tempArgs.assets;

        // Reset temporary variable to 0-value
        delete _tempArgs;

        emit Deployed(basePool, underlying, pool);
        return pool;
    }

The create2 opcode returns address(0) if contract deployment reverted. However, as seen from above, deploy() does not check if the deployment address is address(0).

This is an issue as deploy will not revert when the deployment of pool contract fails then we cannot deploy the pool for that basepool ever again because we are checking at line 61 that the underlying should be address(0) before the deployment but because of no revert we are setting this non zero value here.

Impact

Since deploy() does not revert when the creation of the Pool contract fails, users cannot be able to interact with that basepool ever.

Additionally, deploy() will not be callable for the same basepool , asset, thus protocol can never deploy a pool with these parameters.

Code Snippet

Code

Tool used

Manual Review

Recommendation

In deploy(), consider checking if the deployment address is address(0), and if yes revert


   function deploy(address basePool, address underlying, PoolConfig calldata poolConfig)
        external
        override
        onlyOwner
        returns (address)
    {
        if (poolConfig.lnFeeRateRoot > MAX_LN_FEE_RATE_ROOT) revert Errors.LnFeeRateRootTooHigh();
        if (poolConfig.protocolFeePercent > MAX_PROTOCOL_FEE_PERCENT) revert Errors.ProtocolFeePercentTooHigh();
        if (poolConfig.initialAnchor < MIN_INITIAL_ANCHOR) revert Errors.InitialAnchorTooLow();

        address computedAddr = poolFor(basePool, underlying);
        if (_pools[computedAddr].underlying != address(0)) revert Errors.FactoryPoolAlreadyExists();

        address[3] memory pts = curveTricryptoFactory.get_coins(basePool);

        // Checklist:
        // 1. Base pool must be deployed by `CurveTricryptoFactory`.
        // 2. Underlying asset must be the same as the underlying asset of the principal tokens.
        // 3. Maturity of the principal tokens must be the same.
        uint256 maturity = ITranche(pts[0]).maturity();
        if (maturity != ITranche(pts[1]).maturity() || maturity != ITranche(pts[2]).maturity()) {
            revert Errors.FactoryMaturityMismatch();
        }
        if (
            ITranche(pts[0]).underlying() != underlying || ITranche(pts[1]).underlying() != underlying
                || ITranche(pts[2]).underlying() != underlying
        ) revert Errors.FactoryUnderlyingMismatch();

        // Set temporary variable
        _tempArgs = InitArgs({
            assets: PoolAssets({basePool: basePool, underlying: underlying, principalTokens: pts}),
            configs: poolConfig
        });
        // Deploy pool and temporary variable is read by callback from the pool
        address pool = address(Create2PoolLib.deploy(basePool, underlying));
+        if (pool == address(0)) {
+              revert PoolFactory__deployUnsuccessful();
+         }     
        _pools[pool] = _tempArgs.assets;

        // Reset temporary variable to 0-value
        delete _tempArgs;

        emit Deployed(basePool, underlying, pool);
        return pool;
    }

2. We can add a check whether the `Deployed Address` is same as `Computed Address` which we have implemented here

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/TrancheFactory.sol#L87

sherlock-audit / 2024-01-napier-judging

Atharv - deploy() does not revert if contract deployment failed #10