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.

Sign up for GitHub

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

Jump to bottom

feat: remove ActionData to allow arbitrary calls and test cleanups #9

Merged
merged 6 commits into from
Dec 5, 2023
Merged

feat: remove ActionData to allow arbitrary calls and test cleanups #9

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8f4a29d
Refactor tests and add arbitrary call
zhongeric Nov 30, 2023
7fa5bce
forge fmt
zhongeric Nov 30, 2023
23c460d
add comment
zhongeric Nov 30, 2023
a1ff78d
add comment
zhongeric Nov 30, 2023
acf1d76
remove outdated approvals
zhongeric Nov 30, 2023
8b9f411
Add basic allowance checks
zhongeric Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/RelayOrderReactorIntegrationTest-testExecute.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
303406
288585
2 changes: 1 addition & 1 deletion .forge-snapshots/RelayOrderReactorIntegrationTest-testExecuteWithNativeAsOutput.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
309001
291179
2 changes: 1 addition & 1 deletion .forge-snapshots/RelayOrderReactorIntegrationTest-testPermitAndExecute.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
350945
340631
12 changes: 3 additions & 9 deletions src/lib/RelayOrderLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,6 @@ import {OrderInfo, OutputToken} from "UniswapX/src/base/ReactorStructs.sol";
import {OrderInfoLib} from "UniswapX/src/lib/OrderInfoLib.sol";
import {InputTokenWithRecipient} from "../base/ReactorStructs.sol";

enum ActionType {
ApprovePermit2,
Permit2612,
UniversalRouter
}

