Caviar Private Pools - CodingNameKiki's results

Lines of code

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

Vulnerability details

Impact

Duo to the pool's assumption of the sale price per NFT, it's possible for recipients to receive less or more royalty fee based on the scenario.

Proof of Concept - Overview

The given example shows how a recipient receives less royalty fee than he is supposed to, duo to the system wrongfully assuming the sale price per NFT. In the opposite case it's possible for a recipient to receive more royalty fee, therefore the problem is clearly in the accounting of the sale price.

Let's say a user wants to buy two NFTs from the private pool.

NFT A - 3e18 weight, NFT B - 1e18 weight.

NFT A has 40% royalty fee of the sale price.

NFT B has 0% royalty fee of the sale price.

Technically 40% of NFT A sale price should be paid, but the function sums the weights of the two NFTs. And as you can see below the salePrice is calculated by assuming it's the same for each NFT, which is wrong and leads to the wrong accounting of the royalty fee.

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

A simple example:

NFT A - 3e18 weight - 40% royalty fee applied
NFT B - 1e18 weight. - 0% royalty fee

The function should get the 40% of NFT A's weight, but this leads to diff outcome

uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
uint256 salePrice = (4e18 - 0 - 0) / 2;
uint256 salePrice = 2e18

You can see above that the assumed price per NFT is 2e18, therefore the function 
"_getRoyalty" will get 40% of the 2e18 sale price and not from the right one which should be 3e18.

This problem leads to recipients receiving less royalty fee.

Proof of Concept

By far l described the problem occurring, but lets see how much less royalty fee a user gets.

Lets take for example first, that the sale price is calculated separately:

We have two NFTs, A and B.

A has a sale price of 3e18 and 40% royalty fee.

B has a sale price of 1e18 and 0% royalty fee.

The outcome in the function "royaltyInfo" will look like this:

// Fee for NFT A is set to 40%, which is 4000

uint256 royaltyAmount = (salePrice * royalty.royaltyFraction) / _feeDenominator();
uint256 royaltyAmount = (3e18 * 4000) / 10000;
uint256 royaltyAmount = 1200000000000000000

// The royalty fee for NFT A is 1200000000000000000.

Lets go over the second example, which is how the private pool works.

We have two NFTs, A and B.

A has a sale price of 3e18 and 40% royalty fee.

B has a sale price of 1e18 and 0% royalty fee.

We sum the sale price for these two NFTs and divide it by two. Therefore both NFTs will have a sale price of 2e18.

// Fee for NFT A is set to 40%, which is 4000

uint256 royaltyAmount = (salePrice * royalty.royaltyFraction) / _feeDenominator();
uint256 royaltyAmount = (2e18 * 4000) / 10000;
uint256 royaltyAmount = 800000000000000000

// The royalty fee for NFT A is 800000000000000000.

You can clearly see the difference between the two scenarios and how much less royalty fee a user gets, by the assumption of the sale price per NFT in the system.

Tools Used

Manual review

Recommended Mitigation Steps

Consider calculating the royalty fee based on the real sale price for the NFT and not on the assumption of it, as duo to the issue a recipient can get less or more royalty fee depending on the scenario occurring.

Lines of code

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

Vulnerability details

Impact

Private pools initialized with zero changeFee won't be able to pay the protocolFee, as the calculation of the protocol fee is based on the changeFee, therefore the private pools won't pay any protocol fee duo to multiplication with zero.

Proof of Concept

Users are able to create a private pool however they like, therefore there might be some private pools charging zero change fee prior to calling the function "change".

The following snippet below calculates the feeAmount and the protocolFeeAmount accrued from the change. We can see that both the feeAmount and protocolFeeAmount is based on the changeFee state variable, which shouldn't be the case here. As private pool initialized with zero changeFee won't only not pay the feeAmount, but will skip the protocolFee as well.

uint256 feePerNft = changeFee * 10 ** exponent;
uint256 feePerNft = 0 * 10 ** exponent;
uint256 feePerNft = 0;

feeAmount = (inputAmount * feePerNft) / 1e18;
feeAmount = (inputAmount * 0) / 1e18;
feeAmount = 0;

protocolFeeAmount = (feeAmount * Factory(factory).protocolFeeRate()) / 10_000;
protocolFeeAmount = (0 * Factory(factory).protocolFeeRate()) / 10_000;
protocolFeeAmount = 0;

    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).protocolFeeRate()) /
            10_000;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Hard to think regarding a fix here, since the calculation of the protocolFee depends on the changeFee. Perhaps there should be a hardcoded amount for use, whenever the pool is initialized with zero changeFee.

E.g:

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

        if(changeFee != 0){
           uint256 feePerNft = changeFee * 10 ** exponent;
           feeAmount = (inputAmount * feePerNft) / 1e18;
           protocolFeeAmount =
               (feeAmount * Factory(factory).protocolFeeRate()) /
                10_000;

           return (feeAmount, protocolFeeAmount);
        else {
           uint256 feePerNft = hardcodedAmount * 10 ** exponent;
           feeAmount = (inputAmount * feePerNft) / 1e18;
           protocolFeeAmount =
               (feeAmount * Factory(factory).protocolFeeRate()) /
                10_000;
           
           return (0, protocolFeeAmount);
       }
    }

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

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