optimization: [internal-s47] adjust bubble up revert error to conform…

… with ERC7751
pancakeswap · Oct 29, 2024 · 82addd8 · 82addd8
1 parent a08c55f
commit 82addd8
Show file tree

Hide file tree

Showing 20 changed files with 122 additions and 58 deletions.
diff --git a/.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap
@@ -1 +1 @@
-178162
+178172
diff --git a/.forge-snapshots/BinHookTest#testDonateSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testDonateSucceedsWithHook.snap
@@ -1 +1 @@
-188425
+188435
diff --git a/.forge-snapshots/BinHookTest#testInitializeSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testInitializeSucceedsWithHook.snap
@@ -1 +1 @@
-184048
+184058
diff --git a/.forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap
@@ -1 +1 @@
-311216
+311226
diff --git a/.forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testSwapSucceedsWithHook.snap
@@ -1 +1 @@
-189420
+189430
diff --git a/.forge-snapshots/BinPoolManagerBytecodeSize.snap b/.forge-snapshots/BinPoolManagerBytecodeSize.snap
@@ -1 +1 @@
-23951
+24175
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap b/.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap
@@ -1 +1 @@
-172921
+172918
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap b/.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap
@@ -1 +1 @@
-178949
+178946
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap
@@ -1 +1 @@
-132952
+132949
diff --git a/.forge-snapshots/CLPoolManagerBytecodeSize.snap b/.forge-snapshots/CLPoolManagerBytecodeSize.snap
@@ -1 +1 @@
-20902
+21103
diff --git a/.forge-snapshots/CLPoolManagerTest#swap_withHooks.snap b/.forge-snapshots/CLPoolManagerTest#swap_withHooks.snap
@@ -1 +1 @@
-87398
+87408
diff --git a/.forge-snapshots/VaultBytecodeSize.snap b/.forge-snapshots/VaultBytecodeSize.snap
@@ -1 +1 @@
-7614
+7792
diff --git a/src/libraries/CustomRevert.sol b/src/libraries/CustomRevert.sol
@@ -5,23 +5,46 @@ pragma solidity ^0.8.0;
 /// @notice Contains functions for reverting with custom errors with different argument types efficiently
 /// @dev The functions may tamper with the free memory pointer but it is fine since the call context is exited immediately
 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 {
+    /// @dev ERC-7751 error for wrapping bubbled up reverts
+    error WrappedError(address target, bytes4 selector, bytes reason, bytes details);
+
+    /// @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
@@ -15,12 +15,9 @@ library Hooks {
     using ParametersHelper for bytes32;
     using LPFeeLibrary for uint24;
     using ParseBytes for bytes;
-    using CustomRevert for bytes4;
 
-    /// @notice thrown when a hook call fails
-    /// @param hook the hook address
-    /// @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 Hook permissions contain conflict
     ///  1. enabled beforeSwapReturnsDelta, but lacking beforeSwap call
@@ -79,7 +76,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
@@ -29,17 +29,12 @@ function greaterThanOrEqualTo(Currency currency, Currency other) pure returns (b
 /// @dev This library allows for transferring and holding native tokens and ERC20 tokens
 library CurrencyLibrary {
     using CurrencyLibrary for Currency;
-    using CustomRevert for bytes4;
 
-    /// @notice Thrown when a native transfer fails
-    /// @param recipient address that the transfer failed to
-    /// @param revertReason bubbled up revert reason
-    error Wrap__NativeTransferFailed(address recipient, bytes revertReason);
+    /// @notice Additional context for ERC-7751 wrapped error when a native transfer fails
+    error NativeTransferFailed();
 
-    /// @notice Thrown when an ERC20 transfer fails
-    /// @param token address of the ERC20 token
-    /// @param revertReason bubbled up revert reason
-    error Wrap__ERC20TransferFailed(address token, bytes revertReason);
+    /// @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 NATIVE = Currency.wrap(address(0));
@@ -55,7 +50,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) 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.
@@ -84,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/libraries/Currency.t.sol b/test/libraries/Currency.t.sol
@@ -2,11 +2,12 @@
 pragma solidity ^0.8.24;
 
 import {Test} from "forge-std/Test.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 {TokenRejecter} from "../helpers/TokenRejecter.sol";
 import {TokenSender} from "../helpers/TokenSender.sol";
 import {stdError} from "forge-std/StdError.sol";
+import {CustomRevert} from "../../src/libraries/CustomRevert.sol";
 
 contract TestCurrency is Test {
     using CurrencyLibrary for uint256;
@@ -110,7 +111,13 @@ contract TestCurrency is Test {
         deal(address(sender), 10 ether);
 
         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)
+            )
         );
         sender.send(nativeCurrency, otherAddress, 10 ether + 1);
     }
@@ -121,7 +128,11 @@ contract TestCurrency is Test {
 
         vm.expectRevert(
             abi.encodeWithSelector(
-                CurrencyLibrary.Wrap__ERC20TransferFailed.selector, erc20Currency, stdError.arithmeticError
+                CustomRevert.WrappedError.selector,
+                erc20Currency,
+                ERC20.transfer.selector,
+                stdError.arithmeticError,
+                abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector)
             )
         );
         sender.send(erc20Currency, otherAddress, 101);

diff --git a/test/pool-bin/BinHookRevertWithReason.t.sol b/test/pool-bin/BinHookRevertWithReason.t.sol
@@ -11,6 +11,8 @@ import {BinPoolParametersHelper} from "../../src/pool-bin/libraries/BinPoolParam
 import {Hooks} from "../../src/libraries/Hooks.sol";
 import {BinRevertHook} from "./helpers/BinRevertHook.sol";
 import {BaseBinTestHook} from "./helpers/BaseBinTestHook.sol";
+import {IBinHooks} from "../../src/pool-bin/interfaces/IBinHooks.sol";
+import {CustomRevert} from "../../src/libraries/CustomRevert.sol";
 
 /// @dev make sure the revert reason is bubbled up
 contract BinHookRevertWithReasonTest is Test {
@@ -52,17 +54,27 @@ contract BinHookRevertWithReasonTest is Test {
     }
 
     function testRevertWithNoReason() public {
-        vm.expectRevert(abi.encodeWithSelector(Hooks.Wrap__FailedHookCall.selector, hook, new bytes(0)));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                CustomRevert.WrappedError.selector,
+                hook,
+                IBinHooks.afterInitialize.selector,
+                new bytes(0),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
+            )
+        );
         poolManager.initialize(key, activeId);
     }
 
     function testRevertWithHookNotImplemented() public {
         hook.setRevertWithHookNotImplemented(true);
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.Wrap__FailedHookCall.selector,
+                CustomRevert.WrappedError.selector,
                 hook,
-                abi.encodeWithSelector(BaseBinTestHook.HookNotImplemented.selector)
+                IBinHooks.afterInitialize.selector,
+                abi.encodeWithSelector(BaseBinTestHook.HookNotImplemented.selector),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
             )
         );
         poolManager.initialize(key, activeId);

