From 397723e1a3b2ee8f03534a136d37bd4602f928a6 Mon Sep 17 00:00:00 2001 From: ChefMist <133624774+ChefMist@users.noreply.github.com> Date: Fri, 14 Feb 2025 12:04:11 +0800 Subject: [PATCH] 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 --- .github/workflows/coverage.yml | 67 ------------------------- foundry.toml | 4 +- snapshots/BinPoolManagerTest.json | 4 +- snapshots/CLPoolManagerTest.json | 4 +- src/ProtocolFeeController.sol | 13 +++-- src/ProtocolFees.sol | 4 +- src/pool-cl/libraries/CLPool.sol | 2 +- test/ProtocolFeeController.t.sol | 83 +++++++++++++++++++++++++------ 8 files changed, 84 insertions(+), 97 deletions(-) delete mode 100644 .github/workflows/coverage.yml diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml deleted file mode 100644 index b1049428..00000000 --- a/.github/workflows/coverage.yml +++ /dev/null @@ -1,67 +0,0 @@ -name: coverage - -on: - pull_request: - -jobs: - forge-coverage-comment: - name: Forge coverage and comment - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - with: - submodules: recursive - - - name: Install Foundry - uses: foundry-rs/foundry-toolchain@v1 - with: - version: stable - - # install dependency - - uses: actions/setup-node@v3 - with: - node-version: 16 - cache: 'yarn' - - - run: yarn - - # ignore test, script and total line - - name: Run forge coverage - id: coverage - run: | - { - echo 'COVERAGE<> "$GITHUB_OUTPUT" - - # 41898282 is github-actions bot id: https://github.com/marketplace/actions/bot-details - - name: Comment forge coverage report on PR - id: comment - uses: actions/github-script@v5 - with: - script: | - const {data: comments} = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - }) - - const botComment = comments.find(comment => comment.user.id === 41898282) - const coverageReport = `${{ steps.coverage.outputs.COVERAGE }}`; - - if (botComment) { - github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: botComment.id, - body: coverageReport - }) - } else { - github.rest.issues.createComment({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - body: coverageReport - }); - } diff --git a/foundry.toml b/foundry.toml index 9863e109..bede8749 100644 --- a/foundry.toml +++ b/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 diff --git a/snapshots/BinPoolManagerTest.json b/snapshots/BinPoolManagerTest.json index 50f114f2..7b2afc09 100644 --- a/snapshots/BinPoolManagerTest.json +++ b/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", diff --git a/snapshots/CLPoolManagerTest.json b/snapshots/CLPoolManagerTest.json index 4cf22b8d..982f042c 100644 --- a/snapshots/CLPoolManagerTest.json +++ b/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", diff --git a/src/ProtocolFeeController.sol b/src/ProtocolFeeController.sol index ad6e2ad2..e9cfcfff 100644 --- a/src/ProtocolFeeController.sol +++ b/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); } } diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol index c6f0f216..dbc728d9 100644 --- a/src/ProtocolFees.sol +++ b/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) diff --git a/src/pool-cl/libraries/CLPool.sol b/src/pool-cl/libraries/CLPool.sol index 73a4bd89..74272a18 100644 --- a/src/pool-cl/libraries/CLPool.sol +++ b/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 diff --git a/test/ProtocolFeeController.t.sol b/test/ProtocolFeeController.t.sol index 665219b8..53a75163 100644 --- a/test/ProtocolFeeController.t.sol +++ b/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) ); } }