From 9c94b1a8ded5b5c09bee0ed9526d10065756b83d Mon Sep 17 00:00:00 2001 From: Sara Reynolds <30504811+snreynolds@users.noreply.github.com> Date: Wed, 4 Sep 2024 19:45:54 -0400 Subject: [PATCH] SB-19: Remove revert sync (#866) * override syncedCurrency slot * tests * comments * remove compiler warnings * comments --- ...o already existing position with salt.snap | 2 +- .forge-snapshots/addLiquidity CA fee.snap | 2 +- .../addLiquidity with empty hook.snap | 2 +- .../addLiquidity with native token.snap | 2 +- ...new liquidity to a position with salt.snap | 2 +- .forge-snapshots/donate gas with 1 token.snap | 2 +- .../donate gas with 2 tokens.snap | 2 +- .../poolManager bytecode size.snap | 2 +- ...dLiquidity second addition same range.snap | 2 +- .forge-snapshots/simple addLiquidity.snap | 2 +- .forge-snapshots/simple swap.snap | 2 +- .../swap CA custom curve + swap noop.snap | 2 +- .../swap CA fee on unspecified.snap | 2 +- .forge-snapshots/swap against liquidity.snap | 2 +- .../swap mint native output as 6909.snap | 2 +- .../swap mint output as 6909.snap | 2 +- ...wap skips hook call if hook is caller.snap | 2 +- .forge-snapshots/swap with dynamic fee.snap | 2 +- .forge-snapshots/swap with hooks.snap | 2 +- .../swap with lp fee and protocol fee.snap | 2 +- .../swap with return dynamic fee.snap | 2 +- .../update dynamic fee in before swap.snap | 2 +- src/PoolManager.sol | 17 ++-- src/interfaces/IPoolManager.sol | 3 +- src/libraries/CurrencyReserves.sol | 9 -- src/test/ProxyPoolManager.sol | 12 ++- test/PoolManager.t.sol | 10 +- test/Sync.t.sol | 95 +++++++++++++++++-- 28 files changed, 136 insertions(+), 54 deletions(-) diff --git a/.forge-snapshots/add liquidity to already existing position with salt.snap b/.forge-snapshots/add liquidity to already existing position with salt.snap index 3826ab44b..7931cc0d3 100644 --- a/.forge-snapshots/add liquidity to already existing position with salt.snap +++ b/.forge-snapshots/add liquidity to already existing position with salt.snap @@ -1 +1 @@ -144910 \ No newline at end of file +144608 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity CA fee.snap b/.forge-snapshots/addLiquidity CA fee.snap index f93d89302..329e55959 100644 --- a/.forge-snapshots/addLiquidity CA fee.snap +++ b/.forge-snapshots/addLiquidity CA fee.snap @@ -1 +1 @@ -171199 \ No newline at end of file +170897 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap index e3dcc3d88..cb223237c 100644 --- a/.forge-snapshots/addLiquidity with empty hook.snap +++ b/.forge-snapshots/addLiquidity with empty hook.snap @@ -1 +1 @@ -274477 \ No newline at end of file +274175 \ No newline at end of file diff --git a/.forge-snapshots/addLiquidity with native token.snap b/.forge-snapshots/addLiquidity with native token.snap index 571d4b29b..3ec6fa53a 100644 --- a/.forge-snapshots/addLiquidity with native token.snap +++ b/.forge-snapshots/addLiquidity with native token.snap @@ -1 +1 @@ -135252 \ No newline at end of file +135101 \ No newline at end of file diff --git a/.forge-snapshots/create new liquidity to a position with salt.snap b/.forge-snapshots/create new liquidity to a position with salt.snap index 8f81e13fd..ebcbdf72c 100644 --- a/.forge-snapshots/create new liquidity to a position with salt.snap +++ b/.forge-snapshots/create new liquidity to a position with salt.snap @@ -1 +1 @@ -293102 \ No newline at end of file +292800 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 1 token.snap b/.forge-snapshots/donate gas with 1 token.snap index a120e8bee..a54ba22c4 100644 --- a/.forge-snapshots/donate gas with 1 token.snap +++ b/.forge-snapshots/donate gas with 1 token.snap @@ -1 +1 @@ -106472 \ No newline at end of file +106321 \ No newline at end of file diff --git a/.forge-snapshots/donate gas with 2 tokens.snap b/.forge-snapshots/donate gas with 2 tokens.snap index 91dedf594..60eb0640f 100644 --- a/.forge-snapshots/donate gas with 2 tokens.snap +++ b/.forge-snapshots/donate gas with 2 tokens.snap @@ -1 +1 @@ -146038 \ No newline at end of file +145736 \ No newline at end of file diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index 2111d9fca..991b253d9 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -24081 \ No newline at end of file +24006 \ No newline at end of file diff --git a/.forge-snapshots/simple addLiquidity second addition same range.snap b/.forge-snapshots/simple addLiquidity second addition same range.snap index b152d091a..513cda23b 100644 --- a/.forge-snapshots/simple addLiquidity second addition same range.snap +++ b/.forge-snapshots/simple addLiquidity second addition same range.snap @@ -1 +1 @@ -98970 \ No newline at end of file +98819 \ No newline at end of file diff --git a/.forge-snapshots/simple addLiquidity.snap b/.forge-snapshots/simple addLiquidity.snap index 28cc98e19..0d3371c3f 100644 --- a/.forge-snapshots/simple addLiquidity.snap +++ b/.forge-snapshots/simple addLiquidity.snap @@ -1 +1 @@ -161515 \ No newline at end of file +161364 \ No newline at end of file diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap index e94e8cc60..3e232d361 100644 --- a/.forge-snapshots/simple swap.snap +++ b/.forge-snapshots/simple swap.snap @@ -1 +1 @@ -123337 \ No newline at end of file +123186 \ No newline at end of file diff --git a/.forge-snapshots/swap CA custom curve + swap noop.snap b/.forge-snapshots/swap CA custom curve + swap noop.snap index 64fa20927..8dc70d3a8 100644 --- a/.forge-snapshots/swap CA custom curve + swap noop.snap +++ b/.forge-snapshots/swap CA custom curve + swap noop.snap @@ -1 +1 @@ -127123 \ No newline at end of file +126821 \ No newline at end of file diff --git a/.forge-snapshots/swap CA fee on unspecified.snap b/.forge-snapshots/swap CA fee on unspecified.snap index 2485c9778..0650bd07a 100644 --- a/.forge-snapshots/swap CA fee on unspecified.snap +++ b/.forge-snapshots/swap CA fee on unspecified.snap @@ -1 +1 @@ -154782 \ No newline at end of file +154631 \ No newline at end of file diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap index 0dece8faf..7a0df15bb 100644 --- a/.forge-snapshots/swap against liquidity.snap +++ b/.forge-snapshots/swap against liquidity.snap @@ -1 +1 @@ -116709 \ No newline at end of file +116558 \ No newline at end of file diff --git a/.forge-snapshots/swap mint native output as 6909.snap b/.forge-snapshots/swap mint native output as 6909.snap index 4ac6b2691..41417754a 100644 --- a/.forge-snapshots/swap mint native output as 6909.snap +++ b/.forge-snapshots/swap mint native output as 6909.snap @@ -1 +1 @@ -139754 \ No newline at end of file +139603 \ No newline at end of file diff --git a/.forge-snapshots/swap mint output as 6909.snap b/.forge-snapshots/swap mint output as 6909.snap index e231b294e..f1a50bc46 100644 --- a/.forge-snapshots/swap mint output as 6909.snap +++ b/.forge-snapshots/swap mint output as 6909.snap @@ -1 +1 @@ -155178 \ No newline at end of file +155027 \ No newline at end of file diff --git a/.forge-snapshots/swap skips hook call if hook is caller.snap b/.forge-snapshots/swap skips hook call if hook is caller.snap index 1d836eb75..0ab4deeab 100644 --- a/.forge-snapshots/swap skips hook call if hook is caller.snap +++ b/.forge-snapshots/swap skips hook call if hook is caller.snap @@ -1 +1 @@ -206434 \ No newline at end of file +206132 \ No newline at end of file diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap index 8f0535d0c..3c8c45efc 100644 --- a/.forge-snapshots/swap with dynamic fee.snap +++ b/.forge-snapshots/swap with dynamic fee.snap @@ -1 +1 @@ -139358 \ No newline at end of file +139207 \ No newline at end of file diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap index 7499dbc93..185d91737 100644 --- a/.forge-snapshots/swap with hooks.snap +++ b/.forge-snapshots/swap with hooks.snap @@ -1 +1 @@ -132346 \ No newline at end of file +132195 \ No newline at end of file diff --git a/.forge-snapshots/swap with lp fee and protocol fee.snap b/.forge-snapshots/swap with lp fee and protocol fee.snap index 3ca07ea1c..a0a94cd21 100644 --- a/.forge-snapshots/swap with lp fee and protocol fee.snap +++ b/.forge-snapshots/swap with lp fee and protocol fee.snap @@ -1 +1 @@ -169549 \ No newline at end of file +169398 \ No newline at end of file diff --git a/.forge-snapshots/swap with return dynamic fee.snap b/.forge-snapshots/swap with return dynamic fee.snap index 79ca694af..37efa1363 100644 --- a/.forge-snapshots/swap with return dynamic fee.snap +++ b/.forge-snapshots/swap with return dynamic fee.snap @@ -1 +1 @@ -145685 \ No newline at end of file +145534 \ No newline at end of file diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap index 155001380..4394bf909 100644 --- a/.forge-snapshots/update dynamic fee in before swap.snap +++ b/.forge-snapshots/update dynamic fee in before swap.snap @@ -1 +1 @@ -147965 \ No newline at end of file +147814 \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index 5cfbecbe4..3fb55aaff 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -276,11 +276,14 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim /// @inheritdoc IPoolManager function sync(Currency currency) external onlyWhenUnlocked { - CurrencyReserves.requireNotSynced(); // address(0) is used for the native currency - if (currency.isAddressZero()) return; - uint256 balance = currency.balanceOfSelf(); - CurrencyReserves.syncCurrencyAndReserves(currency, balance); + if (currency.isAddressZero()) { + // The reserves balance is not used for native settling, so we only need to reset the currency. + CurrencyReserves.resetCurrency(); + } else { + uint256 balance = currency.balanceOfSelf(); + CurrencyReserves.syncCurrencyAndReserves(currency, balance); + } } /// @inheritdoc IPoolManager @@ -343,17 +346,19 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim function _settle(address recipient) internal returns (uint256 paid) { Currency currency = CurrencyReserves.getSyncedCurrency(); - // if not previously synced, expects native currency to be settled + + // if not previously synced, or the syncedCurrency slot has been reset, expects native currency to be settled if (currency.isAddressZero()) { paid = msg.value; } else { if (msg.value > 0) NonzeroNativeValue.selector.revertWith(); - // Reserves are guaranteed to be set, because currency and reserves are always set together + // Reserves are guaranteed to be set because currency and reserves are always set together uint256 reservesBefore = CurrencyReserves.getSyncedReserves(); uint256 reservesNow = currency.balanceOfSelf(); paid = reservesNow - reservesBefore; CurrencyReserves.resetCurrency(); } + _accountDelta(currency, paid.toInt128(), recipient); } diff --git a/src/interfaces/IPoolManager.sol b/src/interfaces/IPoolManager.sol index f0b9728dc..dc5b60b49 100644 --- a/src/interfaces/IPoolManager.sol +++ b/src/interfaces/IPoolManager.sol @@ -184,7 +184,8 @@ interface IPoolManager is IProtocolFees, IERC6909Claims, IExtsload, IExttload { /// This is used to checkpoint balances for the manager and derive deltas for the caller. /// @dev This MUST be called before any ERC20 tokens are sent into the contract, but can be skipped /// for native tokens because the amount to settle is determined by the sent value. - /// @param currency The currency whose balance to sync + /// However, if an ERC20 token has been synced and not settled, and the caller instead wants to settle + /// native funds, this function can be called with the native currency to then be able to settle the native currency function sync(Currency currency) external; /// @notice Called by the user to net out some value owed to the user diff --git a/src/libraries/CurrencyReserves.sol b/src/libraries/CurrencyReserves.sol index 59d3b954f..3a3309c33 100644 --- a/src/libraries/CurrencyReserves.sol +++ b/src/libraries/CurrencyReserves.sol @@ -7,20 +7,11 @@ import {CustomRevert} from "./CustomRevert.sol"; library CurrencyReserves { using CustomRevert for bytes4; - /// @notice Thrown when a user has already synced a currency, but not yet settled - error AlreadySynced(); - /// bytes32(uint256(keccak256("ReservesOf")) - 1) bytes32 constant RESERVES_OF_SLOT = 0x1e0745a7db1623981f0b2a5d4232364c00787266eb75ad546f190e6cebe9bd95; /// bytes32(uint256(keccak256("Currency")) - 1) bytes32 constant CURRENCY_SLOT = 0x27e098c505d44ec3574004bca052aabf76bd35004c182099d8c575fb238593b9; - function requireNotSynced() internal view { - if (!getSyncedCurrency().isAddressZero()) { - AlreadySynced.selector.revertWith(); - } - } - function getSyncedCurrency() internal view returns (Currency currency) { assembly ("memory-safe") { currency := tload(CURRENCY_SLOT) diff --git a/src/test/ProxyPoolManager.sol b/src/test/ProxyPoolManager.sol index f8f177be2..b82bf8428 100644 --- a/src/test/ProxyPoolManager.sol +++ b/src/test/ProxyPoolManager.sol @@ -141,10 +141,14 @@ contract ProxyPoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909 /// @inheritdoc IPoolManager function sync(Currency currency) public { - CurrencyReserves.requireNotSynced(); - if (currency.isAddressZero()) return; - uint256 balance = currency.balanceOfSelf(); - CurrencyReserves.syncCurrencyAndReserves(currency, balance); + // 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. + CurrencyReserves.resetCurrency(); + } else { + uint256 balance = currency.balanceOfSelf(); + CurrencyReserves.syncCurrencyAndReserves(currency, balance); + } } /// @inheritdoc IPoolManager diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index e2d90405d..cb8c8fa73 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -1135,16 +1135,16 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot { emptyUnlockRouter.unlock(); } - Action[] actions; + Action[] _actions; function test_unlock_cannotBeCalledTwiceByCaller() public { - actions = [Action.NESTED_SELF_UNLOCK]; - nestedActionRouter.unlock(abi.encode(actions)); + _actions = [Action.NESTED_SELF_UNLOCK]; + nestedActionRouter.unlock(abi.encode(_actions)); } function test_unlock_cannotBeCalledTwiceByDifferentCallers() public { - actions = [Action.NESTED_EXECUTOR_UNLOCK]; - nestedActionRouter.unlock(abi.encode(actions)); + _actions = [Action.NESTED_EXECUTOR_UNLOCK]; + nestedActionRouter.unlock(abi.encode(_actions)); } // function testExtsloadForPoolPrice() public { diff --git a/test/Sync.t.sol b/test/Sync.t.sol index 598160f0d..bc70386fb 100644 --- a/test/Sync.t.sol +++ b/test/Sync.t.sol @@ -19,6 +19,7 @@ import {StateLibrary} from "../src/libraries/StateLibrary.sol"; import {TransientStateLibrary} from "../src/libraries/TransientStateLibrary.sol"; import {NativeERC20} from "../src/test/NativeERC20.sol"; import {IPoolManager} from "../src/interfaces/IPoolManager.sol"; +import {CurrencyLibrary} from "../src/types/Currency.sol"; contract SyncTest is Test, Deployers, GasSnapshot { using StateLibrary for IPoolManager; @@ -40,8 +41,8 @@ contract SyncTest is Test, Deployers, GasSnapshot { function test_sync_balanceIsZero() public { assertEq(currency2.balanceOf(address(manager)), uint256(0)); - Actions[] memory actions = new Actions[](4); - bytes[] memory params = new bytes[](4); + Actions[] memory actions = new Actions[](2); + bytes[] memory params = new bytes[](2); actions[0] = Actions.SYNC; params[0] = abi.encode(currency2); @@ -255,15 +256,19 @@ contract SyncTest is Test, Deployers, GasSnapshot { vm.deal(address(manager), value); NativeERC20 nativeERC20 = new NativeERC20(); - Actions[] memory actions = new Actions[](2); - bytes[] memory params = new bytes[](2); + uint256 nativeERC20Balance = nativeERC20.balanceOf(address(manager)); + + Actions[] memory actions = new Actions[](3); + bytes[] memory params = new bytes[](3); actions[0] = Actions.SYNC; params[0] = abi.encode(nativeERC20); - // Revert with NonzeroNativeValue - actions[1] = Actions.SETTLE_NATIVE; - params[1] = abi.encode(value); + actions[1] = Actions.ASSERT_RESERVES_EQUALS; + params[1] = abi.encode(nativeERC20Balance); + + actions[2] = Actions.SETTLE_NATIVE; + params[2] = abi.encode(value); vm.expectRevert(IPoolManager.NonzeroNativeValue.selector); actionsRouter.executeActions{value: value}(actions, params); @@ -289,4 +294,80 @@ contract SyncTest is Test, Deployers, GasSnapshot { // uint256 balanceAfter = address(this).balance; // assertEq(balanceAfter - balanceBefore, value); } + + function test_settle_native_afterERC20Sync_succeeds(uint256 currency2Balance, uint256 ethBalance) public { + currency2Balance = bound(currency2Balance, 1, uint256(int256(type(int128).max / 2))); + ethBalance = bound(ethBalance, 1, uint256(int256(type(int128).max / 2))); + + vm.deal(address(this), ethBalance); + // ensure the reserves balance is non 0 + currency2.transfer(address(manager), currency2Balance); + + Actions[] memory actions = new Actions[](8); + bytes[] memory params = new bytes[](8); + + actions[0] = Actions.ASSERT_RESERVES_EQUALS; + params[0] = abi.encode(0); + + actions[1] = Actions.SYNC; + params[1] = abi.encode(currency2); + + actions[2] = Actions.ASSERT_RESERVES_EQUALS; + params[2] = abi.encode(currency2Balance); + + actions[3] = Actions.SYNC; + params[3] = abi.encode(CurrencyLibrary.ADDRESS_ZERO); + + // Under the hood this is non-zero but our transient state library overrides the value if the currency is address(0) + actions[4] = Actions.ASSERT_RESERVES_EQUALS; + params[4] = abi.encode(0); + + // This calls settle with a value, of ethBalance. Since the synedCurrency slot is address(0), the call should successfully apply a positive delta on the native currency. + actions[5] = Actions.SETTLE_NATIVE; + params[5] = abi.encode(ethBalance); + + actions[6] = Actions.ASSERT_DELTA_EQUALS; + params[6] = abi.encode(CurrencyLibrary.ADDRESS_ZERO, address(actionsRouter), ethBalance); + + // take the eth to close the deltas + actions[7] = Actions.TAKE; + params[7] = abi.encode(CurrencyLibrary.ADDRESS_ZERO, address(this), ethBalance); + + actionsRouter.executeActions{value: ethBalance}(actions, params); + } + + function test_settle_twice_doesNotApplyDelta(uint256 value) public { + value = bound(value, 1, uint256(int256(type(int128).max / 2))); + currency2.transfer(address(manager), value); + + Actions[] memory actions = new Actions[](8); + bytes[] memory params = new bytes[](8); + + actions[0] = Actions.SYNC; + params[0] = abi.encode(currency2); + + actions[1] = Actions.ASSERT_RESERVES_EQUALS; + params[1] = abi.encode(value); + + actions[2] = Actions.TRANSFER_FROM; + params[2] = abi.encode(currency2, address(this), address(manager), value); + + // This settles the syncedCurrency, currency2. + actions[3] = Actions.SETTLE; + + actions[4] = Actions.ASSERT_DELTA_EQUALS; + params[4] = abi.encode(currency2, address(actionsRouter), value); + + actions[5] = Actions.TAKE; + params[5] = abi.encode(currency2, address(this), value); + + // This settles the syncedCurrency, which has been cleared to address(0). + actions[6] = Actions.SETTLE; + + // Calling settle on address(0) does not apply a delta when called with no value. + actions[7] = Actions.ASSERT_DELTA_EQUALS; + params[7] = abi.encode(CurrencyLibrary.ADDRESS_ZERO, address(actionsRouter), 0); + + actionsRouter.executeActions(actions, params); + } }