Mitigation Confirmed for H-01

[code423n4]

Mitigation of H-01: Issue mitigated

Issue

H-01: Royalty receiver can drain a private pool

Mitigation

https://github.com/outdoteth/caviar-private-pools/pull/12

Assessment of Mitigation

Issue mitigated

Comments

The root cause of the corresponding issue is that the old PrivatePool.buy function has two loops that iterate over each NFT, calls _getRoyalty(tokenIds[i], salePrice) within each loop, and executes msg.sender.safeTransferETH(msg.value - netInputAmount) between these two loops that allows the malicious actor to change royaltyFee.

After combining more commits with this mitigation, the following PrivatePool.buy function is now updated to have only one loop that iterates over each NFT, calls _getRoyalty(tokenIds[i], salePrice) within this only loop, and executes netInputAmount += royaltyFee and recipient.safeTransferETH(royaltyFee) if royaltyFee > 0 && recipient != address(0) is true within the only loop. Then, the PrivatePool.buy function executes if (msg.value < netInputAmount) revert InvalidEthAmount() and msg.sender.safeTransferETH(msg.value - netInputAmount) after the only loop is done. Based on the current code structure, even if the malicious actor changes royaltyFee when executing msg.sender.safeTransferETH(msg.value - netInputAmount), such change does not matter because all of the correct royaltyFee have already been sent to all of the applicable royalty recipient, and netInputAmount has already been increased by all of the correct royaltyFee; then, if (msg.value < netInputAmount) revert InvalidEthAmount() ensures that msg.sender must send enough ETH that covers netInputAmount when calling the PrivatePool.buy function.

If the base token is an ERC20 token, before entering the the only loop, ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount) would be executed before such netInputAmount is increased by royaltyFee so the prices of the NFTs, feeAmount, and protocolFeeAmount are all sent from msg.sender to the PrivatePool contract. Within the only loop, ERC20(baseToken).safeTransferFrom(msg.sender, recipient, royaltyFee) is executed so msg.sender directly pays royaltyFee to the royalty recipient if royaltyFee > 0 && recipient != address(0) is true. In this case, the PrivatePool contract does not send any royaltyFee to the royalty recipient.

Hence, the corresponding issue is mitigated.

https://github.com/outdoteth/caviar-private-pools/blob/main/src/PrivatePool.sol#L221-L294

    function buy(uint256[] calldata tokenIds, uint256[] calldata tokenWeights, MerkleMultiProof calldata proof)
        public
        payable
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        ...

        // calculate the sum of weights of the NFTs to buy
        uint256 weightSum = sumWeightsAndValidateProof(tokenIds, tokenWeights, proof);

        // calculate the required net input amount and fee amount
        (netInputAmount, feeAmount, protocolFeeAmount) = buyQuote(weightSum);

        ...

        if (baseToken != address(0)) {
            // transfer the base token from the caller to the contract
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount);

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(factory, protocolFeeAmount);
        }

        // calculate the sale price (assume it's the same for each NFT even if weights differ)
        uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
        uint256 royaltyFeeAmount = 0;
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // transfer the NFT to the caller
            ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);

            if (payRoyalties) {
                // get the royalty fee for the NFT
                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

                if (royaltyFee > 0 && recipient != address(0)) {
                    // add the royalty fee amount to the net input amount
                    netInputAmount += royaltyFee;

                    // transfer the royalties to the recipient
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransferFrom(msg.sender, recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }
            }
        }

        if (baseToken == address(0)) {
            // check that the caller sent enough ETH to cover the net required input
            if (msg.value < netInputAmount) revert InvalidEthAmount();

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) factory.safeTransferETH(protocolFeeAmount);

            // refund any excess ETH to the caller
            if (msg.value > netInputAmount) msg.sender.safeTransferETH(msg.value - netInputAmount);
        }

        ...
    }

[c4-judge]

GalloDaSballo marked the issue as satisfactory