Caviar Private Pools - Bason's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L274 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L338

Vulnerability details

Impact

Imagine we have 3 NFT bundled together with prices 10ETH, 5ETH, 6ETH, the average sale price is 7ETH ((netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length) With the current implementation the royalty fee will be calculated on the average sale price of 7 ETH. Even though the percentage will be correct the actual sum will be incorrect. The royalty fees must be calculated based on the Nft Weight. This is an issue with both buy and sell functions

The creator of the more expensive NFTs will receive less royalties and the creators of the cheaper ones - more royalties leading to unfair loss and unfair gain.

Proof of concept

At line 236 we calculate an average price for each NFT in a buy transaction uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;

At line 338 we calculate the royalty fee and receipent by calling getRoyalty (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice); // tally the royalty fee amount royaltyFeeAmount += royaltyFee;

Then we transfer the royalty fee.

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

We can see that the royalty fee is actually based on the average NFT price instead of their real price.

Recommended Mitigation Steps

Introduce an nftWeight parameter to properly calculate the royaltyFee and match it with the tokenWeights array parameter passed to the buy and sell functions. The mitigation is applicable for both functions.

for (uint256 i = 0; i < tokenIds.length; i++) {
    royaltyFee = royaltyFee * tokenWeights[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;
            }
        }