Prevent duplicate dynamic fee pools (#711)

* prevent duplicate dynamic fee pools * update natspec --------- Co-authored-by: Alice Henshaw <[email protected]>
Uniswap · May 24, 2024 · e624ffe · e624ffe
1 parent 5eb17c9
commit e624ffe
Show file tree

Hide file tree

Showing 13 changed files with 34 additions and 26 deletions.
diff --git a/.forge-snapshots/initialize.snap b/.forge-snapshots/initialize.snap
@@ -1 +1 @@
-61164
+61173
diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap
@@ -1 +1 @@
-19971
+20003
diff --git a/.forge-snapshots/swap CA custom curve + swap noop.snap b/.forge-snapshots/swap CA custom curve + swap noop.snap
@@ -1 +1 @@
-132021
+132045
diff --git a/.forge-snapshots/swap skips hook call if hook is caller.snap b/.forge-snapshots/swap skips hook call if hook is caller.snap
@@ -1 +1 @@
-217547
+217571
diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap
@@ -1 +1 @@
-138602
+138626
diff --git a/.forge-snapshots/swap with lp fee and protocol fee.snap b/.forge-snapshots/swap with lp fee and protocol fee.snap
@@ -1 +1 @@
-176594
+176624
diff --git a/.forge-snapshots/swap with return dynamic fee.snap b/.forge-snapshots/swap with return dynamic fee.snap
@@ -1 +1 @@
-152374
+152398
diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap
@@ -1 +1 @@
-155010
+155040
diff --git a/src/libraries/LPFeeLibrary.sol b/src/libraries/LPFeeLibrary.sol
@@ -11,7 +11,7 @@ library LPFeeLibrary {
     /// @notice Thrown when the static or dynamic fee on a pool exceeds 100%.
     error FeeTooLarge();
 
-    // the top bit of the fee in a PoolKey is used to signal if a Pool's LP fee is dynamic
+    // an lp fee of exactly 0b1000000... signals a dynamic fee pool. This isnt a valid static fee as it is > MAX_LP_FEE
     uint24 public constant DYNAMIC_FEE_FLAG = 0x800000;
 
     // the second bit of the fee returned by beforeSwap is used to signal if the stored LP fee should be overridden in this swap
@@ -26,7 +26,7 @@ library LPFeeLibrary {
 
     /// @notice returns true if a pool's LP fee signals that the pool has a dynamic fee
     function isDynamicFee(uint24 self) internal pure returns (bool) {
-        return self & DYNAMIC_FEE_FLAG != 0;
+        return self == DYNAMIC_FEE_FLAG;
     }
 
     /// @notice returns true if an LP fee is valid, aka not above the maxmimum permitted fee

diff --git a/src/types/PoolKey.sol b/src/types/PoolKey.sol
@@ -10,7 +10,7 @@ struct PoolKey {
     Currency currency0;
     /// @notice The higher currency of the pool, sorted numerically
     Currency currency1;
-    /// @notice The pool swap fee, capped at 1_000_000. The upper 4 bits determine if the hook sets any fees.
+    /// @notice The pool swap fee, capped at 1_000_000. If the first bit is 1, the pool has a dynamic fee and must be exactly equal to 0x800000
     uint24 fee;
     /// @notice Ticks that involve positions must be a multiple of tick spacing
     int24 tickSpacing;

diff --git a/test/PoolManagerInitialize.t.sol b/test/PoolManagerInitialize.t.sol
@@ -72,7 +72,7 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot {
         } else if (!key0.hooks.isValidHookAddress(key0.fee)) {
             vm.expectRevert(abi.encodeWithSelector(Hooks.HookAddressNotValid.selector, address(key0.hooks)));
             manager.initialize(key0, sqrtPriceX96, ZERO_BYTES);
-        } else if ((key0.fee & LPFeeLibrary.DYNAMIC_FEE_FLAG == 0) && (key0.fee > 1000000)) {
+        } else if ((key0.fee != LPFeeLibrary.DYNAMIC_FEE_FLAG) && (key0.fee > 1000000)) {
             vm.expectRevert(abi.encodeWithSelector(LPFeeLibrary.FeeTooLarge.selector));
             manager.initialize(key0, sqrtPriceX96, ZERO_BYTES);
         } else {

diff --git a/test/libraries/Hooks.t.sol b/test/libraries/Hooks.t.sol
@@ -987,17 +987,18 @@ contract HooksTest is Test, Deployers, GasSnapshot {
         assertTrue(
             Hooks.isValidHookAddress(IHooks(0x1000000000000000000000000000000000000000), LPFeeLibrary.DYNAMIC_FEE_FLAG)
         );
-        assertTrue(
-            Hooks.isValidHookAddress(
-                IHooks(0x1000000000000000000000000000000000000000), LPFeeLibrary.DYNAMIC_FEE_FLAG | uint24(3000)
-            )
-        );
     }
 
     function test_isValidHookAddress_invalid_noFlagsNoDynamicFee() public pure {
         assertFalse(Hooks.isValidHookAddress(IHooks(0x1000000000000000000000000000000000000000), 3000));
         assertFalse(Hooks.isValidHookAddress(IHooks(0x0001000000000000000000000000000000004000), 3000));
         assertFalse(Hooks.isValidHookAddress(IHooks(0x003840A85D5AF5bf1D1762F925BDaDdc42010000), 3000));
+        // not dynamic as another bit is dirty in the fee
+        assertFalse(
+            Hooks.isValidHookAddress(
+                IHooks(0x1000000000000000000000000000000000000000), LPFeeLibrary.DYNAMIC_FEE_FLAG | uint24(3000)
+            )
+        );
     }
 
     function test_callHook_revertsWithBubbleUp() public {

diff --git a/test/libraries/LPFeeLibrary.t.sol b/test/libraries/LPFeeLibrary.t.sol
@@ -10,18 +10,19 @@ contract LPFeeLibraryTest is Test {
         assertTrue(LPFeeLibrary.isDynamicFee(dynamicFee));
     }
 
-    function test_isDynamicFee_returnsTrue_forMaxValue() public pure {
+    function test_isDynamicFee_returnsFalse_forOtherValues() public pure {
         uint24 dynamicFee = 0xFFFFFF;
-        assertTrue(LPFeeLibrary.isDynamicFee(dynamicFee));
-    }
-
-    function test_isDynamicFee_returnsFalse() public pure {
-        uint24 dynamicFee = 0x7FFFFF;
+        assertFalse(LPFeeLibrary.isDynamicFee(dynamicFee));
+        dynamicFee = 0x7FFFFF;
+        assertFalse(LPFeeLibrary.isDynamicFee(dynamicFee));
+        dynamicFee = 0;
+        assertFalse(LPFeeLibrary.isDynamicFee(dynamicFee));
+        dynamicFee = 0x800001;
         assertFalse(LPFeeLibrary.isDynamicFee(dynamicFee));
     }
 
     function test_fuzz_isDynamicFee(uint24 fee) public pure {
-        assertEq((fee >> 23 == 1), LPFeeLibrary.isDynamicFee(fee));
+        assertEq(fee == LPFeeLibrary.DYNAMIC_FEE_FLAG, LPFeeLibrary.isDynamicFee(fee));
     }
 
     function test_validate_doesNotRevertWithNoFee() public pure {
@@ -64,12 +65,18 @@ contract LPFeeLibraryTest is Test {
     }
 
     function test_getInitialLPFee_forDynamicFeeIsZero() public pure {
-        uint24 dynamicFee = 0x800BB8;
+        uint24 dynamicFee = 0x800000;
         assertEq(LPFeeLibrary.getInitialLPFee(dynamicFee), 0);
     }
 
+    function test_getInitialLpFee_revertsWithNonExactDynamicFee() public {
+        uint24 dynamicFee = 0x800001;
+        vm.expectRevert(LPFeeLibrary.FeeTooLarge.selector);
+        LPFeeLibrary.getInitialLPFee(dynamicFee);
+    }
+
     function test_fuzz_getInitialLPFee(uint24 fee) public {
-        if (fee >> 23 == 1) {
+        if (fee == LPFeeLibrary.DYNAMIC_FEE_FLAG) {
             assertEq(LPFeeLibrary.getInitialLPFee(fee), 0);
         } else if (fee > 1000000) {
             vm.expectRevert(LPFeeLibrary.FeeTooLarge.selector);