Caviar Private Pools - MiloTruck's results

Lines of code

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

Vulnerability details

EIP-2981, the NFT Royalty Standard, states:

The implementer MAY choose to change the percentage value based on other predictable variables that do not make assumptions about the unit of exchange.

One more reasonable approach MAY use the number of transfers of an NFT to decide which percentage value is used to calculate the royaltyAmount. The idea being that the percentage value could decrease after each transfer of the NFT.

Another example could be using a different percentage value for each unique _tokenId.

As seen from above, contracts that implement EIP-2981 might return different royalty fees when the royaltyInfo() function is called:

1. NFTs with different token IDs might have different royalty fees.
2. Royalty fees might change based on the current number of transfers of an NFT.

However, the way this protocol implements the calculation of royalty fees does not account for the above, making it non-compliant with EIP-2981.

1. NFTs with different token IDs might have different royalty fees.

Vulnerability Details

In the buy() function of the PrivatePool contract, the royalty fee amount is calculated as such:

PrivatePool.sol#L235-L249

// 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,) = _getRoyalty(tokenIds[i], salePrice);

        // add the royalty fee to the total royalty fee amount
        royaltyFeeAmount += royaltyFee;
    }
}

Where:

• tokenIds - Array containing the token IDs of each NFT being bought.
• netInputAmount - The total price of all NFTs.

In the code above, salePrice is taken as the average price of each NFT to be bought. Afterwards, the royaltyFee for each NFT transfer is calculated based on salePrice, instead of the actual price of each NFT.

However, this implementation would return an incorrect royaltyFeeAmount if:

1. The royalty fee implementation has different royalty fees for NFTs with different token IDs,
2. buy() is called with multiple NFTs that have different prices.

By calling _getRoyalty() with the average sale price of each NFT instead of its actual price, the royaltyFee returned for that NFT would be incorrect.

Proof of Concept

Consider a royalty fee implementation that has double the royalty fee for an NFT where tokenId == 100:

function royaltyInfo(uint256 tokenId, uint256 salePrice) public view override returns (address receiver, uint256 royaltyAmount) {
    receiver = address(0xbeefbeef);

    royaltyAmount = salePrice * royaltyFeeRate / 1e18;
    if (tokenId == 100) {
        royaltyAmount = 2 * salePrice * royaltyFeeRate / 1e18;
    }
}

If a user calls buy() with the following NFTs:

• tokenId = 100, price = 2e18
• tokenId = 1, price = 1e18

the average sale price, would be 15e17.

If each transfer has a 1% royalty fee (royaltyFeeRate = 1e16), the total royalty fee amount should be:

royaltyFeeAmount = (2 * 2e18 * 1e16 / 1e18) + (1e18 * 1e16 / 1e18) = 5e16

However, as buy() uses the average sale price to calculate royalty fees for each transfer, the total royalty fee amount becomes:

royaltyFeeAmount = (2 * 15e17 * 1e16 / 1e18) + (15e17 * 1e16 / 1e18) = 4.5e16

Thus, the royalty recipient receives less royalty fees than he is supposed to.

Recommended Mitigation

Consider calling _getRoyalty() with the actual price of each NFT being transferred, instead of the average price.

Note that this vulnerability is also present in the following functions:

• sell() in PrivatePool.sol.
• buy() in EthRouter.sol.
• sell() in EthRouter.sol.

2. Royalty fees might change based on the current number of transfers of an NFT.

Vulnerability Details

In the buy() function, _getRoyalty() is first called in a loop:

PrivatePool.sol#L235-L249

// 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,) = _getRoyalty(tokenIds[i], salePrice);

        // add the royalty fee to the total royalty fee amount
        royaltyFeeAmount += royaltyFee;
    }
}

As seen above, _getRoyalty() is called in a loop that transfers each NFT.

Later on, _getRoyalty() is called again when transferring royalty fees to the recipient:

PrivatePool.sol#L273-L283

// 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)) {
    if (baseToken != address(0)) {
        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
    } else {
        recipient.safeTransferETH(royaltyFee);
    }
}

However, the second call to _getRoyalty() might return a different royaltyFee for a royalty fee implementation that calculates the royalty fee based on the current number of transfers of an NFT. This would result in either:

1. The buy() function reverting due to an insufficient amount of baseToken.
2. The royalty recipient receiving less royalty fees than intended.

Proof of Concept

Consider a royalty fee implementation that increases royalty fees based on the total number of transfers of the NFT collection:

function royaltyInfo(uint256 tokenId, uint256 salePrice) public view override returns (address receiver, uint256 royaltyAmount) {
    receiver = address(0xbeefbeef);

    royaltyAmount = salePrice * royaltyFeeRate * totalTransferCount / 1e18;
}

Where:

• totalTransferCount - The total number of transfers of the NFT collection.

In the buy() function, when _getRoyalty() is called for the second time, totalTransferCount will be larger than the first call _getRoyalty(), resulting in a larger royaltyFee being returned.

Recommended Mitigation

Consider storing the royalty fee and recipient for each NFT from the first call to _getRoyalty(), and using these stored values when transferring baseToken to the recipient later on.

contract RoyaltyNFT is ERC721 {
    uint256 public totalTransferCount;

    constructor() ERC721("ExampleNFT", "eNFT") {}

    function royaltyInfo(
        uint256 tokenId,
        uint256 salePrice
    ) public view override returns (address receiver, uint256 royaltyAmount) {
        receiver = address(this);
        royaltyAmount = (salePrice * totalTransferCount) / 1000;
    }

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 firstTokenId,
        uint256 batchSize
    ) internal override {
        if (from != address(0) && to != address(0)) {
            ++totalTransferCount;
        }
    }
}

        // 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,) = _getRoyalty(tokenIds[i], salePrice);

                // add the royalty fee to the total royalty fee amount
                royaltyFeeAmount += royaltyFee;
            }
        }

        if (payRoyalties) {
            for (uint256 i = 0; i < tokenIds.length; i++) {
                // 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)) {
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }
            }
        }

(1 ether * 1 / 1000) + (1 ether * 2 / 1000) = 3000000000000000

(1 ether * 2 / 1000) + (1 ether * 2 / 1000) = 4000000000000000