add parameter to bubbled revert wrapper (Uniswap#817)

* add parameter to bubbled revert wrapper * fix * change arg name
trailofbits · Aug 5, 2024 · d3be002 · d3be002
1 parent 825132a
commit d3be002
Show file tree

Hide file tree

Showing 31 changed files with 74 additions and 48 deletions.
diff --git a/.forge-snapshots/addLiquidity CA fee.snap b/.forge-snapshots/addLiquidity CA fee.snap
@@ -1 +1 @@
-170604
+170646
diff --git a/.forge-snapshots/addLiquidity with empty hook.snap b/.forge-snapshots/addLiquidity with empty hook.snap
@@ -1 +1 @@
-273799
+273803
diff --git a/.forge-snapshots/erc20 collect protocol fees.snap b/.forge-snapshots/erc20 collect protocol fees.snap
@@ -1 +1 @@
-57303
+57323
diff --git a/.forge-snapshots/native collect protocol fees.snap b/.forge-snapshots/native collect protocol fees.snap
@@ -1 +1 @@
-59588
+59607
diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap
@@ -1 +1 @@
-24339
+24386
diff --git a/.forge-snapshots/removeLiquidity CA fee.snap b/.forge-snapshots/removeLiquidity CA fee.snap
@@ -1 +1 @@
-140863
+140945
diff --git a/.forge-snapshots/removeLiquidity with empty hook.snap b/.forge-snapshots/removeLiquidity with empty hook.snap
@@ -1 +1 @@
-130377
+130421
diff --git a/.forge-snapshots/removeLiquidity with native token.snap b/.forge-snapshots/removeLiquidity with native token.snap
@@ -1 +1 @@
-112276
+112308
diff --git a/.forge-snapshots/simple removeLiquidity some liquidity remains.snap b/.forge-snapshots/simple removeLiquidity some liquidity remains.snap
@@ -1 +1 @@
-92708
+92728
diff --git a/.forge-snapshots/simple removeLiquidity.snap b/.forge-snapshots/simple removeLiquidity.snap
@@ -1 +1 @@
-84809
+84829
diff --git a/.forge-snapshots/simple swap with native.snap b/.forge-snapshots/simple swap with native.snap
@@ -1 +1 @@
-109335
+109355
diff --git a/.forge-snapshots/simple swap.snap b/.forge-snapshots/simple swap.snap
@@ -1 +1 @@
-124181
+124201
diff --git a/.forge-snapshots/swap CA custom curve + swap noop.snap b/.forge-snapshots/swap CA custom curve + swap noop.snap
@@ -1 +1 @@
-126890
+126932
diff --git a/.forge-snapshots/swap CA fee on unspecified.snap b/.forge-snapshots/swap CA fee on unspecified.snap
@@ -1 +1 @@
-155604
+155646
diff --git a/.forge-snapshots/swap against liquidity with native token.snap b/.forge-snapshots/swap against liquidity with native token.snap
@@ -1 +1 @@
-106228
+106248
diff --git a/.forge-snapshots/swap against liquidity.snap b/.forge-snapshots/swap against liquidity.snap
@@ -1 +1 @@
-117322
+117342
diff --git a/.forge-snapshots/swap burn 6909 for input.snap b/.forge-snapshots/swap burn 6909 for input.snap
@@ -1 +1 @@
-129436
+129456
diff --git a/.forge-snapshots/swap burn native 6909 for input.snap b/.forge-snapshots/swap burn native 6909 for input.snap
@@ -1 +1 @@
-118898
+118918
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
@@ -1 +1 @@
-207901
+207943
diff --git a/.forge-snapshots/swap with dynamic fee.snap b/.forge-snapshots/swap with dynamic fee.snap
@@ -1 +1 @@
-140214
+140234
diff --git a/.forge-snapshots/swap with hooks.snap b/.forge-snapshots/swap with hooks.snap
@@ -1 +1 @@
-132956
+132980
diff --git a/.forge-snapshots/swap with lp fee and protocol fee.snap b/.forge-snapshots/swap with lp fee and protocol fee.snap
@@ -1 +1 @@
-170039
+170061
diff --git a/.forge-snapshots/swap with return dynamic fee.snap b/.forge-snapshots/swap with return dynamic fee.snap
@@ -1 +1 @@
-146539
+146561
diff --git a/.forge-snapshots/update dynamic fee in before swap.snap b/.forge-snapshots/update dynamic fee in before swap.snap
@@ -1 +1 @@
-148819
+148841
diff --git a/src/libraries/CustomRevert.sol b/src/libraries/CustomRevert.sol
@@ -75,20 +75,21 @@ 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(bytes revertReason)`
-    function bubbleUpAndRevertWith(bytes4 selector) internal pure {
+    /// @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 {
         assembly ("memory-safe") {
             let size := returndatasize()
             let fmp := mload(0x40)
 
-            // Encode selector, offset, size, data
+            // Encode selector, address, offset, size, data
             mstore(fmp, selector)
-            mstore(add(fmp, 0x04), 0x20)
-            mstore(add(fmp, 0x24), size)
-            returndatacopy(add(fmp, 0x44), 0, size)
+            mstore(add(fmp, 0x04), addr)
+            mstore(add(fmp, 0x24), 0x40)
+            mstore(add(fmp, 0x44), size)
+            returndatacopy(add(fmp, 0x64), 0, size)
 
             // Ensure the size is a multiple of 32 bytes
-            let encodedSize := add(0x44, mul(div(add(size, 31), 32), 32))
+            let encodedSize := add(0x64, mul(div(add(size, 31), 32), 32))
             revert(fmp, encodedSize)
         }
     }

diff --git a/src/libraries/Hooks.sol b/src/libraries/Hooks.sol
@@ -71,7 +71,7 @@ library Hooks {
 
     /// @notice thrown when a hook call fails
     /// @param revertReason bubbled up revert reason
-    error FailedHookCall(bytes revertReason);
+    error Wrap__FailedHookCall(address hook, bytes revertReason);
 
     /// @notice The hook's delta changed the swap from exactIn to exactOut or vice versa
     error HookDeltaExceedsSwapAmount();
@@ -134,7 +134,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) FailedHookCall.selector.bubbleUpAndRevertWith();
+        if (!success) Wrap__FailedHookCall.selector.bubbleUpAndRevertWith(address(self));
 
         // The call was successful, fetch the returned data
         assembly ("memory-safe") {

diff --git a/src/types/Currency.sol b/src/types/Currency.sol
@@ -31,12 +31,12 @@ library CurrencyLibrary {
     using CustomRevert for bytes4;
 
     /// @notice Thrown when a native transfer fails
-    /// @param revertReason bubbled up revert reason
-    error NativeTransferFailed(bytes revertReason);
+    /// @param reason bubbled up revert reason
+    error Wrap__NativeTransferFailed(address recipient, bytes reason);
 
     /// @notice Thrown when an ERC20 transfer fails
-    /// @param revertReason bubbled up revert reason
-    error ERC20TransferFailed(bytes revertReason);
+    /// @param reason bubbled up revert reason
+    error Wrap__ERC20TransferFailed(address token, bytes reason);
 
     /// @notice A constant to represent the native currency
     Currency public constant NATIVE = Currency.wrap(address(0));
@@ -52,7 +52,7 @@ library CurrencyLibrary {
                 success := call(gas(), to, amount, 0, 0, 0, 0)
             }
             // revert with NativeTransferFailed, containing the bubbled up error as an argument
-            if (!success) NativeTransferFailed.selector.bubbleUpAndRevertWith();
+            if (!success) Wrap__NativeTransferFailed.selector.bubbleUpAndRevertWith(to);
         } else {
             assembly ("memory-safe") {
                 // Get a pointer to some free memory.
@@ -81,7 +81,7 @@ 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) ERC20TransferFailed.selector.bubbleUpAndRevertWith();
+            if (!success) Wrap__ERC20TransferFailed.selector.bubbleUpAndRevertWith(Currency.unwrap(currency));
         }
     }
 

diff --git a/test/DynamicFees.t.sol b/test/DynamicFees.t.sol
@@ -75,7 +75,9 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
 
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.FailedHookCall.selector, abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee)
+                Hooks.Wrap__FailedHookCall.selector,
+                address(dynamicFeesHooks),
+                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee)
             )
         );
         manager.initialize(key, SQRT_PRICE_1_1, ZERO_BYTES);
@@ -111,7 +113,8 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
         // afterInitialize will try to update the fee, and fail
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.FailedHookCall.selector,
+                Hooks.Wrap__FailedHookCall.selector,
+                address(dynamicFeesHooks),
                 abi.encodeWithSelector(IPoolManager.UnauthorizedDynamicLPFeeUpdate.selector)
             )
         );
@@ -129,7 +132,9 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
 
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.FailedHookCall.selector, abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee)
+                Hooks.Wrap__FailedHookCall.selector,
+                address(dynamicFeesHooks),
+                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee)
             )
         );
         swapRouter.swap(key, SWAP_PARAMS, testSettings, ZERO_BYTES);

diff --git a/test/PoolManager.t.sol b/test/PoolManager.t.sol
@@ -890,7 +890,11 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
         );
 
         (uint256 amount0, uint256 amount1) = currency0Invalid ? (1, 0) : (0, 1);
-        vm.expectRevert(abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector, abi.encode(bytes32(0))));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                CurrencyLibrary.Wrap__ERC20TransferFailed.selector, invalidToken, abi.encode(bytes32(0))
+            )
+        );
         takeRouter.take(key, amount0, amount1);
 
         // should not revert when non zero amount passed in for valid currency

diff --git a/test/libraries/Hooks.t.sol b/test/libraries/Hooks.t.sol
@@ -1012,7 +1012,9 @@ contract HooksTest is Test, Deployers, GasSnapshot {
 
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.FailedHookCall.selector, abi.encodeWithSelector(BaseTestHooks.HookNotImplemented.selector)
+                Hooks.Wrap__FailedHookCall.selector,
+                address(revertingHook),
+                abi.encodeWithSelector(BaseTestHooks.HookNotImplemented.selector)
             )
         );
         swapRouter.swap(key, swapParams, testSettings, new bytes(0));
@@ -1034,7 +1036,9 @@ contract HooksTest is Test, Deployers, GasSnapshot {
         PoolSwapTest.TestSettings memory testSettings =
             PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
 
-        vm.expectRevert(abi.encodeWithSelector(Hooks.FailedHookCall.selector, new bytes(0)));
+        vm.expectRevert(
+            abi.encodeWithSelector(Hooks.Wrap__FailedHookCall.selector, address(revertingHook), new bytes(0))
+        );
         swapRouter.swap(key, swapParams, testSettings, new bytes(0));
     }
 }
diff --git a/test/types/Currency.t.sol b/test/types/Currency.t.sol
@@ -121,7 +121,13 @@ contract TestCurrency is Test {
         // This contract reverts with no data
         EmptyRevertContract emptyRevertingToken = new EmptyRevertContract();
         // the token reverts with no data, so our custom error will be emitted instead
-        vm.expectRevert(abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector, new bytes(0)));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                CurrencyLibrary.Wrap__ERC20TransferFailed.selector,
+                Currency.unwrap(Currency.wrap(address(emptyRevertingToken))),
+                new bytes(0)
+            )
+        );
         currencyTest.transfer(Currency.wrap(address(emptyRevertingToken)), otherAddress, 100);
     }
 
@@ -134,7 +140,9 @@ contract TestCurrency is Test {
             assertEq(otherAddress.balance, balanceBefore + amount);
             assertEq(address(currencyTest).balance, contractBalanceBefore - amount);
         } else {
-            vm.expectRevert(abi.encodeWithSelector(CurrencyLibrary.NativeTransferFailed.selector, new bytes(0)));
+            vm.expectRevert(
+                abi.encodeWithSelector(CurrencyLibrary.Wrap__NativeTransferFailed.selector, otherAddress, new bytes(0))
+            );
             currencyTest.transfer(nativeCurrency, otherAddress, amount);
             assertEq(otherAddress.balance, balanceBefore);
         }
@@ -152,7 +160,11 @@ contract TestCurrency is Test {
         } else {
             // the token reverts with an overflow error message, so this is bubbled up
             vm.expectRevert(
-                abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector, stdError.arithmeticError)
+                abi.encodeWithSelector(
+                    CurrencyLibrary.Wrap__ERC20TransferFailed.selector,
+                    Currency.unwrap(erc20Currency),
+                    stdError.arithmeticError
+                )
             );
             currencyTest.transfer(erc20Currency, otherAddress, amount);
             assertEq(currencyTest.balanceOf(erc20Currency, otherAddress), balanceBefore);