change protocol fee push to be via protocol fee controller (Uniswap#577)

* change protocol fee push to be via protocol fee controller * change comment * out of bounds fee controller change
Aperture-Finance · Apr 11, 2024 · 43fa6c3 · 43fa6c3
1 parent 6b69637
commit 43fa6c3
Show file tree

Hide file tree

Showing 27 changed files with 59 additions and 63 deletions.
diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap
@@ -1 +1 @@
-264408
+264452
diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap
@@ -1 +1 @@
-139249
+139293
diff --git a/.forge-snapshots/addLiquidity.snap b/.forge-snapshots/addLiquidity.snap
@@ -1 +1 @@
-144585
+144629
diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap
@@ -1 +1 @@
-101156
+101244
diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap
@@ -1 +1 @@
-132165
+132253
diff --git a/.forge-snapshots/erc20 collect protocol fees.snap b/.forge-snapshots/erc20 collect protocol fees.snap
@@ -1 +1 @@
-24961
+24938
diff --git a/.forge-snapshots/native collect protocol fees.snap b/.forge-snapshots/native collect protocol fees.snap
@@ -1 +1 @@
-36634
+36611
diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap
@@ -1 +1 @@
-22453
+22467
diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap
@@ -1 +1 @@
-54753
+54797
diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap
@@ -1 +1 @@
-146936
+146980
diff --git a/.forge-snapshots/removeLiquidity.snap b/.forge-snapshots/removeLiquidity.snap
@@ -1 +1 @@
-148403
+148447
diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap
@@ -1 +1 @@
-132600
+132688
diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap
@@ -1 +1 @@
-146495
+146583
diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap
@@ -1 +1 @@
-72132
+72220
diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap
@@ -1 +1 @@
-60154
+60242
diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap
@@ -1 +1 @@
-80311
+80399
diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap
@@ -1 +1 @@
-76162
+76250
diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap
@@ -1 +1 @@
-138678
+138766
diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap
@@ -1 +1 @@
-155310
+155398
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 @@
-155767
+155899
diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap
@@ -1 +1 @@
-89385
+89473
diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap
@@ -1 +1 @@
-60132
+60220
diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap
@@ -1 +1 @@
-140011
+140121
diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol
@@ -33,9 +33,9 @@ abstract contract ProtocolFees is IProtocolFees, Owned {
     }
 
     /// @inheritdoc IProtocolFees
-    function setProtocolFee(PoolKey memory key) external {
-        (bool success, uint24 newProtocolFee) = _fetchProtocolFee(key);
-        if (!success) revert ProtocolFeeControllerCallFailedOrInvalidResult();
+    function setProtocolFee(PoolKey memory key, uint24 newProtocolFee) external {
+        if (msg.sender != address(protocolFeeController)) revert InvalidCaller();
+        if (!newProtocolFee.validate()) revert InvalidProtocolFee();
         PoolId id = key.toId();
         _getPool(id).setProtocolFee(newProtocolFee);
         emit ProtocolFeeUpdated(id, newProtocolFee);

diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol
@@ -9,10 +9,10 @@ import {PoolKey} from "../types/PoolKey.sol";
 interface IProtocolFees {
     /// @notice Thrown when not enough gas is provided to look up the protocol fee
     error ProtocolFeeCannotBeFetched();
-    /// @notice Thrown when the call to fetch the protocol fee reverts or returns invalid data.
-    error ProtocolFeeControllerCallFailedOrInvalidResult();
+    /// @notice Thrown when protocol fee is set too high
+    error InvalidProtocolFee();
 
-    /// @notice Thrown when collectProtocolFees is not called by the controller.
+    /// @notice Thrown when collectProtocolFees or setProtocolFee is not called by the controller.
     error InvalidCaller();
 
     event ProtocolFeeControllerUpdated(address protocolFeeController);
@@ -22,9 +22,8 @@ interface IProtocolFees {
     /// @notice Given a currency address, returns the protocol fees accrued in that currency
     function protocolFeesAccrued(Currency) external view returns (uint256);
 
-    /// @notice Sets the protocol's swap fee for the given pool
-    /// Protocol fees are always a portion of the LP swap fee that is owed. If that fee is 0, no protocol fees will accrue even if it is set to > 0.
-    function setProtocolFee(PoolKey memory key) external;
+    /// @notice Sets the protocol fee for the given pool
+    function setProtocolFee(PoolKey memory key, uint24) external;
 
     /// @notice Sets the protocol fee controller
     function setProtocolFeeController(IProtocolFeeController) external;

diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol
@@ -762,8 +762,8 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
 
         uint24 protocolFee = (uint24(protocolFee1) << 12) | uint24(protocolFee0);
 
-        feeController.setProtocolFeeForPool(key.toId(), protocolFee);
-        manager.setProtocolFee(key);
+        vm.prank(address(feeController));
+        manager.setProtocolFee(key, protocolFee);
 
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId());
         assertEq(slot0ProtocolFee, protocolFee);
@@ -954,42 +954,38 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     function test_setProtocolFee_updatesProtocolFeeForInitializedPool(uint24 protocolFee) public {
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId());
         assertEq(slot0ProtocolFee, 0);
-        feeController.setProtocolFeeForPool(key.toId(), protocolFee);
 
         uint16 fee0 = protocolFee.getZeroForOneFee();
         uint16 fee1 = protocolFee.getOneForZeroFee();
+        vm.prank(address(feeController));
         if ((fee0 > 2500) || (fee1 > 2500)) {
-            vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector);
-            manager.setProtocolFee(key);
+            vm.expectRevert(IProtocolFees.InvalidProtocolFee.selector);
+            manager.setProtocolFee(key, protocolFee);
         } else {
             vm.expectEmit(false, false, false, true);
             emit IProtocolFees.ProtocolFeeUpdated(key.toId(), protocolFee);
-            manager.setProtocolFee(key);
+            manager.setProtocolFee(key, protocolFee);
 
             (,, slot0ProtocolFee,) = manager.getSlot0(key.toId());
             assertEq(slot0ProtocolFee, protocolFee);
         }
     }
 
-    function test_setProtocolFee_failsWithInvalidProtocolFeeControllers() public {
+    function test_setProtocolFee_failsWithInvalidFee() public {
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId());
         assertEq(slot0ProtocolFee, 0);
 
-        manager.setProtocolFeeController(revertingFeeController);
-        vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector);
-        manager.setProtocolFee(key);
-
-        manager.setProtocolFeeController(outOfBoundsFeeController);
-        vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector);
-        manager.setProtocolFee(key);
+        vm.prank(address(feeController));
+        vm.expectRevert(IProtocolFees.InvalidProtocolFee.selector);
+        manager.setProtocolFee(key, MAX_FEE_BOTH_TOKENS + 1);
+    }
 
