ABDK CVF 18, 19, 54: bubble-up errors (Uniswap#790)

* bubble-up errors from tokens and hooks

* forge fmt

* Update Currency.t.sol

* add bytes argument to errors and include bubbled up revert data

* Improved comments

---------

Co-authored-by: gretzke <daniel@gretzke.de>

.forge-snapshots/poolManager bytecode size.snap

@@ -1 +1 @@
-23984
+24183

src/libraries/CustomRevert.sol

@@ -73,4 +73,23 @@ library CustomRevert {
             revert(0, 0x44)
         }
     }
+
+    /// @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 {
+        assembly ("memory-safe") {
+            let size := returndatasize()
+            let fmp := mload(0x40)
+
+            // Encode selector, offset, size, data
+            mstore(fmp, selector)
+            mstore(add(fmp, 0x04), 0x20)
+            mstore(add(fmp, 0x24), size)
+            returndatacopy(add(fmp, 0x44), 0, size)
+
+            // Ensure the size is a multiple of 32 bytes
+            let encodedSize := add(0x44, mul(div(add(size, 31), 32), 32))
+            revert(fmp, encodedSize)
+        }
+    }
 }

src/libraries/Hooks.sol

@@ -70,7 +70,8 @@ library Hooks {
     error InvalidHookResponse();
 
     /// @notice thrown when a hook call fails
-    error FailedHookCall();
+    /// @param revertReason bubbled up revert reason
+    error FailedHookCall(bytes revertReason);
 
     /// @notice The hook's delta changed the swap from exactIn to exactOut or vice versa
     error HookDeltaExceedsSwapAmount();
@@ -128,18 +129,15 @@ library Hooks {
     /// @notice performs a hook call using the given calldata on the given hook that doesnt return a delta
     /// @return result The complete data returned by the hook
     function callHook(IHooks self, bytes memory data) internal returns (bytes memory result) {
+        bool success;
+        assembly ("memory-safe") {
+            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();
+
+        // The call was successful, fetch the returned data
         assembly ("memory-safe") {
-            if iszero(call(gas(), self, 0, add(data, 0x20), mload(data), 0, 0)) {
-                if iszero(returndatasize()) {
-                    // if the call failed without a revert reason, revert with `FailedHookCall()`
-                    mstore(0, 0x36bc48c5)
-                    revert(0x1c, 0x04)
-                }
-                // bubble up revert
-                let fmp := mload(0x40)
-                returndatacopy(fmp, 0, returndatasize())
-                revert(fmp, returndatasize())
-            }
             // allocate result byte array from the free memory pointer
             result := mload(0x40)
             // store new free memory pointer at the end of the array padded to 32 bytes

src/test/EmptyRevertContract.sol

@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.24;
+
+contract EmptyRevertContract {
+    // a contract to simulate reverting with no returndata, to test that our error catching works
+    fallback() external {
+        revert();
+    }
+}

src/types/Currency.sol

@@ -31,10 +31,12 @@ library CurrencyLibrary {
     using CustomRevert for bytes4;
 
     /// @notice Thrown when a native transfer fails
-    error NativeTransferFailed();
+    /// @param revertReason bubbled up revert reason
+    error NativeTransferFailed(bytes revertReason);
 
     /// @notice Thrown when an ERC20 transfer fails
-    error ERC20TransferFailed();
+    /// @param revertReason bubbled up revert reason
+    error ERC20TransferFailed(bytes revertReason);
 
     /// @notice A constant to represent the native currency
     Currency public constant NATIVE = Currency.wrap(address(0));
@@ -49,7 +51,8 @@ library CurrencyLibrary {
                 // Transfer the ETH and revert if it fails.
                 success := call(gas(), to, amount, 0, 0, 0, 0)
             }
-            if (!success) NativeTransferFailed.selector.revertWith();
+            // revert with NativeTransferFailed, containing the bubbled up error as an argument
+            if (!success) NativeTransferFailed.selector.bubbleUpAndRevertWith();
         } else {
             assembly ("memory-safe") {
                 // Get a pointer to some free memory.
@@ -77,7 +80,8 @@ library CurrencyLibrary {
                 mstore(add(fmp, 0x20), 0) // 4 bytes of `to` and 28 bytes of `amount` were stored here
                 mstore(add(fmp, 0x40), 0) // 4 bytes of `amount` were stored here
             }
-            if (!success) ERC20TransferFailed.selector.revertWith();
+            // revert with ERC20TransferFailed, containing the bubbled up error as an argument
+            if (!success) ERC20TransferFailed.selector.bubbleUpAndRevertWith();
         }
     }
 

test/DynamicFees.t.sol

@@ -73,7 +73,11 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
         uint24 fee = 1000001;
         dynamicFeesHooks.setFee(fee);
 
-        vm.expectRevert(abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.FailedHookCall.selector, abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee)
+            )
+        );
         manager.initialize(key, SQRT_PRICE_1_1, ZERO_BYTES);
     }
 
@@ -105,7 +109,12 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
         dynamicFeesHooks.setFee(123);
 
         // afterInitialize will try to update the fee, and fail
-        vm.expectRevert(IPoolManager.UnauthorizedDynamicLPFeeUpdate.selector);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.FailedHookCall.selector,
+                abi.encodeWithSelector(IPoolManager.UnauthorizedDynamicLPFeeUpdate.selector)
+            )
+        );
         manager.initialize(key, SQRT_PRICE_1_1, ZERO_BYTES);
     }
 
