Solve ToB-UNI4-3 without restricting sync

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"
 }

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"
 }

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"
 }

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"
 }

snapshots/SyncTest.json

@@ -1,3 +1,3 @@
 {
-  "getReserves": "3931"
+  "getReserves": "3973"
 }

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"
 }

snapshots/TestDynamicReturnFees.json

@@ -1,3 +1,3 @@
 {
-  "swap with return dynamic fee": "145589"
+  "swap with return dynamic fee": "145470"
 }

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.

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;

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);
 

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);
+    }
 }

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;
 

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);

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 {

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();
     }