feat: hexens audit feedback (#233)

* [fh-c3] call to protocol fee controller can be static call

* [fh-c1] typographical errors

* [fh-c2] set constant variable private

* [fh-c4] protocolfeeController: accurate protocolFeeCollected event

* [Part 2] of feat: hexens audit feedback] (#234)

* feat: remove coverage from ci as breaking build

* feat: follow periphery and set 5 runs for local

foundry.toml

@@ -17,9 +17,7 @@ gas_limit = "300000000"
 bytecode_hash = "none"
 
 [fuzz]
-# used for both fuzz and invariant tests for local development
-# not sure why but [profile.default.fuzz] just doesn't work for invariant tests
-runs = 1000
+runs = 5 # change this for higher number of fuzz runs locally
 
 [profile.ci.fuzz]
 runs = 10000

snapshots/BinPoolManagerTest.json

@@ -1,7 +1,7 @@
 {
-  "BinPoolManagerBytecodeSize": "23822",
+  "BinPoolManagerBytecodeSize": "23821",
   "BinPoolManagerTest": "292661",
-  "binPoolManager initcode hash (without constructor params, as uint256)": "73425840329377707764016122574450195738786283045901954729586336840188059167782",
+  "binPoolManager initcode hash (without constructor params, as uint256)": "86819153192491064271307073571725897487337363688428763335923728549485318549268",
   "testBurnNativeCurrency": "146154",
   "testExtLoadPoolActiveId": "272",
   "testGasBurnHalfBin": "141152",

snapshots/CLPoolManagerTest.json

@@ -1,9 +1,9 @@
 {
-  "CLPoolManagerBytecodeSize": "20886",
+  "CLPoolManagerBytecodeSize": "20885",
   "addLiquidity_fromEmpty": "343791",
   "addLiquidity_fromNonEmpty": "161744",
   "addLiquidity_nativeToken": "225761",
-  "clPoolManager initcode hash (without constructor params, as uint256)": "43498189724800683414039944409888665118981458752163755806370462274516389565331",
+  "clPoolManager initcode hash (without constructor params, as uint256)": "69937990386781985587033718064809012542744488478546742857206997773238707862066",
   "donateBothTokens": "162089",
   "gasDonateOneToken": "107144",
   "initializeWithoutHooks": "145737",

src/ProtocolFeeController.sol

@@ -19,10 +19,10 @@ contract ProtocolFeeController is IProtocolFeeController, Ownable2Step {
     error InvalidPoolManager();
 
     /// @notice throw when the protocol fee split ratio is invalid i.e. greater than 100%
-    error InvliadProtocolFeeSplitRatio();
+    error InvalidProtocolFeeSplitRatio();
 
     /// @notice 100% in hundredths of a bip
-    uint256 public constant ONE_HUNDRED_PERCENT_RATIO = 1e6;
+    uint256 private constant ONE_HUNDRED_PERCENT_RATIO = 1e6;
 
     /// @notice The ratio of the protocol fee in the total fee, expressed in hundredths of a bip i.e. 1e4 is 1%
     /// @dev The default value is 33% i.e. protocol fee should be 33% of the total fee
@@ -42,7 +42,7 @@ contract ProtocolFeeController is IProtocolFeeController, Ownable2Step {
     /// @notice Set the ratio of the protocol fee in the total fee
     /// @param newProtocolFeeSplitRatio 30e4 would mean 30% of the total fee goes to protocol
     function setProtocolFeeSplitRatio(uint256 newProtocolFeeSplitRatio) external onlyOwner {
-        if (newProtocolFeeSplitRatio > ONE_HUNDRED_PERCENT_RATIO) revert InvliadProtocolFeeSplitRatio();
+        if (newProtocolFeeSplitRatio > ONE_HUNDRED_PERCENT_RATIO) revert InvalidProtocolFeeSplitRatio();
 
         uint256 oldProtocolFeeSplitRatio = protocolFeeSplitRatio;
         protocolFeeSplitRatio = newProtocolFeeSplitRatio;
@@ -109,7 +109,7 @@ contract ProtocolFeeController is IProtocolFeeController, Ownable2Step {
         return fee + (fee << 12);
     }
 
-    /// @notice Override the default protcool fee for the pool
+    /// @notice Override the default protocol fee for the pool
     /// @dev this could be used for marketing campaign where PCS takes 0 protocol fee for a pool for a period
     /// @param newProtocolFee 1000 = 0.1%, and max at 4000 = 0.4%. If set at 0.1%, this means 0.1% of amountIn for each swap will go to protocol
     function setProtocolFee(PoolKey memory key, uint24 newProtocolFee) external onlyOwner {
@@ -124,8 +124,11 @@ contract ProtocolFeeController is IProtocolFeeController, Ownable2Step {
     /// @param currency The currency of the protocol fee
     /// @param amount The amount of the protocol fee to collect, 0 means collect all
     function collectProtocolFee(address recipient, Currency currency, uint256 amount) external onlyOwner {
+        // balance check to handle fee-on-transfer tokens
+        uint256 balanceBefore = currency.balanceOf(recipient);
         IProtocolFees(poolManager).collectProtocolFees(recipient, currency, amount);
+        uint256 balanceAfter = currency.balanceOf(recipient);
 
-        emit ProtocolFeeCollected(currency, amount);
+        emit ProtocolFeeCollected(currency, balanceAfter - balanceBefore);
     }
 }

src/ProtocolFees.sol

@@ -43,7 +43,7 @@ abstract contract ProtocolFees is IProtocolFees, Owner {
     /// @dev Revert if call to protocolFeeController fails or if return value is not 32 bytes
     /// However if the call to protocolFeeController succeed, it can still revert if the return value is too large
     /// @return protocolFee The protocol fee for the pool
-    function _fetchProtocolFee(PoolKey memory key) internal returns (uint24 protocolFee) {
+    function _fetchProtocolFee(PoolKey memory key) internal view returns (uint24 protocolFee) {
         if (address(protocolFeeController) != address(0)) {
             address targetProtocolFeeController = address(protocolFeeController);
             bytes memory data = abi.encodeCall(IProtocolFeeController.protocolFeeForPool, (key));
@@ -52,7 +52,7 @@ abstract contract ProtocolFees is IProtocolFees, Owner {
             uint256 returnData;
             assembly ("memory-safe") {
                 // only load the first 32 bytes of the return data to prevent gas griefing
-                success := call(gas(), targetProtocolFeeController, 0, add(data, 0x20), mload(data), 0, 32)
+                success := staticcall(gas(), targetProtocolFeeController, add(data, 0x20), mload(data), 0, 32)
 
                 // load the return data
                 returnData := mload(0)

src/pool-cl/libraries/CLPool.sol

@@ -31,7 +31,7 @@ library CLPool {
     using ProtocolFeeLibrary for uint16;
     using LPFeeLibrary for uint24;
 
-    /// @notice Thrown when trying to initalize an already initialized pool
+    /// @notice Thrown when trying to initialize an already initialized pool
     error PoolAlreadyInitialized();
 
     /// @notice Thrown when trying to interact with a non-initialized pool

test/ProtocolFeeController.t.sol

@@ -33,6 +33,9 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
     using BinPoolParametersHelper for bytes32;
     using ProtocolFeeLibrary for *;
 
+    /// @notice 100% in hundredths of a bip
+    uint256 private constant ONE_HUNDRED_PERCENT_RATIO = 1e6;
+
     Vault vault;
     CLPoolManager clPoolManager;
     BinPoolManager binPoolManager;
@@ -97,8 +100,8 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
             controller.setProtocolFeeSplitRatio(newProtocolFeeSplitRatio);
         }
 
-        if (newProtocolFeeSplitRatio > controller.ONE_HUNDRED_PERCENT_RATIO()) {
-            vm.expectRevert(ProtocolFeeController.InvliadProtocolFeeSplitRatio.selector);
+        if (newProtocolFeeSplitRatio > ONE_HUNDRED_PERCENT_RATIO) {
+            vm.expectRevert(ProtocolFeeController.InvalidProtocolFeeSplitRatio.selector);
             controller.setProtocolFeeSplitRatio(newProtocolFeeSplitRatio);
         } else {
             vm.expectEmit(true, true, true, true);
@@ -163,7 +166,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
         ProtocolFeeController controller = new ProtocolFeeController(address(clPoolManager));
 
         // ignore extreme case where splitRatio is over 90% to avoid precision loss
-        splitRatio = uint24(bound(splitRatio, 0, controller.ONE_HUNDRED_PERCENT_RATIO() * 9 / 10));
+        splitRatio = uint24(bound(splitRatio, 0, ONE_HUNDRED_PERCENT_RATIO * 9 / 10));
         controller.setProtocolFeeSplitRatio(splitRatio);
 
         // try to simulate the calculation the process of FE initialization pool
@@ -199,7 +202,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
     function testProtocolFeeForPool(uint24 lpFee, uint256 protocolFeeRatio) public {
         lpFee = uint24(bound(lpFee, 0, LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE));
         ProtocolFeeController controller = new ProtocolFeeController(address(clPoolManager));
-        protocolFeeRatio = bound(protocolFeeRatio, 0, controller.ONE_HUNDRED_PERCENT_RATIO());
+        protocolFeeRatio = bound(protocolFeeRatio, 0, ONE_HUNDRED_PERCENT_RATIO);
         controller.setProtocolFeeSplitRatio(protocolFeeRatio);
 
         PoolKey memory key = PoolKey({
@@ -227,7 +230,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
             // protocol fee should be protocolFeeRatio of the total fee
             uint24 totalFee = protocolFeeZeroForOne.calculateSwapFee(lpFee);
             assertApproxEqAbs(
-                totalFee * controller.protocolFeeSplitRatio() / controller.ONE_HUNDRED_PERCENT_RATIO(),
+                totalFee * controller.protocolFeeSplitRatio() / ONE_HUNDRED_PERCENT_RATIO,
                 protocolFeeZeroForOne,
                 // keeping the error within 0.01% (can't avoid due to precision loss)
                 100
@@ -273,7 +276,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
     function testCLPoolInitWithProtolFeeControllerFuzz(uint24 lpFee, uint256 newProtocolFeeRatio) public {
         lpFee = uint24(bound(lpFee, 0, LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE));
         ProtocolFeeController controller = new ProtocolFeeController(address(clPoolManager));
-        newProtocolFeeRatio = bound(newProtocolFeeRatio, 0, controller.ONE_HUNDRED_PERCENT_RATIO());
+        newProtocolFeeRatio = bound(newProtocolFeeRatio, 0, ONE_HUNDRED_PERCENT_RATIO);
 
         clPoolManager.setProtocolFeeController(controller);
         controller.setProtocolFeeSplitRatio(newProtocolFeeRatio);
@@ -307,7 +310,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
             // protocol fee should be the given ratio of the total fee
             uint24 totalFee = protocolFeeZeroForOne.calculateSwapFee(actualLpFee);
             assertApproxEqAbs(
-                totalFee * controller.protocolFeeSplitRatio() / controller.ONE_HUNDRED_PERCENT_RATIO(),
+                totalFee * controller.protocolFeeSplitRatio() / ONE_HUNDRED_PERCENT_RATIO,
                 protocolFeeZeroForOne,
                 // keeping the error within 0.05% (can't avoid due to precision loss)
                 500
@@ -318,7 +321,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
     function testBinPoolInitWithProtolFeeControllerFuzz(uint24 lpFee, uint256 newProtocolFeeRatio) public {
         lpFee = uint24(bound(lpFee, 0, LPFeeLibrary.TEN_PERCENT_FEE));
         ProtocolFeeController controller = new ProtocolFeeController(address(binPoolManager));
-        newProtocolFeeRatio = bound(newProtocolFeeRatio, 0, controller.ONE_HUNDRED_PERCENT_RATIO());
+        newProtocolFeeRatio = bound(newProtocolFeeRatio, 0, ONE_HUNDRED_PERCENT_RATIO);
 
         binPoolManager.setProtocolFeeController(controller);
         controller.setProtocolFeeSplitRatio(newProtocolFeeRatio);
@@ -352,7 +355,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
             // protocol fee should be the given ratio of the total fee
             uint24 totalFee = protocolFeeZeroForOne.calculateSwapFee(actualLpFee);
             assertApproxEqAbs(
-                totalFee * controller.protocolFeeSplitRatio() / controller.ONE_HUNDRED_PERCENT_RATIO(),
+                totalFee * controller.protocolFeeSplitRatio() / ONE_HUNDRED_PERCENT_RATIO,
                 protocolFeeZeroForOne,
                 // keeping the error within 0.01% (can't avoid due to precision loss)
                 100
@@ -400,6 +403,56 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
         }
     }
 
+    /// @dev when collectProtocolFee with amt=0, event should emit amount collected
+    function testCollectProtocolFee_CollectAllFee() public {
+        // init protocol fee controller and bind it to clPoolManager
+        ProtocolFeeController controller = new ProtocolFeeController(address(clPoolManager));
+        clPoolManager.setProtocolFeeController(controller);
+
+        // init pool with protocol fee controller
+        PoolKey memory key = PoolKey({
+            currency0: currency0,
+            currency1: currency1,
+            hooks: IHooks(address(0)),
+            poolManager: clPoolManager,
+            fee: 2000,
+            parameters: bytes32(0).setTickSpacing(10)
+        });
+        clPoolManager.initialize(key, Constants.SQRT_RATIO_1_1);
+
+        // add some liquidity
+        CLPoolManagerRouter router = new CLPoolManagerRouter(vault, clPoolManager);
+        IERC20(Currency.unwrap(currency0)).approve(address(router), 10000 ether);
+        IERC20(Currency.unwrap(currency1)).approve(address(router), 10000 ether);
+        router.modifyPosition(
+            key,
+            ICLPoolManager.ModifyLiquidityParams({tickLower: -10, tickUpper: 10, liquidityDelta: 1000000 ether, salt: 0}),
+            ""
+        );
+
+        // swap to generate protocol fee
+        // by default splitRatio=33.33% if lpFee is 0.2% then protocol fee should be roughly 0.1%
+        router.swap(
+            key,
+            ICLPoolManager.SwapParams({
+                zeroForOne: true,
+                amountSpecified: -100 ether,
+                sqrtPriceLimitX96: TickMath.MIN_SQRT_RATIO + 1
+            }),
+            CLPoolManagerRouter.SwapTestSettings({withdrawTokens: true, settleUsingTransfer: true}),
+            ""
+        );
+
+        // verify protocol fee accrued > 0
+        uint256 protocolFeesAccrued = clPoolManager.protocolFeesAccrued(currency0);
+        assertGt(protocolFeesAccrued, 0);
+
+        // collect all fee
+        vm.expectEmit();
+        emit ProtocolFeeController.ProtocolFeeCollected(currency0, protocolFeesAccrued);
+        controller.collectProtocolFee(makeAddr("recipient"), currency0, 0);
+    }
+
     function testCollectProtocolFeeForCLPool() public {
         // init protocol fee controller and bind it to clPoolManager
         ProtocolFeeController controller = new ProtocolFeeController(address(clPoolManager));
@@ -443,7 +496,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
 
         assertEq(
             clPoolManager.protocolFeesAccrued(currency0),
-            100 ether * uint256(actualProtocolFee >> 12) / controller.ONE_HUNDRED_PERCENT_RATIO()
+            100 ether * uint256(actualProtocolFee >> 12) / ONE_HUNDRED_PERCENT_RATIO
         );
 
         // lp fee should be roughly 0.1 ether, allow 2% error
@@ -477,6 +530,8 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
 
         // collect half
         uint256 protocolFeeAmount = clPoolManager.protocolFeesAccrued(currency0);
+        vm.expectEmit();
+        emit ProtocolFeeController.ProtocolFeeCollected(currency0, protocolFeeAmount / 2);
         controller.collectProtocolFee(makeAddr("recipient"), currency0, protocolFeeAmount / 2);
 
         assertEq(clPoolManager.protocolFeesAccrued(currency0), protocolFeeAmount / 2);
@@ -518,7 +573,7 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
 
         assertEq(
             binPoolManager.protocolFeesAccrued(currency0),
-            100 ether * uint256(actualProtocolFee >> 12) / controller.ONE_HUNDRED_PERCENT_RATIO()
+            100 ether * uint256(actualProtocolFee >> 12) / ONE_HUNDRED_PERCENT_RATIO
         );
 
         // lp fee should be roughly 0.2 ether
@@ -560,10 +615,8 @@ contract ProtocolFeeControllerTest is Test, BinTestHelper, TokenFixture {
 
     function _calculateLPFeeThreshold(ProtocolFeeController controller) internal view returns (uint24) {
         return uint24(
-            (
-                (controller.ONE_HUNDRED_PERCENT_RATIO() / controller.protocolFeeSplitRatio() - 1)
-                    * ProtocolFeeLibrary.MAX_PROTOCOL_FEE
-            ) / (controller.ONE_HUNDRED_PERCENT_RATIO() - ProtocolFeeLibrary.MAX_PROTOCOL_FEE)
+            ((ONE_HUNDRED_PERCENT_RATIO / controller.protocolFeeSplitRatio() - 1) * ProtocolFeeLibrary.MAX_PROTOCOL_FEE)
+                / (ONE_HUNDRED_PERCENT_RATIO - ProtocolFeeLibrary.MAX_PROTOCOL_FEE)
         );
     }
 }