Update ERC-7751 revert bubbling (#889)

* Update ERC-7751 revert bubbling

* fix: use known selector instead of hard coded

* rename arguments

* fix: lint

* clear bits

* format

* update gas snapshots

* use interface for transfer selector

* remove `CustomRevert` library use for bytes4

* use `returndatasize` in place

---------

Co-authored-by: Mark Toda <toda.mark@gmail.com>
Co-authored-by: dianakocsis <diana.kocsis@uniswap.org>

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

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

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

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

snapshots/TestDynamicReturnFees.json

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

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

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") {

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

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

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

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

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