diff --git a/test/pool-bin/libraries/BinPoolFee.t.sol b/test/pool-bin/libraries/BinPoolFee.t.sol
@@ -28,6 +28,8 @@ import {BinFeeManagerHook} from "../helpers/BinFeeManagerHook.sol";
 import {HOOKS_AFTER_INITIALIZE_OFFSET, HOOKS_BEFORE_MINT_OFFSET} from "../../../src/pool-bin/interfaces/IBinHooks.sol";
 import {Hooks} from "../../../src/libraries/Hooks.sol";
 import {SortTokens} from "../../helpers/SortTokens.sol";
+import {CustomRevert} from "../../../src/libraries/CustomRevert.sol";
+import {IBinHooks} from "../../../src/pool-bin/interfaces/IBinHooks.sol";
 
 /**
  * @dev tests around fee for mint(), swap() and burn()
@@ -172,9 +174,11 @@ contract BinPoolFeeTest is BinTestHelper {
         bytes memory data = abi.encode(true, uint24(swapFee));
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.Wrap__FailedHookCall.selector,
+                CustomRevert.WrappedError.selector,
                 binFeeManagerHook,
-                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, uint24(swapFee))
+                IBinHooks.beforeMint.selector,
+                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, uint24(swapFee)),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
             )
         );
         addLiquidityToBin(key, poolManager, bob, activeId, 10_000 ether, 10_000 ether, 1e18, 1e18, data);
@@ -416,9 +420,11 @@ contract BinPoolFeeTest is BinTestHelper {
         bytes memory data = abi.encode(true, uint24(swapFee));
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.Wrap__FailedHookCall.selector,
+                CustomRevert.WrappedError.selector,
                 binFeeManagerHook,
-                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, uint24(swapFee))
+                IBinHooks.beforeSwap.selector,
+                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, uint24(swapFee)),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
             )
         );
         poolManager.swap(key, true, 1e18, data);

diff --git a/test/pool-cl/CLHookRevertWithReason.t.sol b/test/pool-cl/CLHookRevertWithReason.t.sol
@@ -19,6 +19,8 @@ import {TokenFixture} from "../helpers/TokenFixture.sol";
 import {CLRevertHook} from "./helpers/CLRevertHook.sol";
 import {CLPoolParametersHelper} from "../../src/pool-cl/libraries/CLPoolParametersHelper.sol";
 import {BaseCLTestHook} from "./helpers/BaseCLTestHook.sol";
+import {CustomRevert} from "../../src/libraries/CustomRevert.sol";
+import {ICLHooks} from "../../src/pool-cl/interfaces/ICLHooks.sol";
 
 /// @dev make sure the revert reason is bubbled up
 contract CLHookRevertWithReasonTest is Test, Deployers, TokenFixture {
@@ -45,17 +47,27 @@ contract CLHookRevertWithReasonTest is Test, Deployers, TokenFixture {
     }
 
     function testRevertWithNoReason() public {
-        vm.expectRevert(abi.encodeWithSelector(Hooks.Wrap__FailedHookCall.selector, hook, new bytes(0)));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                CustomRevert.WrappedError.selector,
+                hook,
+                ICLHooks.afterInitialize.selector,
+                new bytes(0),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
+            )
+        );
         poolManager.initialize(key, SQRT_RATIO_1_1);
     }
 
     function testRevertWithHookNotImplemented() public {
         hook.setRevertWithHookNotImplemented(true);
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.Wrap__FailedHookCall.selector,
+                CustomRevert.WrappedError.selector,
                 hook,
-                abi.encodeWithSelector(BaseCLTestHook.HookNotImplemented.selector)
+                ICLHooks.afterInitialize.selector,
+                abi.encodeWithSelector(BaseCLTestHook.HookNotImplemented.selector),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
             )
         );
         poolManager.initialize(key, SQRT_RATIO_1_1);

diff --git a/test/pool-cl/CLPoolManager.t.sol b/test/pool-cl/CLPoolManager.t.sol
@@ -39,6 +39,8 @@ import {NoIsolate} from "../helpers/NoIsolate.sol";
 import {Pausable} from "@openzeppelin/contracts/utils/Pausable.sol";
 import {CLPoolGetter} from "./helpers/CLPoolGetter.sol";
 import {CLSlot0} from "../../src/pool-cl/types/CLSlot0.sol";
+import {CustomRevert} from "../../src/libraries/CustomRevert.sol";
+import {ICLHooks} from "../../src/pool-cl/interfaces/ICLHooks.sol";
 
 contract CLPoolManagerTest is Test, NoIsolate, Deployers, TokenFixture, GasSnapshot {
     using CLPoolParametersHelper for bytes32;
@@ -731,9 +733,11 @@ contract CLPoolManagerTest is Test, NoIsolate, Deployers, TokenFixture, GasSnaps
         clFeeManagerHook.setFee(dynamicSwapFee);
         vm.expectRevert(
             abi.encodeWithSelector(
-                Hooks.Wrap__FailedHookCall.selector,
+                CustomRevert.WrappedError.selector,
                 clFeeManagerHook,
-                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, dynamicSwapFee)
+                ICLHooks.afterInitialize.selector,
+                abi.encodeWithSelector(LPFeeLibrary.LPFeeTooLarge.selector, dynamicSwapFee),
+                abi.encodeWithSelector(Hooks.HookCallFailed.selector)
             )
         );
         poolManager.initialize(key, SQRT_RATIO_1_1);