royaltyFee incorrectly determined in PrivatePool

[code423n4]

Lines of code

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

Vulnerability details

Impact

The protocol calculate the sale price by assuming it's the same for each NFT even if weights differ. This assumption is deemed incorrect and leads to unfair fee distribution to recipients.

Proof of Concept

Here is the scenario:

1. NFT Agreements & Disclosures #1 has a weight of 2.0 where as NFT QA Report #2 has a weight of 1.0, and both NFTs sit in the pool.
2. If NFT Agreements & Disclosures #1 were bought alone, buyQuote() would be based on a weight of 2.0e18 to return netInputAmount and subsequently be used to correspondingly determine salePrice and royaltyFee.
3. If NFT Agreements & Disclosures #1 and QA Report #2 were bought, buyQuote() would be based on a weight of 3.0e18 to return netInputAmount and subsequently be used to evenly determine salePrice and royaltyFee:

File: 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 a result, recipient #1 suffers a fee cut considering her NFT weight was treated as 1.5 instead of 2.0:

File: PrivatePool.sol#L271-L284

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

4. Recipient QA Report #2 would on the hand receive a higher cut of fee with her NFT weight based on 1.5 instead of 1.0 at the expense of recipient Agreements & Disclosures #1's fee.

The impact will be increasingly significant if the weight difference is larger and involves more NFTs in bulk purchase or sale.

Tools Used

Manual

Recommended Mitigation Steps

Consider refactoring the affected arithmetic in bulk purchase and sale to correctly and fairly distribute royalties to the recipients.

For instance, the affected code line of sell() can be refactored as follows:

File: PrivatePool.sol#L335

-    uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;
+    uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) * tokenWeights[i] / weightSum;

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #669

[c4-judge]

GalloDaSballo marked the issue as satisfactory