Mitigation Confirmed for M-10

[code423n4]

Mitigation of M-10: Issue mitigated (Please see comments)

Issue

M-10: Incorrect protocol fee is taken when changing NFTs

Mitigation

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

Assessment of Mitigation

Issue mitigated (Please see comments)

Comments

After combining more commits with this mitigation, the following protocolChangeFeeRate and Factory.setProtocolChangeFeeRate functions are added in the Factory contract. Because of if (_protocolChangeFeeRate > 10_000) revert ProtocolChangeFeeRateTooHigh(), protocolChangeFeeRate can be set to up to 10_000. Based on the sponsor's comment, protocolChangeFeeRate could be set to a reasonable value, such as 3000 that is for 30%.

https://github.com/outdoteth/caviar-private-pools/blob/main/src/Factory.sol#L61-L62

    /// @notice The protocol change fee rate that is taken on each change/flash loan. It's in basis points: 200 = 2.00%.
    uint16 public protocolChangeFeeRate;

https://github.com/outdoteth/caviar-private-pools/blob/main/src/Factory.sol#L165-L171

    function setProtocolChangeFeeRate(uint16 _protocolChangeFeeRate) public onlyOwner {
        // check that the protocol change fee rate is not higher than 100%
        if (_protocolChangeFeeRate > 10_000) revert ProtocolChangeFeeRateTooHigh();

        protocolChangeFeeRate = _protocolChangeFeeRate;
        emit SetProtocolChangeFeeRate(_protocolChangeFeeRate);
    }

The following PrivatePool.changeFeeQuote and PrivatePool.flashFeeAndProtocolFee functions are also updated to execute protocolFeeAmount = feeAmount * Factory(factory).protocolChangeFeeRate() / 10_000, where Factory(factory).protocolChangeFeeRate() is used instead of Factory(factory).protocolFeeRate(). As shown by the Factory.setProtocolFeeRate function below, the highest possible protocolFeeRate would be 500 so protocolChangeFeeRate would be much higher than protocolFeeRate. This makes sense because protocolFeeAmount is a fee on top of feeAmount that is for the private pool, and changeFee that feeAmount depends on can be somewhat small; changing protocolFeeAmount to depend on protocolChangeFeeRate instead of protocolFeeRate can make protocolFeeAmount not as small as before. Thus, the corresponding issue is mitigated.

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

    function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        uint256 feePerNft = changeFee * 10 ** exponent;

        feeAmount = inputAmount * feePerNft / 1e18;
        protocolFeeAmount = feeAmount * Factory(factory).protocolChangeFeeRate() / 10_000;
    }

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

    function flashFeeAndProtocolFee() public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        feeAmount = changeFee * 10 ** exponent;
        protocolFeeAmount = feeAmount * Factory(factory).protocolChangeFeeRate() / 10_000;
    }

https://github.com/outdoteth/caviar-private-pools/blob/main/src/Factory.sol#L155-L161

    function setProtocolFeeRate(uint16 _protocolFeeRate) public onlyOwner {
        // check that the protocol fee rate is not higher than 5%
        if (_protocolFeeRate > 500) revert ProtocolFeeRateTooHigh();

        protocolFeeRate = _protocolFeeRate;
        emit SetProtocolFeeRate(_protocolFeeRate);
    }

Yet, it is worth to note that the PrivatePool.changeFeeQuote and PrivatePool.flashFeeAndProtocolFee functions are still not consistent with the following PrivatePool.buyQuote and PrivatePool.sellQuote functions. In the PrivatePool.buyQuote and PrivatePool.sellQuote functions, protocolFeeAmount is not a fee on top of feeAmount so feeRate set by the private pool owner does not affect protocolFeeAmount. In contrast, in the PrivatePool.changeFeeQuote and PrivatePool.flashFeeAndProtocolFee functions, changeFee set by the private pool owner can affect protocolFeeAmount in which protocolFeeAmount could be 0 if the private pool owner sets changeFee to 0. Although this appears to be a design choice, the sponsor is encouraged to further review the differences among the PrivatePool.changeFeeQuote, PrivatePool.flashFeeAndProtocolFee, PrivatePool.buyQuote and PrivatePool.sellQuote functions to determine whether these functions need to be consistent or not.

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

    function buyQuote(uint256 outputAmount)
        public
        view
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // calculate the input amount based on xy=k invariant and round up by 1 wei
        uint256 inputAmount =
            FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

        protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;
        feeAmount = inputAmount * feeRate / 10_000;
        netInputAmount = inputAmount + feeAmount + protocolFeeAmount;
    }

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

    function sellQuote(uint256 inputAmount)
        public
        view
        returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // calculate the output amount based on xy=k invariant
        uint256 outputAmount = inputAmount * virtualBaseTokenReserves / (virtualNftReserves + inputAmount);

        protocolFeeAmount = outputAmount * Factory(factory).protocolFeeRate() / 10_000;
        feeAmount = outputAmount * feeRate / 10_000;
        netOutputAmount = outputAmount - feeAmount - protocolFeeAmount;
    }

[c4-judge]

GalloDaSballo marked the issue as satisfactory