multicall: bubble up revert reason (#236)

* wip bubble up revert

* fix formatting

* simple bubble

* test different error types on multicall

* additional testing for external contract reverts

* example core revert bubbling

* testing for different lengths

* cleanup

* cleanup unused imports

* delete stale gas

* minor nits

.forge-snapshots/PositionManager_burn_empty.snap

@@ -1 +1 @@
-47186
+47184

.forge-snapshots/PositionManager_burn_empty_native.snap

@@ -1 +1 @@
-47004
+47001

.forge-snapshots/PositionManager_burn_nonEmpty_native_withClose.snap

@@ -1 +1 @@
-123058
+123056

.forge-snapshots/PositionManager_burn_nonEmpty_native_withTakePair.snap

@@ -1 +1 @@
-122756
+122754

.forge-snapshots/PositionManager_burn_nonEmpty_withClose.snap

@@ -1 +1 @@
-130136
+130134

.forge-snapshots/PositionManager_burn_nonEmpty_withTakePair.snap

@@ -1 +1 @@
-129835
+129832

.forge-snapshots/PositionManager_collect_native.snap

@@ -1 +1 @@
-141409
+141406

.forge-snapshots/PositionManager_collect_sameRange.snap

@@ -1 +1 @@
-150257
+150254

.forge-snapshots/PositionManager_collect_withClose.snap

@@ -1 +1 @@
-150257
+150254

.forge-snapshots/PositionManager_collect_withTakePair.snap

@@ -1 +1 @@
-149868
+149865

.forge-snapshots/PositionManager_decreaseLiquidity_native.snap

@@ -1 +1 @@
-108602
+108600

.forge-snapshots/PositionManager_decreaseLiquidity_withClose.snap

@@ -1 +1 @@
-115800
+115797

.forge-snapshots/PositionManager_decreaseLiquidity_withTakePair.snap

@@ -1 +1 @@
-115411
+115408

.forge-snapshots/PositionManager_decrease_burnEmpty.snap

@@ -1 +1 @@
-134196
+134193

.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap

@@ -1 +1 @@
-126935
+126932

.forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap

@@ -1 +1 @@
-128516
+128513

.forge-snapshots/PositionManager_increaseLiquidity_erc20_withClose.snap

@@ -1 +1 @@
-152363
+152360

.forge-snapshots/PositionManager_increaseLiquidity_erc20_withSettlePair.snap

@@ -1 +1 @@
-151604
+151601

.forge-snapshots/PositionManager_increaseLiquidity_native.snap

@@ -1 +1 @@
-134163
+134160

.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap

@@ -1 +1 @@
-130328
+130325

.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap

@@ -1 +1 @@
-171022
+171019

.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap

@@ -1 +1 @@
-141002
+140999

.forge-snapshots/PositionManager_mint_native.snap

@@ -1 +1 @@
-336841
+336838

.forge-snapshots/PositionManager_mint_nativeWithSweep_withClose.snap

@@ -1 +1 @@
-345370
+345367

.forge-snapshots/PositionManager_mint_nativeWithSweep_withSettlePair.snap

@@ -1 +1 @@
-344911
+344908

.forge-snapshots/PositionManager_mint_onSameTickLower.snap

@@ -1 +1 @@
-314823
+314820

.forge-snapshots/PositionManager_mint_onSameTickUpper.snap

@@ -1 +1 @@
-315465
+315462

.forge-snapshots/PositionManager_mint_sameRange.snap

@@ -1 +1 @@
-241047
+241044

.forge-snapshots/PositionManager_mint_settleWithBalance_sweep.snap

@@ -1 +1 @@
-371193
+371190

.forge-snapshots/PositionManager_mint_warmedPool_differentRange.snap

@@ -1 +1 @@
-320841
+320838

.forge-snapshots/PositionManager_mint_withClose.snap

@@ -1 +1 @@
-372141
+372138

.forge-snapshots/PositionManager_mint_withSettlePair.snap

@@ -1 +1 @@
-371520
+371517

.forge-snapshots/PositionManager_multicall_initialize_mint.snap

@@ -1 +1 @@
-416516
+416513

src/base/Multicall.sol

@@ -13,18 +13,10 @@ abstract contract Multicall is IMulticall {
             (bool success, bytes memory result) = address(this).delegatecall(data[i]);
 
             if (!success) {
-                // handle custom errors
-                if (result.length == 4) {
-                    assembly {
-                        revert(add(result, 0x20), mload(result))
-                    }
-                }
-                // Next 5 lines from https://ethereum.stackexchange.com/a/83577
-                if (result.length < 68) revert();
+                // bubble up the revert reason
                 assembly {
-                    result := add(result, 0x04)
+                    revert(add(result, 0x20), mload(result))
                 }
-                revert(abi.decode(result, (string)));
             }
 
             results[i] = result;

test/Multicall.t.sol

@@ -2,7 +2,7 @@
 pragma solidity ^0.8.20;
 
 import "forge-std/Test.sol";
-import {MockMulticall} from "./mocks/MockMulticall.sol";
+import {MockMulticall, RevertContract} from "./mocks/MockMulticall.sol";
 
 contract MulticallTest is Test {
     MockMulticall multicall;
@@ -30,7 +30,7 @@ contract MulticallTest is Test {
     function test_multicall_firstRevert() public {
         bytes[] memory calls = new bytes[](2);
         calls[0] =
-            abi.encodeWithSelector(MockMulticall(multicall).functionThatRevertsWithError.selector, "First call failed");
+            abi.encodeWithSelector(MockMulticall(multicall).functionThatRevertsWithString.selector, "First call failed");
         calls[1] = abi.encodeWithSelector(MockMulticall(multicall).functionThatReturnsTuple.selector, 1, 2);
 
         vm.expectRevert("First call failed");
@@ -40,8 +40,9 @@ contract MulticallTest is Test {
     function test_multicall_secondRevert() public {
         bytes[] memory calls = new bytes[](2);
         calls[0] = abi.encodeWithSelector(MockMulticall(multicall).functionThatReturnsTuple.selector, 1, 2);
-        calls[1] =
-            abi.encodeWithSelector(MockMulticall(multicall).functionThatRevertsWithError.selector, "Second call failed");
+        calls[1] = abi.encodeWithSelector(
+            MockMulticall(multicall).functionThatRevertsWithString.selector, "Second call failed"
+        );
 
         vm.expectRevert("Second call failed");
         multicall.multicall(calls);
@@ -106,4 +107,112 @@ contract MulticallTest is Test {
         assertEq(multicall.msgValue(), 100);
         assertEq(multicall.msgValueDouble(), 200);
     }
+
+    // revert bubbling
+    function test_multicall_bubbleRevert_string() public {
+        bytes[] memory calls = new bytes[](1);
+        calls[0] =
+            abi.encodeWithSelector(MockMulticall(multicall).functionThatRevertsWithString.selector, "errorString");
+
+        vm.expectRevert("errorString");
+        multicall.multicall(calls);
+    }
+
+    function test_multicall_bubbleRevert_4bytes() public {
+        bytes[] memory calls = new bytes[](1);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).revertWith4Bytes.selector);
+
+        // revert is caught
+        vm.expectRevert(MockMulticall.Error4Bytes.selector);
+        multicall.multicall(calls);
+
+        // confirm expected length of the revert
+        try multicall.revertWith4Bytes() {}
+        catch (bytes memory reason) {
+            assertEq(reason.length, 4);
+        }
+    }
+
+    function test_fuzz_multicall_bubbleRevert_36bytes(uint8 num) public {
+        bytes[] memory calls = new bytes[](1);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).revertWith36Bytes.selector, num);
+
+        // revert is caught
+        vm.expectRevert(abi.encodeWithSelector(MockMulticall.Error36Bytes.selector, num));
+        multicall.multicall(calls);
+
+        // confirm expected length of the revert
+        try multicall.revertWith36Bytes(num) {}
+        catch (bytes memory reason) {
+            assertEq(reason.length, 36);
+        }
+    }
+
+    function test_fuzz_multicall_bubbleRevert_68bytes(uint256 a, uint256 b) public {
+        bytes[] memory calls = new bytes[](1);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).revertWith68Bytes.selector, a, b);
+
+        // revert is caught
+        vm.expectRevert(abi.encodeWithSelector(MockMulticall.Error68Bytes.selector, a, b));
+        multicall.multicall(calls);
+
+        // confirm expected length of the revert
+        try multicall.revertWith68Bytes(a, b) {}
+        catch (bytes memory reason) {
+            assertEq(reason.length, 68);
+        }
+    }
+
+    function test_fuzz_multicall_bubbleRevert_arbitraryBytes(uint16 length) public {
+        length = uint16(bound(length, 0, 4096));
+        bytes memory data = new bytes(length);
+        for (uint256 i = 0; i < data.length; i++) {
+            data[i] = bytes1(uint8(i));
+        }
+
+        bytes[] memory calls = new bytes[](1);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).revertWithBytes.selector, data);
+
+        // revert is caught
+        vm.expectRevert(abi.encodeWithSelector(MockMulticall.ErrorBytes.selector, data));
+        multicall.multicall(calls);
+
+        // confirm expected length of the revert
+        try multicall.revertWithBytes(data) {}
+        catch (bytes memory reason) {
+            // errors with 0 bytes are by default 64 bytes of data (length & pointer?) + 4 bytes of selector
+            if (length == 0) {
+                assertEq(reason.length, 68);
+            } else {
+                uint256 expectedLength = 64 + 4; // default length + selector
+                // 32 bytes added to the reason for each 32 bytes of data
+                expectedLength += (((data.length - 1) / 32) + 1) * 32;
+                assertEq(reason.length, expectedLength);
+            }
+        }
+    }
+
+    function test_multicall_bubbleRevert_externalRevertString() public {
+        bytes[] memory calls = new bytes[](2);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).externalRevertString.selector, "errorString");
+
+        vm.expectRevert("errorString");
+        multicall.multicall(calls);
+    }
+
+    function test_multicall_bubbleRevert_externalRevertSimple() public {
+        bytes[] memory calls = new bytes[](1);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).externalRevertError1.selector);
+
+        vm.expectRevert(RevertContract.Error1.selector);
+        multicall.multicall(calls);
+    }
+
+    function test_multicall_bubbleRevert_externalRevertWithParams(uint256 a, uint256 b) public {
+        bytes[] memory calls = new bytes[](1);
+        calls[0] = abi.encodeWithSelector(MockMulticall(multicall).externalRevertError2.selector, a, b);
+
+        vm.expectRevert(abi.encodeWithSelector(RevertContract.Error2.selector, a, b));
+        multicall.multicall(calls);
+    }
 }