From aec45da2fc36de796b9c2012fedd6c1fd03d4f44 Mon Sep 17 00:00:00 2001 From: Alice Henshaw Date: Wed, 12 Jun 2024 13:27:28 -0300 Subject: [PATCH 1/7] Make reentrancy guard transient --- contracts/base/Dispatcher.sol | 17 ++-- contracts/base/LockAndMsgSender.sol | 12 ++- contracts/libraries/Locker.sol | 21 +++++ .../CheckOwnership.gas.test.ts.snap | 2 +- .../__snapshots__/Payments.gas.test.ts.snap | 14 ++-- .../__snapshots__/Uniswap.gas.test.ts.snap | 78 +++++++++---------- .../UniversalRouter.gas.test.ts.snap | 2 +- .../UniversalVSSwapRouter.gas.test.ts.snap | 20 ++--- 8 files changed, 93 insertions(+), 73 deletions(-) create mode 100644 contracts/libraries/Locker.sol diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 8f80c589..e61608f9 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -6,6 +6,7 @@ import {V3SwapRouter} from '../modules/uniswap/v3/V3SwapRouter.sol'; import {BytesLib} from '../modules/uniswap/v3/BytesLib.sol'; import {Payments} from '../modules/Payments.sol'; import {PaymentsImmutables} from '../modules/PaymentsImmutables.sol'; +import {Locker} from '../libraries/Locker.sol'; import {Callbacks} from '../base/Callbacks.sol'; import {Commands} from '../libraries/Commands.sol'; import {LockAndMsgSender} from './LockAndMsgSender.sol'; @@ -49,7 +50,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, payerIsUser := calldataload(add(inputs.offset, 0x80)) } bytes calldata path = inputs.toBytes(3); - address payer = payerIsUser ? lockedBy : address(this); + address payer = payerIsUser ? Locker.get() : address(this); v3SwapExactInput(map(recipient), amountIn, amountOutMin, path, payer); } else if (command == Commands.V3_SWAP_EXACT_OUT) { // equivalent: abi.decode(inputs, (address, uint256, uint256, bytes, bool)) @@ -65,7 +66,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, payerIsUser := calldataload(add(inputs.offset, 0x80)) } bytes calldata path = inputs.toBytes(3); - address payer = payerIsUser ? lockedBy : address(this); + address payer = payerIsUser ? Locker.get() : address(this); v3SwapExactOutput(map(recipient), amountOut, amountInMax, path, payer); } else if (command == Commands.PERMIT2_TRANSFER_FROM) { // equivalent: abi.decode(inputs, (address, address, uint160)) @@ -77,12 +78,12 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, recipient := calldataload(add(inputs.offset, 0x20)) amount := calldataload(add(inputs.offset, 0x40)) } - permit2TransferFrom(token, lockedBy, map(recipient), amount); + permit2TransferFrom(token, Locker.get(), map(recipient), amount); } else if (command == Commands.PERMIT2_PERMIT_BATCH) { (IAllowanceTransfer.PermitBatch memory permitBatch,) = abi.decode(inputs, (IAllowanceTransfer.PermitBatch, bytes)); bytes calldata data = inputs.toBytes(1); - PERMIT2.permit(lockedBy, permitBatch, data); + PERMIT2.permit(Locker.get(), permitBatch, data); } else if (command == Commands.SWEEP) { // equivalent: abi.decode(inputs, (address, address, uint256)) address token; @@ -136,7 +137,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, payerIsUser := calldataload(add(inputs.offset, 0x80)) } address[] calldata path = inputs.toAddressArray(3); - address payer = payerIsUser ? lockedBy : address(this); + address payer = payerIsUser ? Locker.get() : address(this); v2SwapExactInput(map(recipient), amountIn, amountOutMin, path, payer); } else if (command == Commands.V2_SWAP_EXACT_OUT) { // equivalent: abi.decode(inputs, (address, uint256, uint256, bytes, bool)) @@ -152,7 +153,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, payerIsUser := calldataload(add(inputs.offset, 0x80)) } address[] calldata path = inputs.toAddressArray(3); - address payer = payerIsUser ? lockedBy : address(this); + address payer = payerIsUser ? Locker.get() : address(this); v2SwapExactOutput(map(recipient), amountOut, amountInMax, path, payer); } else if (command == Commands.PERMIT2_PERMIT) { // equivalent: abi.decode(inputs, (IAllowanceTransfer.PermitSingle, bytes)) @@ -161,7 +162,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, permitSingle := inputs.offset } bytes calldata data = inputs.toBytes(6); // PermitSingle takes first 6 slots (0..5) - PERMIT2.permit(lockedBy, permitSingle, data); + PERMIT2.permit(Locker.get(), permitSingle, data); } else if (command == Commands.WRAP_ETH) { // equivalent: abi.decode(inputs, (address, uint256)) address recipient; @@ -183,7 +184,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, Callbacks, } else if (command == Commands.PERMIT2_TRANSFER_FROM_BATCH) { (IAllowanceTransfer.AllowanceTransferDetails[] memory batchDetails) = abi.decode(inputs, (IAllowanceTransfer.AllowanceTransferDetails[])); - permit2TransferFrom(batchDetails, lockedBy); + permit2TransferFrom(batchDetails, Locker.get()); } else if (command == Commands.BALANCE_CHECK_ERC20) { // equivalent: abi.decode(inputs, (address, address, uint256)) address owner; diff --git a/contracts/base/LockAndMsgSender.sol b/contracts/base/LockAndMsgSender.sol index 472c4fe3..060295f6 100644 --- a/contracts/base/LockAndMsgSender.sol +++ b/contracts/base/LockAndMsgSender.sol @@ -2,19 +2,17 @@ pragma solidity ^0.8.24; import {Constants} from '../libraries/Constants.sol'; +import {Locker} from '../libraries/Locker.sol'; contract LockAndMsgSender { error ContractLocked(); - address internal constant NOT_LOCKED_FLAG = address(1); - address internal lockedBy = NOT_LOCKED_FLAG; - modifier isNotLocked() { if (msg.sender != address(this)) { - if (lockedBy != NOT_LOCKED_FLAG) revert ContractLocked(); - lockedBy = msg.sender; + if (Locker.get() != address(0)) revert ContractLocked(); + Locker.set(msg.sender); _; - lockedBy = NOT_LOCKED_FLAG; + Locker.set(address(0)); } else { _; } @@ -25,7 +23,7 @@ contract LockAndMsgSender { /// @return output The resultant recipient for the command function map(address recipient) internal view returns (address) { if (recipient == Constants.MSG_SENDER) { - return lockedBy; + return Locker.get(); } else if (recipient == Constants.ADDRESS_THIS) { return address(this); } else { diff --git a/contracts/libraries/Locker.sol b/contracts/libraries/Locker.sol new file mode 100644 index 00000000..713779f1 --- /dev/null +++ b/contracts/libraries/Locker.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity ^0.8.24; + +/// @notice This is a temporary library that allows us to use transient storage (tstore/tload) +/// TODO: This library can be deleted when we have the transient keyword support in solidity. +library Locker { + // The slot holding the locker state, transiently. bytes32(uint256(keccak256("LockedBy")) - 1) + bytes32 constant LOCKED_BY_SLOT = 0x0aedd6bde10e3aa2adec092b02a3e3e805795516cda41f27aa145b8f300af87a; + + function set(address locker) internal { + assembly { + tstore(LOCKED_BY_SLOT, locker) + } + } + + function get() internal view returns (address locker) { + assembly { + locker := tload(LOCKED_BY_SLOT) + } + } +} diff --git a/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap index 51121507..7fcb0967 100644 --- a/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap @@ -3,6 +3,6 @@ exports[`Check Ownership Gas gas: balance check ERC20 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 39759, + "gasUsed": 37619, } `; diff --git a/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap index 56251cf0..e5fbeb7f 100644 --- a/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap @@ -3,48 +3,48 @@ exports[`Payments Gas Tests Individual Command Tests gas: SWEEP with ERC20 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 39128, + "gasUsed": 36988, } `; exports[`Payments Gas Tests Individual Command Tests gas: SWEEP_WITH_FEE 1`] = ` Object { "calldataByteLength": 516, - "gasUsed": 67780, + "gasUsed": 65640, } `; exports[`Payments Gas Tests Individual Command Tests gas: TRANSFER with ERC20 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 38153, + "gasUsed": 36013, } `; exports[`Payments Gas Tests Individual Command Tests gas: TRANSFER with ETH 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 33724, + "gasUsed": 31584, } `; exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH 1`] = ` Object { "calldataByteLength": 324, - "gasUsed": 46692, + "gasUsed": 44552, } `; exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH_WITH_FEE 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 52970, + "gasUsed": 50830, } `; exports[`Payments Gas Tests Individual Command Tests gas: WRAP_ETH 1`] = ` Object { "calldataByteLength": 324, - "gasUsed": 57301, + "gasUsed": 53349, } `; diff --git a/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap index 4705592f..98953505 100644 --- a/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap @@ -3,105 +3,105 @@ exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, both fail but the transaction succeeds 1`] = ` Object { "calldataByteLength": 1764, - "gasUsed": 271359, + "gasUsed": 269189, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, neither fails 1`] = ` Object { "calldataByteLength": 1764, - "gasUsed": 247093, + "gasUsed": 244923, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, second sub plan fails 1`] = ` Object { "calldataByteLength": 1764, - "gasUsed": 247093, + "gasUsed": 244923, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, the first fails 1`] = ` Object { "calldataByteLength": 1764, - "gasUsed": 271359, + "gasUsed": 269189, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Interleaving routes gas: V2, then V3 1`] = ` Object { "calldataByteLength": 836, - "gasUsed": 191528, + "gasUsed": 189376, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Interleaving routes gas: V3, then V2 1`] = ` Object { "calldataByteLength": 836, - "gasUsed": 179118, + "gasUsed": 176960, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, different input tokens, each two hop, with batch permit 1`] = ` Object { "calldataByteLength": 1540, - "gasUsed": 300577, + "gasUsed": 298413, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, each two hop, with explicit permit 1`] = ` Object { "calldataByteLength": 1220, - "gasUsed": 309796, + "gasUsed": 307620, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, each two hop, with explicit permit transfer from batch 1`] = ` Object { "calldataByteLength": 1284, - "gasUsed": 311874, + "gasUsed": 309710, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, each two hop, without explicit permit 1`] = ` Object { "calldataByteLength": 900, - "gasUsed": 306382, + "gasUsed": 304218, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V3, one hop 1`] = ` Object { "calldataByteLength": 996, - "gasUsed": 178884, + "gasUsed": 176726, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V3, one hop, ADDRESS_THIS flag 1`] = ` Object { "calldataByteLength": 996, - "gasUsed": 178659, + "gasUsed": 176501, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ETH split V2 and V3, exactOut, one hop 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 196684, + "gasUsed": 194526, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ETH split V2 and V3, one hop 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 186755, + "gasUsed": 184597, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ETH --> ERC20 split V2 and V3, one hop 1`] = ` Object { "calldataByteLength": 1124, - "gasUsed": 193725, + "gasUsed": 191579, } `; @@ -143,98 +143,98 @@ Object { exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn trade, where an output fee is taken 1`] = ` Object { "calldataByteLength": 836, - "gasUsed": 128642, + "gasUsed": 126490, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 516, - "gasUsed": 108947, + "gasUsed": 106795, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, three hops 1`] = ` Object { "calldataByteLength": 580, - "gasUsed": 243553, + "gasUsed": 241401, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, three hops, no deadline 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 243295, + "gasUsed": 241143, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, two hops 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 176296, + "gasUsed": 174144, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, two hops, MSG_SENDER flag 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 176296, + "gasUsed": 174144, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 516, - "gasUsed": 109053, + "gasUsed": 106901, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, three hops 1`] = ` Object { "calldataByteLength": 580, - "gasUsed": 248954, + "gasUsed": 246802, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, two hops 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 179077, + "gasUsed": 176925, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ETH gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 125148, + "gasUsed": 122996, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ETH gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 804, - "gasUsed": 130412, + "gasUsed": 128254, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ETH gas: exactOut, with ETH fee 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 138353, + "gasUsed": 136201, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ETH --> ERC20 gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 108718, + "gasUsed": 106566, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ETH --> ERC20 gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 772, - "gasUsed": 127611, + "gasUsed": 125459, } `; @@ -283,69 +283,69 @@ Object { exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 516, - "gasUsed": 107624, + "gasUsed": 105472, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, three hops 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 256009, + "gasUsed": 253857, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, two hops 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 179203, + "gasUsed": 177051, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 516, - "gasUsed": 117154, + "gasUsed": 115002, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, three hops 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 253167, + "gasUsed": 251015, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, two hops 1`] = ` Object { "calldataByteLength": 548, - "gasUsed": 176865, + "gasUsed": 174713, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ETH gas: exactIn swap 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 123873, + "gasUsed": 121721, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ETH gas: exactOut swap 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 133475, + "gasUsed": 131323, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ETH --> ERC20 gas: exactIn swap 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 217436, + "gasUsed": 215290, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ETH --> ERC20 gas: exactOut swap 1`] = ` Object { "calldataByteLength": 772, - "gasUsed": 128593, + "gasUsed": 126441, } `; diff --git a/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap index 60180095..b3817feb 100644 --- a/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap @@ -1,3 +1,3 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`UniversalRouter Gas Tests gas: bytecode size 1`] = `12879`; +exports[`UniversalRouter Gas Tests gas: bytecode size 1`] = `13040`; diff --git a/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap index 8cd90000..516e9972 100644 --- a/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap @@ -7,32 +7,32 @@ Object { } `; -exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Max Approval Swap 1`] = `1110322`; +exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Max Approval Swap 1`] = `1103857`; -exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Sign Per Swap 1`] = `1144704`; +exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Sign Per Swap 1`] = `1138233`; exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps SwapRouter02 1`] = `1124979`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Max Approval Swap 1`] = `3101911`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Max Approval Swap 1`] = `3080388`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Sign Per Swap 1`] = `3255320`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Sign Per Swap 1`] = `3233770`; exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps SwapRouter02 1`] = `3195011`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Max Approval Swap 1`] = `4134291`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Max Approval Swap 1`] = `4102032`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Sign Per Swap 1`] = `4338065`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Sign Per Swap 1`] = `4305770`; exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions SwapRouter02 1`] = `4282374`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Max Approval Swap 1`] = `510754`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Max Approval Swap 1`] = `508593`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Sign Per Swap 1`] = `511084`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Sign Per Swap 1`] = `508923`; exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap SwapRouter02 1`] = `500008`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Max Approval Swap 1`] = `301726`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Max Approval Swap 1`] = `299577`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Sign Per Swap 1`] = `301674`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Sign Per Swap 1`] = `299525`; exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap SwapRouter02 1`] = `270033`; From dbb36f78e081ffbc07d8d79ff1609911808f7892 Mon Sep 17 00:00:00 2001 From: Alice Henshaw Date: Wed, 12 Jun 2024 13:41:49 -0300 Subject: [PATCH 2/7] locker tests --- test/foundry-tests/Locker.t.sol | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 test/foundry-tests/Locker.t.sol diff --git a/test/foundry-tests/Locker.t.sol b/test/foundry-tests/Locker.t.sol new file mode 100644 index 00000000..df330c2b --- /dev/null +++ b/test/foundry-tests/Locker.t.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import 'forge-std/Test.sol'; +import {Constants} from '../../contracts/libraries/Constants.sol'; +import {Locker} from '../../contracts/libraries/Locker.sol'; + +contract LockerTest is Test { + function test_fuzz_set_get(address locker1, address locker2, address locker3) public { + assertEq(Locker.get(), address(0)); + + Locker.set(locker1); + assertEq(Locker.get(), locker1); + + Locker.set(locker2); + assertEq(Locker.get(), locker2); + + Locker.set(locker3); + assertEq(Locker.get(), locker3); + + Locker.set(address(0)); + assertEq(Locker.get(), address(0)); + } +} From f4883c74eaa339a0acbc05165def51a9ddb50435 Mon Sep 17 00:00:00 2001 From: Alice Henshaw Date: Wed, 24 Jul 2024 11:55:53 +0100 Subject: [PATCH 3/7] abstract the locker library away --- contracts/base/Dispatcher.sol | 19 +++++++++---------- contracts/base/LockAndMsgSender.sol | 11 +++++++++-- contracts/libraries/Locker.sol | 4 ++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 438c3d66..418a8423 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -6,7 +6,6 @@ import {V3SwapRouter} from '../modules/uniswap/v3/V3SwapRouter.sol'; import {BytesLib} from '../modules/uniswap/v3/BytesLib.sol'; import {Payments} from '../modules/Payments.sol'; import {PaymentsImmutables} from '../modules/PaymentsImmutables.sol'; -import {Locker} from '../libraries/Locker.sol'; import {V3ToV4Migrator} from '../modules/V3ToV4Migrator.sol'; import {Callbacks} from '../base/Callbacks.sol'; import {Commands} from '../libraries/Commands.sol'; @@ -56,7 +55,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr payerIsUser := calldataload(add(inputs.offset, 0x80)) } bytes calldata path = inputs.toBytes(3); - address payer = payerIsUser ? Locker.get() : address(this); + address payer = payerIsUser ? _msgSender() : address(this); v3SwapExactInput(map(recipient), amountIn, amountOutMin, path, payer); } else if (command == Commands.V3_SWAP_EXACT_OUT) { // equivalent: abi.decode(inputs, (address, uint256, uint256, bytes, bool)) @@ -72,7 +71,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr payerIsUser := calldataload(add(inputs.offset, 0x80)) } bytes calldata path = inputs.toBytes(3); - address payer = payerIsUser ? Locker.get() : address(this); + address payer = payerIsUser ? _msgSender() : address(this); v3SwapExactOutput(map(recipient), amountOut, amountInMax, path, payer); } else if (command == Commands.PERMIT2_TRANSFER_FROM) { // equivalent: abi.decode(inputs, (address, address, uint160)) @@ -84,12 +83,12 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr recipient := calldataload(add(inputs.offset, 0x20)) amount := calldataload(add(inputs.offset, 0x40)) } - permit2TransferFrom(token, Locker.get(), map(recipient), amount); + permit2TransferFrom(token, _msgSender(), map(recipient), amount); } else if (command == Commands.PERMIT2_PERMIT_BATCH) { (IAllowanceTransfer.PermitBatch memory permitBatch,) = abi.decode(inputs, (IAllowanceTransfer.PermitBatch, bytes)); bytes calldata data = inputs.toBytes(1); - PERMIT2.permit(Locker.get(), permitBatch, data); + PERMIT2.permit(_msgSender(), permitBatch, data); } else if (command == Commands.SWEEP) { // equivalent: abi.decode(inputs, (address, address, uint256)) address token; @@ -143,7 +142,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr payerIsUser := calldataload(add(inputs.offset, 0x80)) } address[] calldata path = inputs.toAddressArray(3); - address payer = payerIsUser ? Locker.get() : address(this); + address payer = payerIsUser ? _msgSender() : address(this); v2SwapExactInput(map(recipient), amountIn, amountOutMin, path, payer); } else if (command == Commands.V2_SWAP_EXACT_OUT) { // equivalent: abi.decode(inputs, (address, uint256, uint256, bytes, bool)) @@ -159,7 +158,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr payerIsUser := calldataload(add(inputs.offset, 0x80)) } address[] calldata path = inputs.toAddressArray(3); - address payer = payerIsUser ? Locker.get() : address(this); + address payer = payerIsUser ? _msgSender() : address(this); v2SwapExactOutput(map(recipient), amountOut, amountInMax, path, payer); } else if (command == Commands.PERMIT2_PERMIT) { // equivalent: abi.decode(inputs, (IAllowanceTransfer.PermitSingle, bytes)) @@ -168,7 +167,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr permitSingle := inputs.offset } bytes calldata data = inputs.toBytes(6); // PermitSingle takes first 6 slots (0..5) - PERMIT2.permit(Locker.get(), permitSingle, data); + PERMIT2.permit(_msgSender(), permitSingle, data); } else if (command == Commands.WRAP_ETH) { // equivalent: abi.decode(inputs, (address, uint256)) address recipient; @@ -190,7 +189,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr } else if (command == Commands.PERMIT2_TRANSFER_FROM_BATCH) { (IAllowanceTransfer.AllowanceTransferDetails[] memory batchDetails) = abi.decode(inputs, (IAllowanceTransfer.AllowanceTransferDetails[])); - permit2TransferFrom(batchDetails, Locker.get()); + permit2TransferFrom(batchDetails, _msgSender()); } else if (command == Commands.BALANCE_CHECK_ERC20) { // equivalent: abi.decode(inputs, (address, address, uint256)) address owner; @@ -237,7 +236,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr // This can be done in 2 ways: // 1. This contract is permitted for the specific token and the caller is approved for ALL of the owner's tokens // 2. This contract is permitted for ALL of the owner's tokens and the caller is permitted for the specific token - if (!isAuthorizedForToken(Locker.get(), tokenId)) { + if (!isAuthorizedForToken(_msgSender(), tokenId)) { revert NotAuthorizedForToken(tokenId); } diff --git a/contracts/base/LockAndMsgSender.sol b/contracts/base/LockAndMsgSender.sol index 060295f6..b1edf7b8 100644 --- a/contracts/base/LockAndMsgSender.sol +++ b/contracts/base/LockAndMsgSender.sol @@ -8,8 +8,9 @@ contract LockAndMsgSender { error ContractLocked(); modifier isNotLocked() { + // The contract is allowed to reenter itself to perform EXECUTE_SUB_PLAN commands if (msg.sender != address(this)) { - if (Locker.get() != address(0)) revert ContractLocked(); + if (Locker.isLocked()) revert ContractLocked(); Locker.set(msg.sender); _; Locker.set(address(0)); @@ -18,12 +19,18 @@ contract LockAndMsgSender { } } + /// @notice Function to be used instead of msg.sender, as the contract performs self-reentrancy. So at + /// times msg.sender == address(this). Instead _msgSender() returns the initiator of the command execution + function _msgSender() internal view returns (address) { + return Locker.get(); + } + /// @notice Calculates the recipient address for a command /// @param recipient The recipient or recipient-flag for the command /// @return output The resultant recipient for the command function map(address recipient) internal view returns (address) { if (recipient == Constants.MSG_SENDER) { - return Locker.get(); + return _msgSender(); } else if (recipient == Constants.ADDRESS_THIS) { return address(this); } else { diff --git a/contracts/libraries/Locker.sol b/contracts/libraries/Locker.sol index 713779f1..c98c50b9 100644 --- a/contracts/libraries/Locker.sol +++ b/contracts/libraries/Locker.sol @@ -18,4 +18,8 @@ library Locker { locker := tload(LOCKED_BY_SLOT) } } + + function isLocked() internal view returns (bool) { + return Locker.get() != address(0); + } } From afbbe80b8099b01838ef9adf2d0fbe7b8ec437cc Mon Sep 17 00:00:00 2001 From: Alice Henshaw Date: Wed, 24 Jul 2024 12:18:09 +0100 Subject: [PATCH 4/7] Updated locker tests --- contracts/base/LockAndMsgSender.sol | 2 +- contracts/libraries/Locker.sol | 16 +++++++++------- package.json | 4 ++-- test/foundry-tests/Locker.t.sol | 17 +++++++++++++++++ 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/contracts/base/LockAndMsgSender.sol b/contracts/base/LockAndMsgSender.sol index b1edf7b8..81fab0f7 100644 --- a/contracts/base/LockAndMsgSender.sol +++ b/contracts/base/LockAndMsgSender.sol @@ -19,7 +19,7 @@ contract LockAndMsgSender { } } - /// @notice Function to be used instead of msg.sender, as the contract performs self-reentrancy. So at + /// @notice Function to be used instead of msg.sender, as the contract performs self-reentrancy. So at /// times msg.sender == address(this). Instead _msgSender() returns the initiator of the command execution function _msgSender() internal view returns (address) { return Locker.get(); diff --git a/contracts/libraries/Locker.sol b/contracts/libraries/Locker.sol index c98c50b9..aa506545 100644 --- a/contracts/libraries/Locker.sol +++ b/contracts/libraries/Locker.sol @@ -1,21 +1,23 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.24; -/// @notice This is a temporary library that allows us to use transient storage (tstore/tload) +/// @notice A library to implement a reentrancy lock in transient storage. +/// @dev Instead of storing a boolean, the locker's address is stored to allow the contract to know who locked the contract /// TODO: This library can be deleted when we have the transient keyword support in solidity. library Locker { - // The slot holding the locker state, transiently. bytes32(uint256(keccak256("LockedBy")) - 1) - bytes32 constant LOCKED_BY_SLOT = 0x0aedd6bde10e3aa2adec092b02a3e3e805795516cda41f27aa145b8f300af87a; + // The slot holding the locker state, transiently. bytes32(uint256(keccak256("Locker")) - 1) + bytes32 constant LOCKER_SLOT = 0x0e87e1788ebd9ed6a7e63c70a374cd3283e41cad601d21fbe27863899ed4a708; function set(address locker) internal { - assembly { - tstore(LOCKED_BY_SLOT, locker) + // The locker is always msg.sender or address(0) so does not need to be cleaned + assembly ("memory-safe") { + tstore(LOCKER_SLOT, locker) } } function get() internal view returns (address locker) { - assembly { - locker := tload(LOCKED_BY_SLOT) + assembly ("memory-safe") { + locker := tload(LOCKER_SLOT) } } diff --git a/package.json b/package.json index 0da6296d..c8095b6f 100644 --- a/package.json +++ b/package.json @@ -61,8 +61,8 @@ "typescript": "^3.7.3" }, "scripts": { - "compile": "hardhat compile", - "test": "hardhat test", + "compile": "hardhat compile && forge build", + "test": "yarn compile && hardhat test && forge test --isolate", "test:gas": "UPDATE_SNAPSHOT=1 yarn test --grep gas", "test:all": "UPDATE_SNAPSHOT=1 yarn test", "prettier:fix": "prettier --write '**/*.ts' && prettier --write '**/*.json'", diff --git a/test/foundry-tests/Locker.t.sol b/test/foundry-tests/Locker.t.sol index df330c2b..a7ffd454 100644 --- a/test/foundry-tests/Locker.t.sol +++ b/test/foundry-tests/Locker.t.sol @@ -21,4 +21,21 @@ contract LockerTest is Test { Locker.set(address(0)); assertEq(Locker.get(), address(0)); } + + function test_fuzz_isLocked(address locker) public { + assertEq(Locker.get(), address(0)); + assertEq(Locker.isLocked(), false); + + Locker.set(locker); + // the contract is locked when the locker is not address(0) + assertEq(Locker.isLocked(), locker != address(0)); + + Locker.set(address(0)); + assertEq(Locker.isLocked(), false); + } + + function test_lockerSlot() public { + bytes32 expectedSlot = bytes32(uint256(keccak256('Locker')) - 1); + assertEq(expectedSlot, Locker.LOCKER_SLOT); + } } From 83fcada49e275c7dac64b5bebb38524868205e3f Mon Sep 17 00:00:00 2001 From: Alice Henshaw Date: Wed, 24 Jul 2024 12:26:20 +0100 Subject: [PATCH 5/7] move map, and fix CI --- .github/workflows/forge.yml | 2 +- .github/workflows/lint.yml | 2 +- .github/workflows/test.yml | 2 +- contracts/base/Dispatcher.sol | 14 ++++++++++++++ contracts/base/LockAndMsgSender.sol | 19 +++---------------- package.json | 8 ++++---- 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/.github/workflows/forge.yml b/.github/workflows/forge.yml index 7deb0df6..7ac8fecd 100644 --- a/.github/workflows/forge.yml +++ b/.github/workflows/forge.yml @@ -56,7 +56,7 @@ jobs: - name: Run Forge tests run: | - forge test -vvv + forge test --isolate -vvv id: test env: FORK_URL: https://mainnet.infura.io/v3/${{ secrets.INFURA_API_KEY }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 1573cf2f..e6af4bd9 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -40,4 +40,4 @@ jobs: id: build - name: Run linter - run: yarn run prettier + run: yarn run lint::check diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2cdb27f1..318e9734 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -47,6 +47,6 @@ jobs: run: yarn compile - name: Run Integration tests - run: yarn test + run: yarn test:hardhat env: INFURA_API_KEY: ${{ secrets.INFURA_API_KEY }} diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 418a8423..84869d06 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -13,6 +13,7 @@ import {LockAndMsgSender} from './LockAndMsgSender.sol'; import {ERC20} from 'solmate/src/tokens/ERC20.sol'; import {IAllowanceTransfer} from 'permit2/src/interfaces/IAllowanceTransfer.sol'; import {IERC721Permit} from '@uniswap/v3-periphery/contracts/interfaces/IERC721Permit.sol'; +import {Constants} from '../libraries/Constants.sol'; /// @title Decodes and Executes Commands /// @notice Called by the UniversalRouter contract to efficiently decode and execute a singular command @@ -259,6 +260,19 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V3ToV4Migr } } + /// @notice Calculates the recipient address for a command + /// @param recipient The recipient or recipient-flag for the command + /// @return output The resultant recipient for the command + function map(address recipient) internal view returns (address) { + if (recipient == Constants.MSG_SENDER) { + return _msgSender(); + } else if (recipient == Constants.ADDRESS_THIS) { + return address(this); + } else { + return recipient; + } + } + /// @notice Executes encoded commands along with provided inputs. /// @param commands A set of concatenated commands, each 1 byte in length /// @param inputs An array of byte strings containing abi encoded inputs for each command diff --git a/contracts/base/LockAndMsgSender.sol b/contracts/base/LockAndMsgSender.sol index 81fab0f7..e4dbbf74 100644 --- a/contracts/base/LockAndMsgSender.sol +++ b/contracts/base/LockAndMsgSender.sol @@ -1,12 +1,12 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.24; -import {Constants} from '../libraries/Constants.sol'; import {Locker} from '../libraries/Locker.sol'; contract LockAndMsgSender { error ContractLocked(); + /// @notice Modifier enforcing a reentrancy lock that allows self-reentrancy modifier isNotLocked() { // The contract is allowed to reenter itself to perform EXECUTE_SUB_PLAN commands if (msg.sender != address(this)) { @@ -19,22 +19,9 @@ contract LockAndMsgSender { } } - /// @notice Function to be used instead of msg.sender, as the contract performs self-reentrancy. So at - /// times msg.sender == address(this). Instead _msgSender() returns the initiator of the command execution + /// @notice Function to be used instead of msg.sender, as the contract performs self-reentrancy and at + /// times msg.sender == address(this). Instead _msgSender() returns the initiator of the lock function _msgSender() internal view returns (address) { return Locker.get(); } - - /// @notice Calculates the recipient address for a command - /// @param recipient The recipient or recipient-flag for the command - /// @return output The resultant recipient for the command - function map(address recipient) internal view returns (address) { - if (recipient == Constants.MSG_SENDER) { - return _msgSender(); - } else if (recipient == Constants.ADDRESS_THIS) { - return address(this); - } else { - return recipient; - } - } } diff --git a/package.json b/package.json index c8095b6f..a4fd2fe4 100644 --- a/package.json +++ b/package.json @@ -62,11 +62,11 @@ }, "scripts": { "compile": "hardhat compile && forge build", - "test": "yarn compile && hardhat test && forge test --isolate", + "test:hardhat": "yarn compile && hardhat test", "test:gas": "UPDATE_SNAPSHOT=1 yarn test --grep gas", - "test:all": "UPDATE_SNAPSHOT=1 yarn test", + "test:all": "UPDATE_SNAPSHOT=1 yarn test:hardhat && forge test --isolate", "prettier:fix": "prettier --write '**/*.ts' && prettier --write '**/*.json'", - "lint:fix": "yarn prettier:fix && forge fmt", - "prettier": "prettier --check '**/*.ts' && forge fmt --check" + "lint": "yarn prettier:fix && forge fmt", + "lint:check": "prettier --check '**/*.ts' && forge fmt --check" } } From 93c269608f880aea06eb7ede922501e63125d56a Mon Sep 17 00:00:00 2001 From: Alice <34962750+hensha256@users.noreply.github.com> Date: Wed, 24 Jul 2024 12:26:50 +0100 Subject: [PATCH 6/7] Update lint.yml --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e6af4bd9..f9ab6491 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -40,4 +40,4 @@ jobs: id: build - name: Run linter - run: yarn run lint::check + run: yarn run lint:check From 333fd3a70713326cf5571860a652275e8e59db51 Mon Sep 17 00:00:00 2001 From: Alice Henshaw Date: Wed, 24 Jul 2024 15:56:47 +0100 Subject: [PATCH 7/7] update comment --- contracts/base/LockAndMsgSender.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/base/LockAndMsgSender.sol b/contracts/base/LockAndMsgSender.sol index e4dbbf74..c9adbf86 100644 --- a/contracts/base/LockAndMsgSender.sol +++ b/contracts/base/LockAndMsgSender.sol @@ -8,13 +8,14 @@ contract LockAndMsgSender { /// @notice Modifier enforcing a reentrancy lock that allows self-reentrancy modifier isNotLocked() { - // The contract is allowed to reenter itself to perform EXECUTE_SUB_PLAN commands + // Apply a reentrancy lock for all external callers if (msg.sender != address(this)) { if (Locker.isLocked()) revert ContractLocked(); Locker.set(msg.sender); _; Locker.set(address(0)); } else { + // The contract is allowed to reenter itself, so the lock is not checked _; } }