Rounding on division in changeFeeQuote() may cause Pool owners to mistakenly or by purpose avoid paying the protocol fee

[code423n4]

Lines of code

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

Vulnerability details

Impact

Depending on how the PrivatePool is set-up (initialized) the owner of the pool may mistakenly or by purpose avoid paying protocol fees on the change NFT functionality - change() function. The same issue applies to the pool fee. Please note that this is easier or rather will be more common if a baseToken with fewer decimals is used. We'll pick USDC in this case which is currently the second most popular stablecoin. USDC is currently configured with 6 decimals.

Proof of Concept

Let's expand how the protocolFeeAmount is calculated in changeFeeQuote(). This issue is more apparent when using baseToken with fewer decimals, so we will use USDC which has 6 decimals. In this case the calculated exponent is 2.

The following legend will be used for brevity:
exponent $= 2$
feePerNft $= feePN$
changeFee $= chFee$
feeAmount $= fee$
inputAmount $= inpt$
protocolFeeAmount $= protoFeeAmnt$
protocolFeeRate $= protoFeeRate$

$feePN = chFee \times 10^2$
$fee = \frac{inpt \times feePN}{10^{18}} \Rightarrow \frac{inpt \times chFee \times 100}{10^{18}}$
$protoFeeAmnt = \frac{fee \times protoFeeRate}{10000} \Rightarrow \frac{\frac{inpt \times chFee \times 100}{10^{18}} \times protoFeeRate}{10000} \Rightarrow \frac{inpt \times chFee \times \cancel{100} \times protoFeeRate}{10^{\cancel{22}20}} \Rightarrow \frac{inpt \times chFee \times protoFeeRate}{10^{20}}$

When the numerator is less than the denominator the result will be rounded down to 0, which means the protocolFeeAmount will not be paid.

So this happens whenever this equation is satisfied:
$inpt \times chFee \times protoFeeRate < 10^{20}$

changeFee is an absolute value specified with 4 decimals precision and heavily depends on the selected baseToken, let's assume in case of USDC that changeFee is somewhere in the range of 10000 to 1000000 (1 to 100 USDC). protocolFeeRate is in basis points, for brevity let's assume it is 100 (1.0%). The overwhelmingly important factor here is the inputAmount which is dependant on the tokenWeights that is arbitrarily set by the pool owner. Let's put in the numbers (using 100 USDC for change fee):
$inpt \times chFee \times protoFeeRate < 10^{20}$
$inpt \times 1000000 \times 100 < 10^{20}$
$inpt < 10^{12}$

So in this scenario if the NFT token weight is less than $10^{12}$ the protocol fee on change() is 0. The similar issue exists for feeAmount that goes to the pool owner, but in that case the owner of the pool could correct the contract settings and provide new NFT weights to rectify the scenario, while the protocolFeeRate is global across all created private pools.

Note that this is easier achieved with baseToken with fewer decimals because of the smaller exponent in the equation, hence using USDC as an example. Also note we're using a single NTF change scenario for simplifying the showcase.

Let's verify this with a simple test (using the project test suite):

diff --git a/test/PrivatePool/Change.t.sol b/test/PrivatePool/Change.t.sol
index 558a9d1..bd4197d 100644
--- a/test/PrivatePool/Change.t.sol
+++ b/test/PrivatePool/Change.t.sol
@@ -2,6 +2,11 @@
 pragma solidity ^0.8.19;
 
 import "../Fixture.sol";
+import {ERC20} from "solmate/tokens/ERC20.sol";
+
+contract MockUSDC is ERC20 {
+    constructor() ERC20("mockUSDC", "USDC", 6) { }
+}
 
 contract ChangeTest is Fixture {
     event Change(
@@ -50,6 +55,24 @@ contract ChangeTest is Fixture {
         milady.setApprovalForAll(address(privatePool), true);
     }
 
+    function test_ProtocolFeeIssue() public {
+        // Using the mockUSDC, which has 6 decimals just like the real USDC
+        ERC20 mockUSDC = new MockUSDC();
+        PrivatePool privatePoolUSDC = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
+        // Change fee is 100 USDC
+        privatePoolUSDC.initialize(
+            address(mockUSDC), address(milady), 10e18, 10e18, 1000000, 1999, merkleRoot, true, false
+        );
+        // Setting the protocol fee to 1%
+        factory.setProtocolFeeRate(100);
+
+        // get the change fee quotes for a single NFT change with weights that aren't normalized to 1e18
+        // Anything less than 1e12 will result in 0 protocolFeeAmount even though protocol fee is set to 1%
+        (uint256 feeAmount, uint256 protocolFeeAmount) = privatePoolUSDC.changeFeeQuote(1e12 - 1);
+
+        assertGt(protocolFeeAmount, 0, "Should quote a protocol fee greater than 0");
+    }
+
     function test_EmitsChangeEvent() public {
         // arrange
         inputTokenIds.push(0);

Play around with the input of privatePoolUSDC.changeFeeQuote() and notice the test will succeed on input 1e12, but fail with anything less.

Tools Used

Manual review.

Recommended Mitigation Steps

Some mitigation ideas:

1. Document that the NFT weighs must be normalized to 1e18. This would at least save honest misconfigurations.
2. You could revert the transaction if the Protocol fee rate is greater than 0 but the protocolFeeAmount ends up 0. This would force the pool owner to update the weights according to the documentation (see Agreements & Disclosures #1)
3. Or require that inputAmount in changeFeeQuote() is equal or greater than 1e18.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

outdoteth marked the issue as disagree with severity

[outdoteth]

not sure if this should be a low or not

[c4-sponsor]

outdoteth requested judge review

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Fees will round down to zero due to integer math, I believe this is Low Severity
The fact that this can be done by the owner further reduces impact, but I believe it's still QA - Low

[indijanc]

Leaving a fact based comment as I believe the impact and likelihood was missed or rather not properly presented in the issue.

• Pool owners and Protocol owner incentives are not aligned when it comes to the Protocol fee. Pool owners benefit if they can avoid the Protocol fee as their pool will be cheaper to use, gaining advantage or an edge over honest Pool owners and more revenue for them at the cost of Protocol revenue

• Protocol owner is designed to decide and set the Protocol fee, without Pool owners ability to avoid it. This issue demonstrates that in fact Pool owners are able to avoid the Protocol fee for the change NFT functionality using the NFT weights that they set on the Pool

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

Thank you for the feedback @indijanc I still believe that a rounding error is Low Severity as the loss is marginal, I think quality was good and believe if you sent more this would have been awarded as QA

[GalloDaSballo]

Also note that inputAmount is normalized in 1e18s meaning that this should not happen unless it's misconfigured