Audit/hexens 2nd round part1 (#206)

* fix: [hexen-s4] binPool donate won't revert when bin share is exactly the threshold * fix: [hexen-s5] rename MAX_BIN_STEP, MIN_BIN_SHARE_FOR_DONATE to camelCase since they are both storage variables instead of constant * docs: [hexen-s6] update code comments for clPool swapFee=100% case * fix: [hexen-s7] remove unused import * docs: [hexen-s9] added spec for Hooks library
pancakeswap · Nov 7, 2024 · 11cb533 · 11cb533
1 parent 09486ed
commit 11cb533
Show file tree

Hide file tree

Showing 24 changed files with 42 additions and 45 deletions.
diff --git a/.forge-snapshots/BinCustomCurveHookTest#test_Swap_CustomCurve.snap b/.forge-snapshots/BinCustomCurveHookTest#test_Swap_CustomCurve.snap
@@ -1 +1 @@
-142390
+142368
diff --git a/.forge-snapshots/BinHookTest#testDonateSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testDonateSucceedsWithHook.snap
@@ -1 +1 @@
-188435
+188410
diff --git a/.forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap
@@ -1 +1 @@
-189473
+189451
diff --git a/.forge-snapshots/BinPoolManagerBytecodeSize.snap b/.forge-snapshots/BinPoolManagerBytecodeSize.snap
@@ -1 +1 @@
-23296
+23295
diff --git a/.forge-snapshots/BinPoolManagerTest#testExtLoadPoolActiveId.snap b/.forge-snapshots/BinPoolManagerTest#testExtLoadPoolActiveId.snap
@@ -1 +1 @@
-1877
+1855
diff --git a/.forge-snapshots/BinPoolManagerTest#testFuzzUpdateDynamicLPFee.snap b/.forge-snapshots/BinPoolManagerTest#testFuzzUpdateDynamicLPFee.snap
@@ -1 +1 @@
-32521
+32499
diff --git a/.forge-snapshots/BinPoolManagerTest#testFuzz_SetMaxBinStep.snap b/.forge-snapshots/BinPoolManagerTest#testFuzz_SetMaxBinStep.snap
@@ -1 +1 @@
-34966
+34922
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasDonate.snap b/.forge-snapshots/BinPoolManagerTest#testGasDonate.snap
@@ -1 +1 @@
-118628
+118603
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasGetBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasGetBin.snap
@@ -1 +1 @@
-4083
+4061
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap b/.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap
@@ -1 +1 @@
-173054
+173032
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap b/.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap
@@ -1 +1 @@
-179082
+179060
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap
@@ -1 +1 @@
-133085
+133063
diff --git a/.forge-snapshots/BinPoolManagerTest#testSetProtocolFee.snap b/.forge-snapshots/BinPoolManagerTest#testSetProtocolFee.snap
@@ -1 +1 @@
-34485
+34463
diff --git a/src/libraries/Hooks.sol b/src/libraries/Hooks.sol
@@ -10,6 +10,10 @@ import {ParametersHelper} from "./math/ParametersHelper.sol";
 import {ParseBytes} from "./ParseBytes.sol";
 import {CustomRevert} from "./CustomRevert.sol";
 
+/**
+ * @title Hooks library
+ * @notice It provides some general helper functions that are used by the poolManager when verifying hook permissions, interacting with hooks, etc.
+ */
 library Hooks {
     using Encoded for bytes32;
     using ParametersHelper for bytes32;

diff --git a/src/pool-bin/BinPoolManager.sol b/src/pool-bin/BinPoolManager.sol
@@ -21,7 +21,6 @@ import {Extsload} from "../Extsload.sol";
 import {BinHooks} from "./libraries/BinHooks.sol";
 import {PriceHelper} from "./libraries/PriceHelper.sol";
 import {BeforeSwapDelta} from "../types/BeforeSwapDelta.sol";
-import "./interfaces/IBinHooks.sol";
 import {BinSlot0} from "./types/BinSlot0.sol";
 import {VaultAppDeltaSettlement} from "../libraries/VaultAppDeltaSettlement.sol";
 
@@ -39,10 +38,10 @@ contract BinPoolManager is IBinPoolManager, ProtocolFees, Extsload {
     uint16 public constant override MIN_BIN_STEP = 1;
 
     /// @inheritdoc IBinPoolManager
-    uint16 public override MAX_BIN_STEP = 100;
+    uint16 public override maxBinStep = 100;
 
     /// @inheritdoc IBinPoolManager
-    uint256 public override MIN_BIN_SHARE_FOR_DONATE = 2 ** 128;
+    uint256 public override minBinShareForDonate = 2 ** 128;
 
     mapping(PoolId id => BinPool.State poolState) public pools;
 
@@ -106,7 +105,7 @@ contract BinPoolManager is IBinPoolManager, ProtocolFees, Extsload {
     {
         uint16 binStep = key.parameters.getBinStep();
         if (binStep < MIN_BIN_STEP) revert BinStepTooSmall(binStep);
-        if (binStep > MAX_BIN_STEP) revert BinStepTooLarge(binStep);
+        if (binStep > maxBinStep) revert BinStepTooLarge(binStep);
         if (key.currency0 >= key.currency1) {
             revert CurrenciesInitializedOutOfOrder(Currency.unwrap(key.currency0), Currency.unwrap(key.currency1));
         }
@@ -276,7 +275,7 @@ contract BinPoolManager is IBinPoolManager, ProtocolFees, Extsload {
 
         /// @dev Share is 1:1 liquidity when liquidity is first added to bin
         uint256 currentBinShare = pool.shareOfBin[pool.slot0.activeId()];
-        if (currentBinShare <= MIN_BIN_SHARE_FOR_DONATE) {
+        if (currentBinShare < minBinShareForDonate) {
             revert InsufficientBinShareForDonate(currentBinShare);
         }
 
@@ -291,16 +290,16 @@ contract BinPoolManager is IBinPoolManager, ProtocolFees, Extsload {
     }
 
     /// @inheritdoc IBinPoolManager
-    function setMaxBinStep(uint16 maxBinStep) external override onlyOwner {
-        if (maxBinStep <= MIN_BIN_STEP) revert MaxBinStepTooSmall(maxBinStep);
+    function setMaxBinStep(uint16 newMaxBinStep) external override onlyOwner {
+        if (newMaxBinStep <= MIN_BIN_STEP) revert MaxBinStepTooSmall(newMaxBinStep);
 
-        MAX_BIN_STEP = maxBinStep;
-        emit SetMaxBinStep(maxBinStep);
+        maxBinStep = newMaxBinStep;
+        emit SetMaxBinStep(newMaxBinStep);
     }
 
     /// @inheritdoc IBinPoolManager
     function setMinBinSharesForDonate(uint256 minBinShare) external override onlyOwner {
-        MIN_BIN_SHARE_FOR_DONATE = minBinShare;
+        minBinShareForDonate = minBinShare;
         emit SetMinBinSharesForDonate(minBinShare);
     }
 

diff --git a/src/pool-bin/BinPoolManagerOwner.sol b/src/pool-bin/BinPoolManagerOwner.sol
@@ -2,7 +2,6 @@
 // Copyright (C) 2024 PancakeSwap
 pragma solidity 0.8.26;
 
-import {Currency} from "../types/Currency.sol";
 import {IProtocolFeeController} from "../interfaces/IProtocolFeeController.sol";
 import {PausableRole} from "../base/PausableRole.sol";
 import {IBinPoolManager} from "./interfaces/IBinPoolManager.sol";

diff --git a/src/pool-bin/interfaces/IBinPoolManager.sol b/src/pool-bin/interfaces/IBinPoolManager.sol
@@ -18,7 +18,7 @@ interface IBinPoolManager is IProtocolFees, IPoolManager, IExtsload {
     /// @notice Pool binStep cannot be lesser than 1. Otherwise there will be no price jump between bin
     error BinStepTooSmall(uint16 binStep);
 
-    /// @notice Pool binstep cannot be greater than the limit set at MAX_BIN_STEP
+    /// @notice Pool binstep cannot be greater than the limit set at maxBinStep
     error BinStepTooLarge(uint16 binStep);
 
     /// @notice Error thrown when owner set max bin step too small
@@ -32,14 +32,14 @@ interface IBinPoolManager is IProtocolFees, IPoolManager, IExtsload {
 
     /// @notice Returns the constant representing the max bin step
     /// @return maxBinStep a value of 100 would represent a 1% price jump between bin (limit can be raised by owner)
-    function MAX_BIN_STEP() external view returns (uint16);
+    function maxBinStep() external view returns (uint16);
 
     /// @notice Returns the constant representing the min bin step
     /// @dev 1 would represent a 0.01% price jump between bin
     function MIN_BIN_STEP() external view returns (uint16);
 
     /// @notice min share in bin before donate is allowed in current bin
-    function MIN_BIN_SHARE_FOR_DONATE() external view returns (uint256);
+    function minBinShareForDonate() external view returns (uint256);
 
     /// @notice Emitted when a new pool is initialized
     /// @param id The abi encoded hash of the pool key struct for the new pool

diff --git a/src/pool-cl/CLPoolManager.sol b/src/pool-cl/CLPoolManager.sol
@@ -2,7 +2,6 @@
 // Copyright (C) 2024 PancakeSwap
 pragma solidity 0.8.26;
 
-import "./interfaces/ICLHooks.sol";
 import {ProtocolFees} from "../ProtocolFees.sol";
 import {ICLPoolManager} from "./interfaces/ICLPoolManager.sol";
 import {IVault} from "../interfaces/IVault.sol";

diff --git a/src/pool-cl/CLPoolManagerOwner.sol b/src/pool-cl/CLPoolManagerOwner.sol
@@ -2,7 +2,6 @@
 // Copyright (C) 2024 PancakeSwap
 pragma solidity 0.8.26;
 
-import {Currency} from "../types/Currency.sol";
 import {IProtocolFeeController} from "../interfaces/IProtocolFeeController.sol";
 import {PausableRole} from "../base/PausableRole.sol";
 import {ICLPoolManager} from "./interfaces/ICLPoolManager.sol";

diff --git a/src/pool-cl/libraries/CLPool.sol b/src/pool-cl/libraries/CLPool.sol
@@ -232,8 +232,7 @@ library CLPool {
             });
         }
 
-        /// @dev If amountSpecified is the output, also given amountSpecified cant be 0,
-        /// then the tx will always revert if the swap fee is 100%
+        /// @dev a swap fee totaling 100% makes exact output swaps impossible since the input is entirely consumed by the fee
         if (state.swapFee >= LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE) {
             if (!exactInput) {
                 revert InvalidFeeForExactOut();

diff --git a/test/pool-bin/BinPoolManager.t.sol b/test/pool-bin/BinPoolManager.t.sol
@@ -127,7 +127,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
     }
 
     function test_FuzzInitializePool(uint16 binStep) public {
-        binStep = uint16(bound(binStep, poolManager.MIN_BIN_STEP(), poolManager.MAX_BIN_STEP()));
+        binStep = uint16(bound(binStep, poolManager.MIN_BIN_STEP(), poolManager.maxBinStep()));
 
         uint16 bitMap = 0x0008; // after mint call
         MockBinHooks mockHooks = new MockBinHooks();
@@ -337,12 +337,10 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
             hooks: IHooks(address(0)),
             poolManager: IPoolManager(address(poolManager)),
             fee: uint24(3000), // 3000 = 0.3%
-            parameters: poolParam.setBinStep(poolManager.MAX_BIN_STEP() + 1) // binStep
+            parameters: poolParam.setBinStep(poolManager.maxBinStep() + 1) // binStep
         });
 
-        vm.expectRevert(
-            abi.encodeWithSelector(IBinPoolManager.BinStepTooLarge.selector, poolManager.MAX_BIN_STEP() + 1)
-        );
+        vm.expectRevert(abi.encodeWithSelector(IBinPoolManager.BinStepTooLarge.selector, poolManager.maxBinStep() + 1));
         poolManager.initialize(key, activeId);
     }
 
@@ -939,7 +937,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         emit SetMaxBinStep(binStep);
         poolManager.setMaxBinStep(binStep);
 
-        assertEq(poolManager.MAX_BIN_STEP(), binStep);
+        assertEq(poolManager.maxBinStep(), binStep);
     }
 
     function testGas_SetMaxBinStep() public {
@@ -951,7 +949,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         poolManager.setMaxBinStep(binStep);
         snapEnd();
 
-        assertEq(poolManager.MAX_BIN_STEP(), binStep);
+        assertEq(poolManager.maxBinStep(), binStep);
     }
 
     function testSetMaxBinStep() public {
@@ -971,7 +969,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         emit IBinPoolManager.SetMinBinSharesForDonate(minShare);
         poolManager.setMinBinSharesForDonate(minShare);
 
-        assertEq(poolManager.MIN_BIN_SHARE_FOR_DONATE(), minShare);
+        assertEq(poolManager.minBinShareForDonate(), minShare);
     }
 
     function testMinBinSharesForDonate_OnlyOwner() public {

diff --git a/test/pool-bin/BinPoolManagerBehaviorComparison.t.sol b/test/pool-bin/BinPoolManagerBehaviorComparison.t.sol
@@ -73,7 +73,7 @@ abstract contract LBFuzzer is LBHelper, BinTestHelper {
         public
         returns (ILBPair lbPair, PoolKey memory key_, uint16 boundBinStep, uint24 boundActiveId)
     {
-        boundBinStep = uint16(bound(binStep, manager.MIN_BIN_STEP(), manager.MAX_BIN_STEP()));
+        boundBinStep = uint16(bound(binStep, manager.MIN_BIN_STEP(), manager.maxBinStep()));
         boundActiveId = _getBoundId(boundBinStep, activeId);
 
         // lb init

diff --git a/test/pool-bin/BinPoolManagerOwner.t.sol b/test/pool-bin/BinPoolManagerOwner.t.sol
@@ -124,11 +124,11 @@ contract BinPoolManagerOwnerTest is Test {
 
     function test_SetMaxBinStep_OnlyOwner() public {
         // before
-        assertEq(poolManager.MAX_BIN_STEP(), 100);
+        assertEq(poolManager.maxBinStep(), 100);
 
         // after
         binPoolManagerOwner.setMaxBinStep(200);
-        assertEq(poolManager.MAX_BIN_STEP(), 200);
+        assertEq(poolManager.maxBinStep(), 200);
     }
 
     function test_SetMaxBinStep_NotOwner() public {
@@ -140,11 +140,11 @@ contract BinPoolManagerOwnerTest is Test {
 
     function test_SetMinBinSharesForDonate_OnlyOwner() public {
         // before
-        assertEq(poolManager.MIN_BIN_SHARE_FOR_DONATE(), 2 ** 128);
+        assertEq(poolManager.minBinShareForDonate(), 2 ** 128);
 
         // after
         binPoolManagerOwner.setMinBinSharesForDonate(1e18);
-        assertEq(poolManager.MIN_BIN_SHARE_FOR_DONATE(), 1e18);
+        assertEq(poolManager.minBinShareForDonate(), 1e18);
 
         // if set beow
         vm.expectRevert(abi.encodeWithSelector(BinPoolManagerOwner.MinShareTooSmall.selector, 1));

diff --git a/test/pool-bin/libraries/BinPoolDonate.t.sol b/test/pool-bin/libraries/BinPoolDonate.t.sol
@@ -62,7 +62,7 @@ contract BinPoolDonateTest is BinTestHelper {
         addLiquidityToBin(key, poolManager, alice, activeId, 1e18, 1e18, 1e18, 1e18, "");
 
         // Remove all share leaving less than MIN_LIQUIDITY_BEFORE_DONATE shares
-        remainingShare = bound(remainingShare, 1, poolManager.MIN_BIN_SHARE_FOR_DONATE() - 1);
+        remainingShare = bound(remainingShare, 1, poolManager.minBinShareForDonate() - 1);
         uint256 aliceShare = poolManager.getPosition(poolId, alice, activeId, 0).share;
         removeLiquidityFromBin(key, poolManager, alice, activeId, aliceShare - remainingShare, "");