-        manager.setProtocolFeeController(overflowFeeController);
-        vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector);
-        manager.setProtocolFee(key);
+    function test_setProtocolFee_failsWithInvalidCaller() public {
+        (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId());
+        assertEq(slot0ProtocolFee, 0);
 
-        manager.setProtocolFeeController(invalidReturnSizeFeeController);
-        vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector);
-        manager.setProtocolFee(key);
+        vm.expectRevert(IProtocolFees.InvalidCaller.selector);
+        manager.setProtocolFee(key, MAX_FEE_BOTH_TOKENS);
     }
 
     function test_collectProtocolFees_initializesWithProtocolFeeIfCalled() public {
@@ -1008,8 +1004,9 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     function test_collectProtocolFees_ERC20_accumulateFees_gas() public {
         uint256 expectedFees = 7;
 
-        feeController.setProtocolFeeForPool(key.toId(), MAX_FEE_BOTH_TOKENS);
-        manager.setProtocolFee(key);
+        uint24 protocolFee = MAX_FEE_BOTH_TOKENS;
+        vm.prank(address(feeController));
+        manager.setProtocolFee(key, protocolFee);
 
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId());
         assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS);
@@ -1035,8 +1032,9 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
     function test_collectProtocolFees_ERC20_returnsAllFeesIf0IsProvidedAsParameter() public {
         uint256 expectedFees = 7;
 
-        feeController.setProtocolFeeForPool(key.toId(), MAX_FEE_BOTH_TOKENS);
-        manager.setProtocolFee(key);
+        uint24 protocolFee = MAX_FEE_BOTH_TOKENS;
+        vm.prank(address(feeController));
+        manager.setProtocolFee(key, protocolFee);
 
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(key.toId());
         assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS);
@@ -1062,8 +1060,9 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
         Currency nativeCurrency = CurrencyLibrary.NATIVE;
 
         // set protocol fee before initializing the pool as it is fetched on initialization
-        feeController.setProtocolFeeForPool(nativeKey.toId(), MAX_FEE_BOTH_TOKENS);
-        manager.setProtocolFee(nativeKey);
+        uint24 protocolFee = MAX_FEE_BOTH_TOKENS;
+        vm.prank(address(feeController));
+        manager.setProtocolFee(nativeKey, protocolFee);
 
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId());
         assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS);
@@ -1090,8 +1089,9 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
         uint256 expectedFees = 7;
         Currency nativeCurrency = CurrencyLibrary.NATIVE;
 
-        feeController.setProtocolFeeForPool(nativeKey.toId(), MAX_FEE_BOTH_TOKENS);
-        manager.setProtocolFee(nativeKey);
+        uint24 protocolFee = MAX_FEE_BOTH_TOKENS;
+        vm.prank(address(feeController));
+        manager.setProtocolFee(nativeKey, protocolFee);
 
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(nativeKey.toId());
         assertEq(slot0ProtocolFee, MAX_FEE_BOTH_TOKENS);

diff --git a/test/PoolManagerInitialize.t.sol b/test/PoolManagerInitialize.t.sol
@@ -321,9 +321,6 @@ contract PoolManagerInitializeTest is Test, Deployers, GasSnapshot {
         // protocol fees should default to 0
         (,, uint24 slot0ProtocolFee,) = manager.getSlot0(uninitializedKey.toId());
         assertEq(slot0ProtocolFee, 0);
-        // call to setProtocolFee should also revert
-        vm.expectRevert(IProtocolFees.ProtocolFeeControllerCallFailedOrInvalidResult.selector);
-        manager.setProtocolFee(uninitializedKey);
     }
 
     function test_initialize_succeedsWithRevertingFeeController(uint160 sqrtPriceX96) public {