sell function will not work as expected when royalty fee is high

[code423n4]

Lines of code

https://github.com/outdoteth/caviar-private-pools/blob/main/src/PrivatePool.sol#L307-L382

Vulnerability details

Impact

sell function will revert when royalty fee is high.

Proof of Concept

When user sell nft, then pool fee and factory fee is applied to the price. In case if user should receive 500 tokens for his nfts, then he receives 500 token minus fees. Private pool fee is 50% maximum, while protocol fee is 5% maximum, so together maximum 55% fee is possible. We should just remember right now, that maximum fee in the pool is 55%.

Now, lets check how royalty is distributed. uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length; salePrice is amount that user should receive + fees divided by tokens amount. This salePrice is maximum royalty amount that can be paid. So for example if royalty is 100%, that means that whole netOutputAmount + feeAmount + protocolFeeAmount amount should be paid as royaties. All royaties are accumulated into royaltyFeeAmount variable. And later, netOutputAmount, which is money that user should receive is decreased by royaltyFeeAmount. In this moment we can receive underflow error in several cases.

Now, i would like to provide example as it's more simple for me to explain in this way. Suppose that user wants to sell 5 nft that costs 100 tokens each. Pool fee is 20% and protocol fee is 5%, together is 25%. Approx amounts will be: netOutputAmount + feeAmount + protocolFeeAmount == 500, feeAmount == 100 and protocolFeeAmount == 25, netOutputAmount == 375. So according to the formula salePrice == 500/5 == 100. And suppose that royalty fee is 80%. that means that royalty is royaltyFeeAmount = 80 * 5 = 400 And then this line netOutputAmount -= royaltyFeeAmount; in the sell function will underflow as netOutputAmount == 375. This line means for us that 100% of royalties should be equal to netOutputAmount, but it's not like that now. In case if we will use maximum fee which 50% + 5%, then even royalty 46% will fail.

I think that this is a bug, because in this case noone is cheating, it's just such situation in which pool can't work normally.

Tools Used

VsCode

Recommended Mitigation Steps

I think that maybe netOutputAmount should be considered as maximum royalties payment(in this case if 100% royalties exists, then maximum netOutputAmount will be paid). In this case salePrice should be uint256 salePrice = netOutputAmount / tokenIds.length;.

Otherwise, you should check each nft's royalty percentage before you add it to the pool.

Assessed type

Invalid Validation

[c4-sponsor]

outdoteth marked the issue as sponsor acknowledged

[outdoteth]

It's very unlikely in practice that royalty fees are greater than 45%.

[GalloDaSballo]

The finding requires the royalty to be higher than 45% which is possible but an edge case in my opinion

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

QA seems more appropriate

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

Would rate as Low Severity, closing for awarding