@@ -118,7 +127,11 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
         PoolSwapTest.TestSettings memory testSettings =
             PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
 
-        vm.expectRevert(abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.FailedHookCall.selector, abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, fee)
+            )
+        );
         swapRouter.swap(key, SWAP_PARAMS, testSettings, ZERO_BYTES);
     }
 

test/PoolManager.t.sol

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

test/libraries/Hooks.t.sol

@@ -21,7 +21,7 @@ import {PoolKey} from "../../src/types/PoolKey.sol";
 import {IERC20Minimal} from "../../src/interfaces/external/IERC20Minimal.sol";
 import {BalanceDelta} from "../../src/types/BalanceDelta.sol";
 import {BaseTestHooks} from "../../src/test/BaseTestHooks.sol";
-import {EmptyRevertHook} from "../../src/test/EmptyRevertHook.sol";
+import {EmptyRevertContract} from "../../src/test/EmptyRevertContract.sol";
 import {StateLibrary} from "../../src/libraries/StateLibrary.sol";
 import {Constants} from "../utils/Constants.sol";
 
@@ -1010,18 +1010,22 @@ contract HooksTest is Test, Deployers, GasSnapshot {
         PoolSwapTest.TestSettings memory testSettings =
             PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
 
-        vm.expectRevert(BaseTestHooks.HookNotImplemented.selector);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.FailedHookCall.selector, abi.encodeWithSelector(BaseTestHooks.HookNotImplemented.selector)
+            )
+        );
         swapRouter.swap(key, swapParams, testSettings, new bytes(0));
     }
 
-    function test_callHook_revertsWithInternalErrorHookCallFailed() public {
+    function test_callHook_revertsWithInternalErrorFailedHookCall() public {
         // This test executes _callHook through beforeSwap.
-        EmptyRevertHook emptyRevertingHookImpl = new EmptyRevertHook();
+        EmptyRevertContract emptyRevertingHookImpl = new EmptyRevertContract();
         address beforeSwapFlag = address(uint160(Hooks.BEFORE_SWAP_FLAG));
         vm.etch(beforeSwapFlag, address(emptyRevertingHookImpl).code);
-        EmptyRevertHook revertingHook = EmptyRevertHook(beforeSwapFlag);
+        EmptyRevertContract revertingHook = EmptyRevertContract(beforeSwapFlag);
 
-        PoolKey memory key = PoolKey(currency0, currency1, 0, 60, IHooks(revertingHook));
+        PoolKey memory key = PoolKey(currency0, currency1, 0, 60, IHooks(address(revertingHook)));
         manager.initialize(key, SQRT_PRICE_1_1, new bytes(0));
 
         IPoolManager.SwapParams memory swapParams =
@@ -1030,7 +1034,7 @@ contract HooksTest is Test, Deployers, GasSnapshot {
         PoolSwapTest.TestSettings memory testSettings =
             PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
 
-        vm.expectRevert(Hooks.FailedHookCall.selector);
+        vm.expectRevert(abi.encodeWithSelector(Hooks.FailedHookCall.selector, new bytes(0)));
         swapRouter.swap(key, swapParams, testSettings, new bytes(0));
     }
 }

test/types/Currency.t.sol

@@ -2,9 +2,11 @@
 pragma solidity ^0.8.20;
 
 import {Test} from "forge-std/Test.sol";
+import {stdError} from "forge-std/StdError.sol";
 import {MockERC20} from "solmate/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";
 
 contract TestCurrency is Test {
     uint256 constant initialERC20Balance = 1000 ether;
@@ -115,6 +117,14 @@ contract TestCurrency is Test {
         assertEq(id & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, currencyTest.toId(currencyTest.fromId(id)));
     }
 
+    function test_transfer_noReturnData() public {
+        // 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)));
+        currencyTest.transfer(Currency.wrap(address(emptyRevertingToken)), otherAddress, 100);
+    }
+
     function test_fuzz_transfer_native(uint256 amount) public {
         uint256 balanceBefore = otherAddress.balance;
         uint256 contractBalanceBefore = address(currencyTest).balance;
@@ -124,7 +134,7 @@ contract TestCurrency is Test {
             assertEq(otherAddress.balance, balanceBefore + amount);
             assertEq(address(currencyTest).balance, contractBalanceBefore - amount);
         } else {
-            vm.expectRevert(CurrencyLibrary.NativeTransferFailed.selector);
+            vm.expectRevert(abi.encodeWithSelector(CurrencyLibrary.NativeTransferFailed.selector, new bytes(0)));
             currencyTest.transfer(nativeCurrency, otherAddress, amount);
             assertEq(otherAddress.balance, balanceBefore);
         }
@@ -140,7 +150,10 @@ contract TestCurrency is Test {
                 MockERC20(Currency.unwrap(erc20Currency)).balanceOf(address(currencyTest)), initialERC20Balance - amount
             );
         } else {
-            vm.expectRevert(CurrencyLibrary.ERC20TransferFailed.selector);
+            // the token reverts with an overflow error message, so this is bubbled up
+            vm.expectRevert(
+                abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector, stdError.arithmeticError)
+            );
             currencyTest.transfer(erc20Currency, otherAddress, amount);
             assertEq(currencyTest.balanceOf(erc20Currency, otherAddress), balanceBefore);
         }