Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ERC-7751 revert bubbling #889

Merged
merged 13 commits into from
Oct 29, 2024
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity CA fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
170928
170933
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
274240
274250
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
23659
24040
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity CA fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
141194
141199
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
130603
130613
2 changes: 1 addition & 1 deletion .forge-snapshots/swap CA custom curve + swap noop.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
124635
124640
2 changes: 1 addition & 1 deletion .forge-snapshots/swap CA fee on unspecified.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
154686
154691
Original file line number Diff line number Diff line change
@@ -1 +1 @@
206263
206268
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132274
132284
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with return dynamic fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
145589
145594
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
147857
147862
49 changes: 36 additions & 13 deletions src/libraries/CustomRevert.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down Expand Up @@ -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
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(size, 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), size)
// revert reason
returndatacopy(add(fmp, 0xa4), 0, size)
gretzke marked this conversation as resolved.
Show resolved Hide resolved
// 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))
}
}
}
7 changes: 3 additions & 4 deletions src/libraries/Hooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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") {
Expand Down
20 changes: 12 additions & 8 deletions src/types/Currency.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,11 @@ function greaterThanOrEqualTo(Currency currency, Currency other) pure returns (b
library CurrencyLibrary {
using CustomRevert for bytes4;
gretzke marked this conversation as resolved.
Show resolved Hide resolved

/// @notice Thrown when a native transfer fails
/// @param reason bubbled up revert reason
error Wrap__NativeTransferFailed(address recipient, bytes reason);
/// @notice Additional context for ERC-7751 wrapped error when a native transfer fails
error NativeTransferFailed();

/// @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));
Expand All @@ -52,7 +50,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.
Expand Down Expand Up @@ -81,7 +81,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
);
}
}
}

Expand Down
20 changes: 14 additions & 6 deletions test/DynamicFees.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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, GasSnapshot {
Expand Down Expand Up @@ -71,9 +72,11 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {

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);
Expand Down Expand Up @@ -109,9 +112,11 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {
// 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);
Expand All @@ -128,11 +133,14 @@ contract TestDynamicFees is Test, Deployers, GasSnapshot {

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

Expand Down
7 changes: 6 additions & 1 deletion test/PoolManager.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {ProtocolFeeLibrary} from "../src/libraries/ProtocolFeeLibrary.sol";
import {IProtocolFees} from "../src/interfaces/IProtocolFees.sol";
import {StateLibrary} from "../src/libraries/StateLibrary.sol";
import {Actions} from "../src/test/ActionsRouter.sol";
import {CustomRevert} from "../src/libraries/CustomRevert.sol";

contract PoolManagerTest is Test, Deployers, GasSnapshot {
using Hooks for IHooks;
Expand Down Expand Up @@ -929,7 +930,11 @@ contract PoolManagerTest is Test, Deployers, GasSnapshot {
(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);
Expand Down
15 changes: 12 additions & 3 deletions test/libraries/Hooks.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,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, GasSnapshot {
using Hooks for IHooks;
Expand Down Expand Up @@ -1011,9 +1012,11 @@ contract HooksTest is Test, Deployers, GasSnapshot {

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));
Expand All @@ -1036,7 +1039,13 @@ contract HooksTest is Test, Deployers, GasSnapshot {
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));
}
Expand Down
23 changes: 17 additions & 6 deletions test/types/Currency.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {MockERC20} 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;
Expand Down Expand Up @@ -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),
bytes4(0xa9059cbb),
gretzke marked this conversation as resolved.
Show resolved Hide resolved
"",
abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector)
)
);
currencyTest.transfer(Currency.wrap(address(emptyRevertingToken)), otherAddress, 100);
Expand All @@ -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);
Expand All @@ -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
bytes4(0xa9059cbb),
gretzke marked this conversation as resolved.
Show resolved Hide resolved
stdError.arithmeticError,
abi.encodeWithSelector(CurrencyLibrary.ERC20TransferFailed.selector)
)
);
currencyTest.transfer(erc20Currency, otherAddress, amount);
Expand Down
Loading