diff --git a/snapshots/CustomAccountingTest.json b/snapshots/CustomAccountingTest.json index 429c419f9..3d75f4228 100644 --- a/snapshots/CustomAccountingTest.json +++ b/snapshots/CustomAccountingTest.json @@ -1,6 +1,6 @@ { - "addLiquidity CA fee": "170928", + "addLiquidity CA fee": "170690", "removeLiquidity CA fee": "141194", - "swap CA custom curve + swap noop": "124635", - "swap CA fee on unspecified": "154686" + "swap CA custom curve + swap noop": "124397", + "swap CA fee on unspecified": "154567" } \ No newline at end of file diff --git a/snapshots/ModifyLiquidityTest.json b/snapshots/ModifyLiquidityTest.json index 7aee2faed..7744b9abe 100644 --- a/snapshots/ModifyLiquidityTest.json +++ b/snapshots/ModifyLiquidityTest.json @@ -1,4 +1,4 @@ { - "add liquidity to already existing position with salt": "144639", - "create new liquidity to a position with salt": "292831" + "add liquidity to already existing position with salt": "144401", + "create new liquidity to a position with salt": "292593" } \ No newline at end of file diff --git a/snapshots/PoolManagerTest.json b/snapshots/PoolManagerTest.json index 375bbdce4..0db8f1537 100644 --- a/snapshots/PoolManagerTest.json +++ b/snapshots/PoolManagerTest.json @@ -1,24 +1,24 @@ { - "addLiquidity with empty hook": "274240", - "addLiquidity with native token": "135120", - "donate gas with 1 token": "106333", - "donate gas with 2 tokens": "145748", - "erc20 collect protocol fees": "57451", - "native collect protocol fees": "59723", - "poolManager bytecode size": "23694", + "addLiquidity with empty hook": "274002", + "addLiquidity with native token": "135001", + "donate gas with 1 token": "106214", + "donate gas with 2 tokens": "145510", + "erc20 collect protocol fees": "57500", + "native collect protocol fees": "59643", + "poolManager bytecode size": "23671", "removeLiquidity with empty hook": "130603", "removeLiquidity with native token": "112523", - "simple addLiquidity": "161395", - "simple addLiquidity second addition same range": "98850", + "simple addLiquidity": "161276", + "simple addLiquidity second addition same range": "98731", "simple removeLiquidity": "85099", "simple removeLiquidity some liquidity remains": "92986", - "simple swap": "123263", + "simple swap": "123144", "simple swap with native": "108434", - "swap against liquidity": "116646", + "swap against liquidity": "116527", "swap against liquidity with native token": "105569", "swap burn 6909 for input": "129285", "swap burn native 6909 for input": "118672", - "swap mint native output as 6909": "139739", - "swap mint output as 6909": "155104", - "swap with hooks": "132274" + "swap mint native output as 6909": "139620", + "swap mint output as 6909": "154985", + "swap with hooks": "132155" } \ No newline at end of file diff --git a/snapshots/SkipCallsTest.json b/snapshots/SkipCallsTest.json index 1f15eb95f..e5a1427e1 100644 --- a/snapshots/SkipCallsTest.json +++ b/snapshots/SkipCallsTest.json @@ -1,3 +1,3 @@ { - "swap skips hook call if hook is caller": "206263" + "swap skips hook call if hook is caller": "206025" } \ No newline at end of file diff --git a/snapshots/SyncTest.json b/snapshots/SyncTest.json index f4074cf4b..80aa773b0 100644 --- a/snapshots/SyncTest.json +++ b/snapshots/SyncTest.json @@ -1,3 +1,3 @@ { - "getReserves": "3931" + "getReserves": "3973" } \ No newline at end of file diff --git a/snapshots/TestDynamicFees.json b/snapshots/TestDynamicFees.json index e16227c2e..8f16d78f2 100644 --- a/snapshots/TestDynamicFees.json +++ b/snapshots/TestDynamicFees.json @@ -1,4 +1,4 @@ { - "swap with dynamic fee": "139272", - "update dynamic fee in before swap": "147857" + "swap with dynamic fee": "139153", + "update dynamic fee in before swap": "147738" } \ No newline at end of file diff --git a/snapshots/TestDynamicReturnFees.json b/snapshots/TestDynamicReturnFees.json index 9b9325633..435a2a631 100644 --- a/snapshots/TestDynamicReturnFees.json +++ b/snapshots/TestDynamicReturnFees.json @@ -1,3 +1,3 @@ { - "swap with return dynamic fee": "145589" + "swap with return dynamic fee": "145470" } \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index 8733bc276..d2587c304 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -273,7 +273,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } /// @inheritdoc IPoolManager - function sync(Currency currency) external onlyWhenUnlocked { + function sync(Currency currency) external { // address(0) is used for the native currency if (currency.isAddressZero()) { // The reserves balance is not used for native settling, so we only need to reset the currency. diff --git a/src/ProtocolFees.sol b/src/ProtocolFees.sol index 5eab2e71b..cfae6bceb 100644 --- a/src/ProtocolFees.sol +++ b/src/ProtocolFees.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.0; import {Currency} from "./types/Currency.sol"; +import {CurrencyReserves} from "./libraries/CurrencyReserves.sol"; import {IProtocolFees} from "./interfaces/IProtocolFees.sol"; import {PoolKey} from "./types/PoolKey.sol"; import {ProtocolFeeLibrary} from "./libraries/ProtocolFeeLibrary.sol"; @@ -45,7 +46,10 @@ abstract contract ProtocolFees is IProtocolFees, Owned { returns (uint256 amountCollected) { if (msg.sender != protocolFeeController) InvalidCaller.selector.revertWith(); - if (_isUnlocked()) ContractUnlocked.selector.revertWith(); + if (!currency.isAddressZero() && CurrencyReserves.getSyncedCurrency() == currency) { + // prevent transfer between the sync and settle balanceOfs (native settle uses msg.value) + ProtocolFeeCurrencySynced.selector.revertWith(); + } amountCollected = (amount == 0) ? protocolFeesAccrued[currency] : amount; protocolFeesAccrued[currency] -= amountCollected; diff --git a/src/interfaces/IProtocolFees.sol b/src/interfaces/IProtocolFees.sol index 4a9db7a8a..2c2af5944 100644 --- a/src/interfaces/IProtocolFees.sol +++ b/src/interfaces/IProtocolFees.sol @@ -10,12 +10,12 @@ interface IProtocolFees { /// @notice Thrown when protocol fee is set too high error ProtocolFeeTooLarge(uint24 fee); - /// @notice Thrown when the contract is unlocked - error ContractUnlocked(); - /// @notice Thrown when collectProtocolFees or setProtocolFee is not called by the controller. error InvalidCaller(); + /// @notice Thrown when collectProtocolFees is attempted on a token that is synced. + error ProtocolFeeCurrencySynced(); + /// @notice Emitted when the protocol fee controller address is updated in setProtocolFeeController. event ProtocolFeeControllerUpdated(address indexed protocolFeeController); diff --git a/src/test/ActionsRouter.sol b/src/test/ActionsRouter.sol index 8f39024d1..b1e2bed51 100644 --- a/src/test/ActionsRouter.sol +++ b/src/test/ActionsRouter.sol @@ -23,7 +23,8 @@ enum Actions { ASSERT_RESERVES_EQUALS, ASSERT_DELTA_EQUALS, ASSERT_NONZERO_DELTA_COUNT_EQUALS, - TRANSFER_FROM + TRANSFER_FROM, + COLLECT_PROTOCOL_FEES } // TODO: Add other actions as needed. // BURN, @@ -80,6 +81,8 @@ contract ActionsRouter is IUnlockCallback, Test { _assertNonzeroDeltaCountEquals(param); } else if (action == Actions.TRANSFER_FROM) { _transferFrom(param); + } else if (action == Actions.COLLECT_PROTOCOL_FEES) { + _collectProtocolFees(param); } } return ""; @@ -159,4 +162,9 @@ contract ActionsRouter is IUnlockCallback, Test { abi.decode(params, (Currency, address, address, uint256)); MockERC20(Currency.unwrap(currency)).transferFrom(from, recipient, uint256(amount)); } + + function _collectProtocolFees(bytes memory params) internal { + (address to, Currency currency, uint256 amount) = abi.decode(params, (address, Currency, uint256)); + manager.collectProtocolFees(to, currency, amount); + } } diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index bd3d838f2..d5be182ad 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -30,6 +30,7 @@ import {AmountHelpers} from "./utils/AmountHelpers.sol"; import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; import {IProtocolFees} from "../src/interfaces/IProtocolFees.sol"; import {StateLibrary} from "../src/libraries/StateLibrary.sol"; +import {TransientStateLibrary} from "../src/libraries/TransientStateLibrary.sol"; import {Actions} from "../src/test/ActionsRouter.sol"; contract PoolManagerTest is Test, Deployers { @@ -38,6 +39,7 @@ contract PoolManagerTest is Test, Deployers { using SafeCast for *; using ProtocolFeeLibrary for uint24; using StateLibrary for IPoolManager; + using TransientStateLibrary for IPoolManager; event UnlockCallback(); event ProtocolFeeControllerUpdated(address feeController); @@ -982,6 +984,46 @@ contract PoolManagerTest is Test, Deployers { manager.burn(address(this), key.currency0.toId(), 1); } + function test_collectProtocolFees_locked_revertsWithProtocolFeeCurrencySynced() public noIsolate { + manager.setProtocolFeeController(address(this)); + // currency1 is never native + manager.sync(key.currency1); + assertEq(Currency.unwrap(key.currency1), Currency.unwrap(manager.getSyncedCurrency())); + vm.expectRevert(IProtocolFees.ProtocolFeeCurrencySynced.selector); + manager.collectProtocolFees(address(this), key.currency1, 1); + } + + function test_sync_locked_collectProtocolFees_unlocked_revertsWithProtocolFeeCurrencySynced() public noIsolate { + manager.setProtocolFeeController(address(actionsRouter)); + manager.sync(key.currency1); + assertEq(Currency.unwrap(key.currency1), Currency.unwrap(manager.getSyncedCurrency())); + + Actions[] memory actions = new Actions[](1); + bytes[] memory params = new bytes[](1); + + actions[0] = Actions.COLLECT_PROTOCOL_FEES; + params[0] = abi.encode(address(this), key.currency1, 1); + + vm.expectRevert(IProtocolFees.ProtocolFeeCurrencySynced.selector); + actionsRouter.executeActions(actions, params); + } + + function test_collectProtocolFees_unlocked_revertsWithProtocolFeeCurrencySynced() public { + manager.setProtocolFeeController(address(actionsRouter)); + + Actions[] memory actions = new Actions[](2); + bytes[] memory params = new bytes[](2); + + actions[0] = Actions.SYNC; + params[0] = abi.encode(key.currency1); + + actions[1] = Actions.COLLECT_PROTOCOL_FEES; + params[1] = abi.encode(address(this), key.currency1, 1); + + vm.expectRevert(IProtocolFees.ProtocolFeeCurrencySynced.selector); + actionsRouter.executeActions(actions, params); + } + function test_collectProtocolFees_ERC20_accumulateFees_gas() public { uint256 expectedFees = 10; diff --git a/test/ProtocolFeesImplementation.t.sol b/test/ProtocolFeesImplementation.t.sol index 3abe6433d..de53a8c03 100644 --- a/test/ProtocolFeesImplementation.t.sol +++ b/test/ProtocolFeesImplementation.t.sol @@ -102,16 +102,6 @@ contract ProtocolFeesTest is Test, Deployers { protocolFees.collectProtocolFees(address(1), currency0, 0); } - function test_collectProtocolFees_revertsWithContractUnlocked() public { - protocolFees.setIsUnlocked(true); - - protocolFees.setProtocolFeeController(feeController); - vm.prank(feeController); - - vm.expectRevert(IProtocolFees.ContractUnlocked.selector); - protocolFees.collectProtocolFees(address(1), currency0, 0); - } - function test_collectProtocolFees_succeeds() public { // set a balance of protocol fees that can be collected protocolFees.updateProtocolFees(currency0, 100); diff --git a/test/Sync.t.sol b/test/Sync.t.sol index 199e7d25e..189f93c12 100644 --- a/test/Sync.t.sol +++ b/test/Sync.t.sol @@ -32,9 +32,11 @@ contract SyncTest is Test, Deployers { currency2 = deployMintAndApproveCurrency(); } - function test_settle_failsIfLocked() public { - vm.expectRevert(IPoolManager.ManagerLocked.selector); + function test_sync_multiple_unlocked() public noIsolate { + manager.sync(currency1); + assertEq(Currency.unwrap(currency1), Currency.unwrap(manager.getSyncedCurrency())); manager.sync(currency0); + assertEq(Currency.unwrap(currency0), Currency.unwrap(manager.getSyncedCurrency())); } function test_sync_balanceIsZero() public { diff --git a/test/utils/Deployers.sol b/test/utils/Deployers.sol index 27b179a4c..7cbc90cf9 100644 --- a/test/utils/Deployers.sol +++ b/test/utils/Deployers.sol @@ -74,6 +74,15 @@ contract Deployers is Test { uint160 hookPermissionCount = 14; uint160 clearAllHookPermissionsMask = ~uint160(0) << (hookPermissionCount); + modifier noIsolate() { + if (msg.sender != address(this)) { + (bool success,) = address(this).call(msg.data); + require(success); + } else { + _; + } + } + function deployFreshManager() internal virtual { manager = new PoolManager(); }