Caviar Private Pools - T1MOH's results

Lines of code

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

Vulnerability details

Impact

Function change() will revert if token's decimals is less than 4, which blocks using this function

Proof of Concept

        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;

Tools Used

Manual Review

Recommended Mitigation Steps

Handle this case when token's decimals is less than 4

Lines of code

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

Vulnerability details

Impact

Royalties paid for nft in single buy/sell can differ from batch, making it unfair for royalty receivers. That's not only unfair distribution, it can be less than intended.

Proof of Concept

1)Suppose there are 3 tokens A B C. A has royalty 5%, B has royalty 10%, C has royalty 20% 2)Suppose total pool weight is very big for ease calculations to exclude slippage. 3)A has weight = 1, B has weight = 3, C has weight = 10. 4)1 weight costs 100 ETH. 5)Paid royalty should be following (and they will be following if call 3 times buy with single tokenId). A: 100 * 0.05 = 5 ETH; B: 100 * 3 * 0.10 = 30 ETH; C: 100 * 10 * 0.20 = 200 ETH; Remember amounts 6)But according to formula salePrice will be (100 + 100 * 3 + 100 * 10) / 3 = 466 ETH 7)Total actual royalty will be the next. A: 466 * 0.05 = 23.3 ETH; B: 466 * 0.10 = 46.6 ETH; C: 466 * 0.20 = 93.2 ETH TotalPaid is 23.3 + 46.6 + 93.2 = 163.1 ETH. What is much less

That's unfair formula used in code

        // calculate the sum of weights of the NFTs to sell
        uint256 weightSum = sumWeightsAndValidateProof(tokenIds, tokenWeights, proof);

        // calculate the net output amount and fee amount
        (netOutputAmount, feeAmount, protocolFeeAmount) = sellQuote(weightSum);
        ...
        uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;

        // get the royalty fee for the NFT
        (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

Tools Used

Manual Review

Recommended Mitigation Steps

Pass to _getRoyalty() actual sum according weight of this nft, don't assume all nfts have the same salePrice