diff --git a/snapshots/CustomAccountingTest.json b/snapshots/CustomAccountingTest.json index 3d75f4228..348909c5f 100644 --- a/snapshots/CustomAccountingTest.json +++ b/snapshots/CustomAccountingTest.json @@ -1,6 +1,6 @@ { - "addLiquidity CA fee": "170690", - "removeLiquidity CA fee": "141194", - "swap CA custom curve + swap noop": "124397", - "swap CA fee on unspecified": "154567" + "addLiquidity CA fee": "170695", + "removeLiquidity CA fee": "141199", + "swap CA custom curve + swap noop": "124402", + "swap CA fee on unspecified": "154572" } \ No newline at end of file diff --git a/snapshots/PoolManagerTest.json b/snapshots/PoolManagerTest.json index 0db8f1537..4b6cd6039 100644 --- a/snapshots/PoolManagerTest.json +++ b/snapshots/PoolManagerTest.json @@ -1,12 +1,12 @@ { - "addLiquidity with empty hook": "274002", + "addLiquidity with empty hook": "274012", "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", + "poolManager bytecode size": "24050", + "removeLiquidity with empty hook": "130613", "removeLiquidity with native token": "112523", "simple addLiquidity": "161276", "simple addLiquidity second addition same range": "98731", @@ -20,5 +20,5 @@ "swap burn native 6909 for input": "118672", "swap mint native output as 6909": "139620", "swap mint output as 6909": "154985", - "swap with hooks": "132155" + "swap with hooks": "132165" } \ No newline at end of file diff --git a/snapshots/SkipCallsTest.json b/snapshots/SkipCallsTest.json index e5a1427e1..6aa00e361 100644 --- a/snapshots/SkipCallsTest.json +++ b/snapshots/SkipCallsTest.json @@ -1,3 +1,3 @@ { - "swap skips hook call if hook is caller": "206025" + "swap skips hook call if hook is caller": "206030" } \ No newline at end of file diff --git a/snapshots/TestDynamicFees.json b/snapshots/TestDynamicFees.json index 8f16d78f2..b25bd6b18 100644 --- a/snapshots/TestDynamicFees.json +++ b/snapshots/TestDynamicFees.json @@ -1,4 +1,4 @@ { "swap with dynamic fee": "139153", - "update dynamic fee in before swap": "147738" + "update dynamic fee in before swap": "147743" } \ No newline at end of file diff --git a/snapshots/TestDynamicReturnFees.json b/snapshots/TestDynamicReturnFees.json index 435a2a631..d2635299d 100644 --- a/snapshots/TestDynamicReturnFees.json +++ b/snapshots/TestDynamicReturnFees.json @@ -1,3 +1,3 @@ { - "swap with return dynamic fee": "145470" + "swap with return dynamic fee": "145475" } \ No newline at end of file diff --git a/src/libraries/CustomRevert.sol b/src/libraries/CustomRevert.sol index 8ea27d2e4..5d7073308 100644 --- a/src/libraries/CustomRevert.sol +++ b/src/libraries/CustomRevert.sol @@ -7,6 +7,9 @@ pragma solidity ^0.8.0; /// `CustomError.selector.revertWith()` /// @dev The functions may tamper with the free memory pointer but it is fine since the call context is exited immediately library CustomRevert { + /// @dev ERC-7751 error for wrapping bubbled up reverts + error WrappedError(address target, bytes4 selector, bytes reason, bytes details); + /// @dev Reverts with the selector of a custom error in the scratch space function revertWith(bytes4 selector) internal pure { assembly ("memory-safe") { @@ -75,23 +78,43 @@ library CustomRevert { } } - /// @notice bubble up the revert message returned by a call and revert with the selector provided - /// @dev this function should only be used with custom errors of the type `CustomError(address target, bytes revertReason)` - function bubbleUpAndRevertWith(bytes4 selector, address addr) internal pure { + /// @notice bubble up the revert message returned by a call and revert with a wrapped ERC-7751 error + /// @dev this method can be vulnerable to revert data bombs + function bubbleUpAndRevertWith( + address revertingContract, + bytes4 revertingFunctionSelector, + bytes4 additionalContext + ) internal pure { + bytes4 wrappedErrorSelector = WrappedError.selector; assembly ("memory-safe") { - let size := returndatasize() - let fmp := mload(0x40) + // Ensure the size of the revert data is a multiple of 32 bytes + let encodedDataSize := mul(div(add(returndatasize(), 31), 32), 32) - // Encode selector, address, offset, size, data - mstore(fmp, selector) - mstore(add(fmp, 0x04), addr) - mstore(add(fmp, 0x24), 0x40) - mstore(add(fmp, 0x44), size) - returndatacopy(add(fmp, 0x64), 0, size) + let fmp := mload(0x40) - // Ensure the size is a multiple of 32 bytes - let encodedSize := add(0x64, mul(div(add(size, 31), 32), 32)) - revert(fmp, encodedSize) + // Encode wrapped error selector, address, function selector, offset, additional context, size, revert reason + mstore(fmp, wrappedErrorSelector) + mstore(add(fmp, 0x04), and(revertingContract, 0xffffffffffffffffffffffffffffffffffffffff)) + mstore( + add(fmp, 0x24), + and(revertingFunctionSelector, 0xffffffff00000000000000000000000000000000000000000000000000000000) + ) + // offset revert reason + mstore(add(fmp, 0x44), 0x80) + // offset additional context + mstore(add(fmp, 0x64), add(0xa0, encodedDataSize)) + // size revert reason + mstore(add(fmp, 0x84), returndatasize()) + // revert reason + returndatacopy(add(fmp, 0xa4), 0, returndatasize()) + // size additional context + mstore(add(fmp, add(0xa4, encodedDataSize)), 0x04) + // additional context + mstore( + add(fmp, add(0xc4, encodedDataSize)), + and(additionalContext, 0xffffffff00000000000000000000000000000000000000000000000000000000) + ) + revert(fmp, add(0xe4, encodedDataSize)) } } } diff --git a/src/libraries/Hooks.sol b/src/libraries/Hooks.sol index d7c9ed328..0b8c869da 100644 --- a/src/libraries/Hooks.sol +++ b/src/libraries/Hooks.sol @@ -69,9 +69,8 @@ library Hooks { /// @notice Hook did not return its selector error InvalidHookResponse(); - /// @notice thrown when a hook call fails - /// @param revertReason bubbled up revert reason - error Wrap__FailedHookCall(address hook, bytes revertReason); + /// @notice Additional context for ERC-7751 wrapped error when a hook call fails + error HookCallFailed(); /// @notice The hook's delta changed the swap from exactIn to exactOut or vice versa error HookDeltaExceedsSwapAmount(); @@ -134,7 +133,7 @@ library Hooks { success := call(gas(), self, 0, add(data, 0x20), mload(data), 0, 0) } // Revert with FailedHookCall, containing any error message to bubble up - if (!success) Wrap__FailedHookCall.selector.bubbleUpAndRevertWith(address(self)); + if (!success) CustomRevert.bubbleUpAndRevertWith(address(self), bytes4(data), HookCallFailed.selector); // The call was successful, fetch the returned data assembly ("memory-safe") { diff --git a/src/types/Currency.sol b/src/types/Currency.sol index 47cb077de..771653400 100644 --- a/src/types/Currency.sol +++ b/src/types/Currency.sol @@ -28,15 +28,11 @@ function greaterThanOrEqualTo(Currency currency, Currency other) pure returns (b /// @title CurrencyLibrary /// @dev This library allows for transferring and holding native tokens and ERC20 tokens library CurrencyLibrary { - using CustomRevert for bytes4; + /// @notice Additional context for ERC-7751 wrapped error when a native transfer fails + error NativeTransferFailed(); - /// @notice Thrown when a native transfer fails - /// @param reason bubbled up revert reason - error Wrap__NativeTransferFailed(address recipient, bytes reason); - - /// @notice Thrown when an ERC20 transfer fails - /// @param reason bubbled up revert reason - error Wrap__ERC20TransferFailed(address token, bytes reason); + /// @notice Additional context for ERC-7751 wrapped error when an ERC20 transfer fails + error ERC20TransferFailed(); /// @notice A constant to represent the native currency Currency public constant ADDRESS_ZERO = Currency.wrap(address(0)); @@ -52,7 +48,9 @@ library CurrencyLibrary { success := call(gas(), to, amount, 0, 0, 0, 0) } // revert with NativeTransferFailed, containing the bubbled up error as an argument - if (!success) Wrap__NativeTransferFailed.selector.bubbleUpAndRevertWith(to); + if (!success) { + CustomRevert.bubbleUpAndRevertWith(to, bytes4(0), NativeTransferFailed.selector); + } } else { assembly ("memory-safe") { // Get a pointer to some free memory. @@ -81,7 +79,11 @@ library CurrencyLibrary { mstore(add(fmp, 0x40), 0) // 4 bytes of `amount` were stored here } // revert with ERC20TransferFailed, containing the bubbled up error as an argument - if (!success) Wrap__ERC20TransferFailed.selector.bubbleUpAndRevertWith(Currency.unwrap(currency)); + if (!success) { + CustomRevert.bubbleUpAndRevertWith( + Currency.unwrap(currency), IERC20Minimal.transfer.selector, ERC20TransferFailed.selector + ); + } } } diff --git a/test/DynamicFees.t.sol b/test/DynamicFees.t.sol index 8dd80bb1f..1dfc69599 100644 --- a/test/DynamicFees.t.sol +++ b/test/DynamicFees.t.sol @@ -19,6 +19,7 @@ import {MockERC20} from "solmate/src/test/utils/mocks/MockERC20.sol"; import {Pool} from "../src/libraries/Pool.sol"; import {BalanceDelta, BalanceDeltaLibrary} from "../src/types/BalanceDelta.sol"; import {StateLibrary} from "../src/libraries/StateLibrary.sol"; +import {CustomRevert} from "../src/libraries/CustomRevert.sol"; import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol"; contract TestDynamicFees is Test, Deployers { @@ -70,9 +71,11 @@ contract TestDynamicFees is Test, Deployers { vm.expectRevert( abi.encodeWithSelector( - Hooks.Wrap__FailedHookCall.selector, + CustomRevert.WrappedError.selector, address(dynamicFeesHooks), - abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee) + IHooks.afterInitialize.selector, + abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee), + abi.encodeWithSelector(Hooks.HookCallFailed.selector) ) ); manager.initialize(key, SQRT_PRICE_1_1); @@ -108,9 +111,11 @@ contract TestDynamicFees is Test, Deployers { // afterInitialize will try to update the fee, and fail vm.expectRevert( abi.encodeWithSelector( - Hooks.Wrap__FailedHookCall.selector, + CustomRevert.WrappedError.selector, address(dynamicFeesHooks), - abi.encodeWithSelector(IPoolManager.UnauthorizedDynamicLPFeeUpdate.selector) + IHooks.afterInitialize.selector, + abi.encodeWithSelector(IPoolManager.UnauthorizedDynamicLPFeeUpdate.selector), + abi.encodeWithSelector(Hooks.HookCallFailed.selector) ) ); manager.initialize(key, SQRT_PRICE_1_1); @@ -127,11 +132,14 @@ contract TestDynamicFees is Test, Deployers { vm.expectRevert( abi.encodeWithSelector( - Hooks.Wrap__FailedHookCall.selector, + CustomRevert.WrappedError.selector, address(dynamicFeesHooks), - abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee) + IHooks.beforeSwap.selector, + abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee), + abi.encodeWithSelector(Hooks.HookCallFailed.selector) ) ); + swapRouter.swap(key, SWAP_PARAMS, testSettings, ZERO_BYTES); } diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol index d5be182ad..e469b25be 100644 --- a/test/PoolManager.t.sol +++ b/test/PoolManager.t.sol @@ -32,6 +32,7 @@ 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"; +import {CustomRevert} from "../src/libraries/CustomRevert.sol"; contract PoolManagerTest is Test, Deployers { using Hooks for IHooks; @@ -930,7 +931,11 @@ contract PoolManagerTest is Test, Deployers { (uint256 amount0, uint256 amount1) = currency0Invalid ? (1, 0) : (0, 1); vm.expectRevert( abi.encodeWithSelector( - CurrencyLibrary.Wrap__ERC20TransferFailed.selector, invalidToken, abi.encode(bytes32(0)) + CustomRevert.WrappedError.selector, + address(invalidToken), + TestInvalidERC20.transfer.selector, + abi.encode(bytes32(0)), + abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector) ) ); takeRouter.take(key, amount0, amount1); diff --git a/test/libraries/Hooks.t.sol b/test/libraries/Hooks.t.sol index 6305e09cd..6d7da20a1 100644 --- a/test/libraries/Hooks.t.sol +++ b/test/libraries/Hooks.t.sol @@ -23,6 +23,7 @@ import {BaseTestHooks} from "../../src/test/BaseTestHooks.sol"; import {EmptyRevertContract} from "../../src/test/EmptyRevertContract.sol"; import {StateLibrary} from "../../src/libraries/StateLibrary.sol"; import {Constants} from "../utils/Constants.sol"; +import {CustomRevert} from "../../src/libraries/CustomRevert.sol"; contract HooksTest is Test, Deployers { using Hooks for IHooks; @@ -1010,9 +1011,11 @@ contract HooksTest is Test, Deployers { vm.expectRevert( abi.encodeWithSelector( - Hooks.Wrap__FailedHookCall.selector, + CustomRevert.WrappedError.selector, address(revertingHook), - abi.encodeWithSelector(BaseTestHooks.HookNotImplemented.selector) + IHooks.beforeSwap.selector, + abi.encodeWithSelector(BaseTestHooks.HookNotImplemented.selector), + abi.encodeWithSelector(Hooks.HookCallFailed.selector) ) ); swapRouter.swap(key, swapParams, testSettings, new bytes(0)); @@ -1035,7 +1038,13 @@ contract HooksTest is Test, Deployers { PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false}); vm.expectRevert( - abi.encodeWithSelector(Hooks.Wrap__FailedHookCall.selector, address(revertingHook), new bytes(0)) + abi.encodeWithSelector( + CustomRevert.WrappedError.selector, + address(beforeSwapFlag), + IHooks.beforeSwap.selector, + "", + abi.encodeWithSelector(Hooks.HookCallFailed.selector) + ) ); swapRouter.swap(key, swapParams, testSettings, new bytes(0)); } diff --git a/test/types/Currency.t.sol b/test/types/Currency.t.sol index 1ca8e02d2..88f897e1e 100644 --- a/test/types/Currency.t.sol +++ b/test/types/Currency.t.sol @@ -3,10 +3,11 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; import {stdError} from "forge-std/StdError.sol"; -import {MockERC20} from "solmate/src/test/utils/mocks/MockERC20.sol"; +import {MockERC20, ERC20} from "solmate/src/test/utils/mocks/MockERC20.sol"; import {Currency, CurrencyLibrary} from "../../src/types/Currency.sol"; import {CurrencyTest} from "../../src/test/CurrencyTest.sol"; import {EmptyRevertContract} from "../../src/test/EmptyRevertContract.sol"; +import {CustomRevert} from "../../src/libraries/CustomRevert.sol"; contract TestCurrency is Test { uint256 constant initialERC20Balance = 1000 ether; @@ -123,9 +124,11 @@ contract TestCurrency is Test { // the token reverts with no data, so our custom error will be emitted instead vm.expectRevert( abi.encodeWithSelector( - CurrencyLibrary.Wrap__ERC20TransferFailed.selector, - Currency.unwrap(Currency.wrap(address(emptyRevertingToken))), - new bytes(0) + CustomRevert.WrappedError.selector, + address(emptyRevertingToken), + ERC20.transfer.selector, + "", + abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector) ) ); currencyTest.transfer(Currency.wrap(address(emptyRevertingToken)), otherAddress, 100); @@ -141,7 +144,13 @@ contract TestCurrency is Test { assertEq(address(currencyTest).balance, contractBalanceBefore - amount); } else { vm.expectRevert( - abi.encodeWithSelector(CurrencyLibrary.Wrap__NativeTransferFailed.selector, otherAddress, new bytes(0)) + abi.encodeWithSelector( + CustomRevert.WrappedError.selector, + otherAddress, + bytes4(0), + new bytes(0), + abi.encodeWithSelector(CurrencyLibrary.NativeTransferFailed.selector) + ) ); currencyTest.transfer(nativeCurrency, otherAddress, amount); assertEq(otherAddress.balance, balanceBefore); @@ -161,9 +170,11 @@ contract TestCurrency is Test { // the token reverts with an overflow error message, so this is bubbled up vm.expectRevert( abi.encodeWithSelector( - CurrencyLibrary.Wrap__ERC20TransferFailed.selector, + CustomRevert.WrappedError.selector, Currency.unwrap(erc20Currency), - stdError.arithmeticError + ERC20.transfer.selector, + stdError.arithmeticError, + abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector) ) ); currencyTest.transfer(erc20Currency, otherAddress, amount);