Mitigation Confirmed for M-08

[code423n4]

Mitigation of M-08: Issue mitigated

Issue

M-08: Loss of funds for traders due to accounting error in royalty calculations

Mitigation

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

Assessment of Mitigation

Issue mitigated

Comments

After combining more commits with this mitigation, the following PrivatePool.buy function is currently updated to only increase netInputAmount by royaltyFee if the corresponding NFT's royaltyFee is positive and royalty recipient is not address(0).

If the base token is an ERC20 token, ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount) would be executed before such netInputAmount is increased by royaltyFee, and ERC20(baseToken).safeTransferFrom(msg.sender, recipient, royaltyFee) would be executed, where recipient is not address(0). These operations will not lock any ERC20 tokens from msg.sender as unused royalty fees in the PrivatePool contract.

If the base token is ETH, it is okay if msg.sender sends a msg.value that is more than netInputAmount, which does include the relevant royaltyFee. Because of if (msg.value > netInputAmount) msg.sender.safeTransferETH(msg.value - netInputAmount), msg.value - netInputAmount will be refunded to msg.sender. In this case, only netInputAmount is spent in which netInputAmount would only contain all used royaltyFee that are sent to all royalty recipient, which are not address(0). Thus, no ETH will be locked as unused royalty fees in the PrivatePool contract.

Therefore, the corresponding issue is mitigated in the PrivatePool.buy function.

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);

        // check that the caller sent 0 ETH if the base token is not ETH
        if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount();

        ...

        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);
        }

        ...
    }

Moreover, the following PrivatePool.sell function is currently updated to only increase royaltyFeeAmount by royaltyFee if the corresponding NFT's royaltyFee is positive and royalty recipient is not address(0). Because royaltyFeeAmount includes all royaltyFee that are sent to all relevant royalty recipient, royaltyFeeAmount is all used. Then, netOutputAmount -= royaltyFeeAmount would cause netOutputAmount to exclude all used royaltyFee. Afterwards, executing ERC20(baseToken).transfer(msg.sender, netOutputAmount) if the base token is an ERC20 token and msg.sender.safeTransferETH(netOutputAmount) if the base token is ETH would send the number of tokens, which msg.sender is entitled to, to msg.sender. In this case, no ERC20 tokens or ETH would be locked as unused royalty fees in the PrivatePool contract. Hence, the corresponding issue is also mitigated in the PrivatePool.sell function.

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

    function sell(
        uint256[] calldata tokenIds,
        uint256[] calldata tokenWeights,
        MerkleMultiProof calldata proof,
        IStolenNftOracle.Message[] memory stolenNftProofs // put in memory to avoid stack too deep error
    ) public returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount) {
        ...

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

        // calculate the net output amount and fee amount
        (netOutputAmount, feeAmount, protocolFeeAmount) = sellQuote(weightSum);

        ...

        uint256 royaltyFeeAmount = 0;
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // transfer each nft from the caller
            ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);

            if (payRoyalties) {
                // calculate the sale price (assume it's the same for each NFT even if weights differ)
                uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;

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

                // transfer the royalty fee to the recipient if it's greater than 0
                if (royaltyFee > 0 && recipient != address(0)) {
                    // tally the royalty fee amount
                    royaltyFeeAmount += royaltyFee;

                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }
            }
        }

        // subtract the royalty fee amount from the net output amount
        netOutputAmount -= royaltyFeeAmount;

        if (baseToken == address(0)) {
            // transfer ETH to the caller
            msg.sender.safeTransferETH(netOutputAmount);

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) factory.safeTransferETH(protocolFeeAmount);
        } else {
            // transfer base tokens to the caller
            ERC20(baseToken).transfer(msg.sender, netOutputAmount);

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

        ...
    }

[c4-judge]

GalloDaSballo marked the issue as satisfactory