/// @dev External struct used to specify simple relay orders
struct RelayOrder {
// generic order information
Expand Down Expand Up @@ -104,12 +98,12 @@ library RelayOrderLib {
function hash(RelayOrder memory order) internal pure returns (bytes32) {
return keccak256(
abi.encode(
ORDER_TYPE_HASH,
order.info.hash(),
ORDER_TYPE_HASH,
order.info.hash(),
order.decayStartTime,
order.decayEndTime,
order.actions,
hash(order.inputs),
hash(order.inputs),
hash(order.outputs)
)
);
Expand Down
31 changes: 8 additions & 23 deletions src/reactors/RelayOrderReactor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import {ReactorErrors} from "../base/ReactorErrors.sol";
import {InputTokenWithRecipient, ResolvedRelayOrder} from "../base/ReactorStructs.sol";
import {CurrencyLibrary, NATIVE} from "../lib/CurrencyLibrary.sol";
import {Permit2Lib} from "../lib/Permit2Lib.sol";
import {RelayOrderLib, RelayOrder, ActionType} from "../lib/RelayOrderLib.sol";
import {RelayOrderLib, RelayOrder} from "../lib/RelayOrderLib.sol";
import {ResolvedRelayOrderLib} from "../lib/ResolvedRelayOrderLib.sol";
import {RelayDecayLib} from "../lib/RelayDecayLib.sol";

/// @notice Reactor for relaying calls to UniversalRouter onchain
/// @dev This reactor only supports V2/V3 swaps, do NOT attempt to use other Universal Router commands
/// @notice Reactor for handling the execution of RelayOrders
/// @notice This contract MUST NOT have approvals or priviledged access
contract RelayOrderReactor is ReactorEvents, ReactorErrors, ReentrancyGuard, IRelayOrderReactor {
using SafeTransferLib for ERC20;
using CurrencyLibrary for address;
Expand Down Expand Up @@ -62,30 +62,15 @@ contract RelayOrderReactor is ReactorEvents, ReactorErrors, ReentrancyGuard, IRe

function _execute(ResolvedRelayOrder[] memory orders) internal {
uint256 ordersLength = orders.length;
// actions are encoded as (ActionType actionType, bytes actionData)[]
// actions are encoded as (address target, uint256 value, bytes data)[]
for (uint256 i = 0; i < ordersLength;) {
ResolvedRelayOrder memory order = orders[i];
uint256 actionsLength = order.actions.length;
for (uint256 j = 0; j < actionsLength;) {
(ActionType actionType, bytes memory actionData) = abi.decode(order.actions[j], (ActionType, bytes));
if (actionType == ActionType.UniversalRouter) {
/// @dev to use universal router integration, this contract must be recipient of all output tokens
(bool success,) = universalRouter.call(actionData);
if (!success) revert CallFailed();
}
// Max approve an ERC20 to UniversalRouter from the reactor using Permit2
else if (actionType == ActionType.ApprovePermit2) {
(address token) = abi.decode(actionData, (address));
if (token == address(0)) revert InvalidToken();
if (ERC20(token).allowance(address(this), address(permit2)) == 0) {
ERC20(token).approve(address(permit2), type(uint256).max);
}
permit2.approve(token, universalRouter, type(uint160).max, type(uint48).max);
}
// Catch unsupported action types
else {
revert UnsupportedAction();
}
(address target, uint256 value, bytes memory data) =
abi.decode(order.actions[j], (address, uint256, bytes));
(bool success,) = target.call{value: value}(data);
if (!success) revert CallFailed();
unchecked {
j++;
}
Expand Down
114 changes: 78 additions & 36 deletions test/foundry-tests/integration/RelayOrderReactorIntegration.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {InputTokenWithRecipient, ResolvedRelayOrder} from "../../../src/base/Rea
import {ReactorEvents} from "../../../src/base/ReactorEvents.sol";
import {CurrencyLibrary} from "../../../src/lib/CurrencyLibrary.sol";
import {PermitSignature} from "../util/PermitSignature.sol";
import {RelayOrderLib, RelayOrder, ActionType} from "../../../src/lib/RelayOrderLib.sol";
import {RelayOrderLib, RelayOrder} from "../../../src/lib/RelayOrderLib.sol";
import {RelayOrderReactor} from "../../../src/reactors/RelayOrderReactor.sol";
import {PermitExecutor} from "../../../src/sample-executors/PermitExecutor.sol";
import {MethodParameters, Interop} from "../util/Interop.sol";
Expand Down Expand Up @@ -49,6 +49,12 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
error InvalidNonce();
error InvalidSigner();

uint256 swapperInputBalanceStart;
uint256 swapperOutputBalanceStart;
uint256 routerInputBalanceStart;
uint256 routerOutputBalanceStart;
uint256 fillerGasInputBalanceStart;

function setUp() public {
swapperPrivateKey = 0xbabe;
swapper = vm.addr(swapperPrivateKey);
Expand All @@ -63,30 +69,23 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
reactor = RelayOrderReactor(RELAY_ORDER_REACTOR);
permitExecutor = new PermitExecutor(address(filler), reactor, address(filler));

// Swapper max approves permit post
// Swapper max approves permit post for all input tokens
vm.startPrank(swapper);
DAI.approve(address(PERMIT2), type(uint256).max);
USDC.approve(address(PERMIT2), type(uint256).max);
PERMIT2.approve(address(DAI), address(reactor), type(uint160).max, type(uint48).max);
PERMIT2.approve(address(USDC), address(reactor), type(uint160).max, type(uint48).max);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the users are approving this router on permit2, can't they then hack each other?

  • Alice does PERMIT2.approve(dai, reactor, maxAmount, maxTimestamp)
  • Bob calls reactor with an action: target=permit2, call="transfer alice's tokens to bob"
  • Permit2 has approval to let the reactor access Alice's tokens, so it accepts the call and gives alice's tokens to bob?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to add a test to check if this is possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha totally, I think we don't need to approves on Permit2 because we do signature transfer so I should remove these and see if the tests pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya we don't need these approves. I think in this design, you definitely cannot be approving this reactor

vm.stopPrank();

// reactor max approves permit post
vm.startPrank(address(reactor));
DAI.approve(address(PERMIT2), type(uint256).max);
USDC.approve(address(PERMIT2), type(uint256).max);
PERMIT2.approve(address(DAI), UNIVERSAL_ROUTER, type(uint160).max, type(uint48).max);
PERMIT2.approve(address(USDC), UNIVERSAL_ROUTER, type(uint160).max, type(uint48).max);
vm.stopPrank();

// Transfer 1000 DAI to swapper
// Fund swappers
vm.startPrank(WHALE);
DAI.transfer(swapper, 1000 * ONE);
DAI.transfer(swapper2, 1000 * ONE);
USDC.transfer(swapper, 1000 * USDC_ONE);
USDC.transfer(swapper2, 1000 * USDC_ONE);
vm.stopPrank();

// initial assumptions
assertEq(USDC.balanceOf(address(reactor)), 0, "reactor should have no USDC");
assertEq(DAI.balanceOf(address(reactor)), 0, "reactor should have no DAI");
}
Expand All @@ -109,7 +108,7 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS

bytes[] memory actions = new bytes[](1);
MethodParameters memory methodParameters = readFixture(json, "._UNISWAP_V3_DAI_USDC");
actions[0] = abi.encode(ActionType.UniversalRouter, methodParameters.data);
actions[0] = abi.encode(UNIVERSAL_ROUTER, methodParameters.value, methodParameters.data);

RelayOrder memory order = RelayOrder({
info: OrderInfoBuilder.init(address(reactor)).withSwapper(swapper).withDeadline(block.timestamp + 100),
Expand All @@ -123,17 +122,25 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
SignedOrder memory signedOrder =
SignedOrder(abi.encode(order), signOrder(swapperPrivateKey, address(PERMIT2), order));

uint256 routerDaiBalanceBefore = DAI.balanceOf(UNIVERSAL_ROUTER);
ERC20 tokenIn = DAI;
ERC20 tokenOut = USDC;
_checkpointBalances(swapper, filler, tokenIn, tokenOut, USDC);

vm.prank(filler);
snapStart("RelayOrderReactorIntegrationTest-testExecute");
reactor.execute{value: methodParameters.value}(signedOrder);
snapEnd();

assertEq(DAI.balanceOf(UNIVERSAL_ROUTER), routerDaiBalanceBefore, "No leftover input in router");
assertEq(USDC.balanceOf(address(reactor)), 0, "No leftover output in reactor");
assertGe(USDC.balanceOf(swapper), amountOutMin, "Swapper did not receive enough output");
assertEq(USDC.balanceOf((filler)), 10 * USDC_ONE, "filler did not receive enough USDC");
assertEq(tokenIn.balanceOf(UNIVERSAL_ROUTER), routerInputBalanceStart, "No leftover input in router");
assertEq(tokenOut.balanceOf(UNIVERSAL_ROUTER), routerOutputBalanceStart, "No leftover output in reactor");
assertEq(tokenOut.balanceOf(address(reactor)), 0, "No leftover output in reactor");
assertEq(tokenIn.balanceOf(swapper), swapperInputBalanceStart - 100 * ONE, "Swapper input tokens");
assertGe(
tokenOut.balanceOf(swapper),
swapperOutputBalanceStart + amountOutMin - 10 * USDC_ONE,
"Swapper did not receive enough output"
);
assertEq(tokenOut.balanceOf((filler)), fillerGasInputBalanceStart + 10 * USDC_ONE, "filler balance");
}

function testPermitAndExecute() public {
Expand All @@ -157,8 +164,6 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
uint256 amountOutMin = 95 * ONE;

// sign permit for USDC
uint256 amount = type(uint256).max - 1; // infinite approval to permit2
uint256 deadline = type(uint256).max - 1; // never expires
bytes32 digest = keccak256(
abi.encodePacked(
"\x19\x01",
Expand All @@ -168,9 +173,9 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
swapper2,
address(PERMIT2),
amount,
type(uint256).max - 1, // infinite approval
USDC.nonces(swapper2),
deadline
type(uint256).max - 1 // infinite deadline
)
)
)
Expand All @@ -180,12 +185,13 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
address signer = ecrecover(digest, v, r, s);
assertEq(signer, swapper2);

bytes memory permitData =
abi.encode(address(USDC), abi.encode(swapper2, address(PERMIT2), amount, deadline, v, r, s));
bytes memory permitData = abi.encode(
address(USDC), abi.encode(swapper2, address(PERMIT2), type(uint256).max - 1, type(uint256).max - 1, v, r, s)
);

bytes[] memory actions = new bytes[](1);
MethodParameters memory methodParameters = readFixture(json, "._UNISWAP_V3_USDC_DAI");
actions[0] = abi.encode(ActionType.UniversalRouter, methodParameters.data);
actions[0] = abi.encode(UNIVERSAL_ROUTER, methodParameters.value, methodParameters.data);

RelayOrder memory order = RelayOrder({
info: OrderInfoBuilder.init(address(reactor)).withSwapper(swapper2).withDeadline(block.timestamp + 100),
Expand All @@ -199,18 +205,33 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
SignedOrder memory signedOrder =
SignedOrder(abi.encode(order), signOrder(swapper2PrivateKey, address(PERMIT2), order));

uint256 routerUSDCBalanceBefore = USDC.balanceOf(UNIVERSAL_ROUTER);
ERC20 tokenIn = USDC;
ERC20 tokenOut = DAI;
// in this case, gas payment will go to executor (msg.sender)
_checkpointBalances(swapper2, address(permitExecutor), tokenIn, tokenOut, USDC);

vm.prank(filler);
snapStart("RelayOrderReactorIntegrationTest-testPermitAndExecute");
permitExecutor.executeWithPermit{value: methodParameters.value}(signedOrder, permitData);
snapEnd();

assertEq(USDC.balanceOf(UNIVERSAL_ROUTER), routerUSDCBalanceBefore, "No leftover input in router");
assertEq(DAI.balanceOf(address(reactor)), 0, "No leftover output in reactor");
assertGe(DAI.balanceOf(swapper2), amountOutMin, "Swapper did not receive enough output");
// in this case, gas payment will go to sender (executor)
assertEq(USDC.balanceOf(address(permitExecutor)), 10 * USDC_ONE, "executor did not receive enough USDC");
assertEq(tokenIn.balanceOf(UNIVERSAL_ROUTER), routerInputBalanceStart, "No leftover input in router");
assertEq(tokenOut.balanceOf(UNIVERSAL_ROUTER), routerOutputBalanceStart, "No leftover output in reactor");
assertEq(tokenOut.balanceOf(address(reactor)), 0, "No leftover output in reactor");
// swapper must have spent 100 USDC for the swap and 10 USDC for gas
assertEq(
tokenIn.balanceOf(swapper2),
swapperInputBalanceStart - 100 * USDC_ONE - 10 * USDC_ONE,
"Swapper input tokens"
);
assertGe(
tokenOut.balanceOf(swapper2),
swapperOutputBalanceStart + amountOutMin,
"Swapper did not receive enough output"
);
assertEq(
tokenIn.balanceOf(address(permitExecutor)), fillerGasInputBalanceStart + 10 * USDC_ONE, "executor balance"
);
}

// swapper creates one order containing a universal router swap for 100 DAI -> ETH
Expand All @@ -231,7 +252,7 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS

bytes[] memory actions = new bytes[](1);
MethodParameters memory methodParameters = readFixture(json, "._UNISWAP_V3_DAI_ETH");
actions[0] = abi.encode(ActionType.UniversalRouter, methodParameters.data);
actions[0] = abi.encode(UNIVERSAL_ROUTER, methodParameters.value, methodParameters.data);

RelayOrder memory order = RelayOrder({
info: OrderInfoBuilder.init(address(reactor)).withSwapper(swapper).withDeadline(block.timestamp + 100),
Expand All @@ -246,17 +267,28 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
SignedOrder memory signedOrder =
SignedOrder(abi.encode(order), signOrder(swapperPrivateKey, address(PERMIT2), order));

uint256 routerDaiBalanceBefore = DAI.balanceOf(UNIVERSAL_ROUTER);
ERC20 tokenIn = DAI;
swapperInputBalanceStart = tokenIn.balanceOf(swapper);
swapperOutputBalanceStart = swapper.balance;
routerInputBalanceStart = tokenIn.balanceOf(UNIVERSAL_ROUTER);
routerOutputBalanceStart = UNIVERSAL_ROUTER.balance;
fillerGasInputBalanceStart = USDC.balanceOf(filler);

vm.prank(filler);
snapStart("RelayOrderReactorIntegrationTest-testExecuteWithNativeAsOutput");
reactor.execute{value: methodParameters.value}(signedOrder);
snapEnd();

assertEq(DAI.balanceOf(UNIVERSAL_ROUTER), routerDaiBalanceBefore, "No leftover input in router");
assertEq(tokenIn.balanceOf(UNIVERSAL_ROUTER), routerInputBalanceStart, "No leftover input in router");
assertEq(UNIVERSAL_ROUTER.balance, routerOutputBalanceStart, "No leftover output in reactor");
assertEq(address(reactor).balance, 0, "No leftover output in reactor");
assertGe(swapper.balance, amountOutMin, "Swapper did not receive enough output");
assertEq(USDC.balanceOf((filler)), 10 * USDC_ONE, "filler did not receive enough USDC");
assertEq(tokenIn.balanceOf(swapper), swapperInputBalanceStart - 100 * ONE, "Swapper input tokens");
assertGe(
swapper.balance,
swapperOutputBalanceStart + amountOutMin - 10 * USDC_ONE,
"Swapper did not receive enough output"
);
assertEq(USDC.balanceOf(filler), fillerGasInputBalanceStart + 10 * USDC_ONE, "filler balance");
}

function testExecuteFailsIfReactorIsNotRecipient() public {
Expand All @@ -274,7 +306,7 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS

bytes[] memory actions = new bytes[](1);
MethodParameters memory methodParameters = readFixture(json, "._UNISWAP_V3_DAI_USDC_RECIPIENT_NOT_REACTOR");
actions[0] = abi.encode(ActionType.UniversalRouter, methodParameters.data);
actions[0] = abi.encode(UNIVERSAL_ROUTER, methodParameters.value, methodParameters.data);

RelayOrder memory order = RelayOrder({
info: OrderInfoBuilder.init(address(reactor)).withSwapper(swapper).withDeadline(block.timestamp + 100),
Expand All @@ -292,4 +324,14 @@ contract RelayOrderReactorIntegrationTest is GasSnapshot, Test, Interop, PermitS
vm.expectRevert(CurrencyLibrary.InsufficientBalance.selector);
reactor.execute{value: methodParameters.value}(signedOrder);
}

function _checkpointBalances(address _swapper, address _filler, ERC20 tokenIn, ERC20 tokenOut, ERC20 gasInput)
internal
{
swapperInputBalanceStart = tokenIn.balanceOf(_swapper);
swapperOutputBalanceStart = tokenOut.balanceOf(_swapper);
routerInputBalanceStart = tokenIn.balanceOf(UNIVERSAL_ROUTER);
routerOutputBalanceStart = tokenOut.balanceOf(UNIVERSAL_ROUTER);
fillerGasInputBalanceStart = gasInput.balanceOf(_filler);
}
}