Caviar Private Pools - J4de's results

Lines of code

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

Vulnerability details

Impact

Take the PrivatePool contract as an example.

File: PrivatePool.sol
333             if (payRoyalties) {
334                 // calculate the sale price (assume it's the same for each NFT even if weights differ)
335                 uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;
336
337                 // get the royalty fee for the NFT
338                 (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);
339
340                 // tally the royalty fee amount
341                 royaltyFeeAmount += royaltyFee;
342
343                 // transfer the royalty fee to the recipient if it's greater than 0
344                 if (royaltyFee > 0 && recipient != address(0)) {
345                     if (baseToken != address(0)) {
346                         ERC20(baseToken).safeTransfer(recipient, royaltyFee);
347                     } else {
348                         recipient.safeTransferETH(royaltyFee);
349                     }
350                 }
351             }

When calculating royalties, the PrivatePool contract calculates the royalties of all NFTs based on the average price of NFTs. This is wrong, and the royalties should be calculated at the price of each NFT.

For example, there are 2 NFTs, one worth 1000 and one worth 200, their royalty rates are 2% and 1% respectively.

• According to the algorithm of the PrivatePool contract, the royalty is 600 * 2% + 600 * 1% = 18
• The correct royalty is 1000 * 2% + 200 * 1% = 22

In actual situations, the error may be greater, and users can avoid tax by purchasing some NFTs that are very cheap and have a low royalty rate.

This problem also exists in the EthRouter contract.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to calculate royalties according to the price of each NFT.

Lines of code

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

Vulnerability details

Impact

File: PrivatePool.sol
731     function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
732         // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
733         uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
734         uint256 feePerNft = changeFee * 10 ** exponent;
735
736         feeAmount = inputAmount * feePerNft / 1e18;
737 [1]->   protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
738     }

In code [1], protocolFeeAmount should be calculated based on inputAmount instead of feeAmount, which will result in much less protocolFeeAmount. Please refer to buyQuote and sellQuote for correct usage.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

    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;
+       protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;
    }