From e91ee9612995eb038fb0f0c837438976cedc9a9a Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Wed, 22 Jan 2025 16:55:17 -0500 Subject: [PATCH 1/7] feat: delegatecall to executor in SwapExecutionDispatcher --- foundry/interfaces/ISwapExecutor.sol | 38 ++++++++++++++ foundry/remappings.txt | 1 + foundry/src/SwapExecutionDispatcher.sol | 59 +++++++++++++++++++++- foundry/test/SwapExecutionDispatcher.t.sol | 36 +++++++++++++ 4 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 foundry/interfaces/ISwapExecutor.sol create mode 100644 foundry/test/SwapExecutionDispatcher.t.sol diff --git a/foundry/interfaces/ISwapExecutor.sol b/foundry/interfaces/ISwapExecutor.sol new file mode 100644 index 0000000..e79a868 --- /dev/null +++ b/foundry/interfaces/ISwapExecutor.sol @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.7.5; + +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +pragma abicoder v2; + +interface ISwapExecutor { + /** + * @notice Performs a swap on a liquidity pool. + * @dev This method can either take the amount of the input token or the amount + * of the output token that we would like to swap. If called with the amount of + * the input token, the amount of the output token will be returned, and vice + * versa. Whether it is the input or output that is given, is encoded in the data + * parameter. + * + * Note Part of the informal interface is that the executor supports sending the received + * tokens to a receiver address. If the underlying smart contract does not provide this + * functionality consider adding an additional transfer in the implementation. + * + * This function is marked as `payable` to accommodate delegatecalls, which can forward + * a potential `msg.value` to it. + * + * @param givenAmount The amount of either the input token or output token to swap. + * @param data Data that holds information necessary to perform the swap. + * @return calculatedAmount The amount of either the input token or output token + * swapped, depending on the givenAmount inputted. + */ + function swap(uint256 givenAmount, bytes calldata data) + external + payable + returns (uint256 calculatedAmount); +} + +interface ISwapExecutorErrors { + error InvalidParameterLength(uint256); + error UnknownCurveType(uint8); +} diff --git a/foundry/remappings.txt b/foundry/remappings.txt index f1fbdf6..0ab4175 100644 --- a/foundry/remappings.txt +++ b/foundry/remappings.txt @@ -1,3 +1,4 @@ @openzeppelin/=lib/openzeppelin-contracts/ +@interfaces/=interfaces/ @permit2/=lib/permit2/ @src/=src/ \ No newline at end of file diff --git a/foundry/src/SwapExecutionDispatcher.sol b/foundry/src/SwapExecutionDispatcher.sol index 3173104..53435d7 100644 --- a/foundry/src/SwapExecutionDispatcher.sol +++ b/foundry/src/SwapExecutionDispatcher.sol @@ -1,6 +1,10 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.28; +import "@interfaces/ISwapExecutor.sol"; + +error SwapExecutionDispatcher__UnapprovedExecutor(); + /** * @title SwapExecutionDispatcher - Dispatch swap execution to external contracts * @author PropellerHeads Devs @@ -10,8 +14,61 @@ pragma solidity ^0.8.28; * be called using delegatecall so they can share state with the main * contract if needed. * - * Note Executor contracts need to implement the ISwapExecutor interface + * Note Executor contracts need to implement the ISwapExecutor interface unless + * an alternate selector is specified. */ contract SwapExecutionDispatcher { mapping(address => bool) public swapExecutors; + + /** + * @dev Calls an executor, assumes swap.protocolData contains + * token addresses if required by the executor. + */ + function _callSwapExecutor( + uint8 exchange, + uint256 amount, + bytes calldata data + ) internal returns (uint256 calculatedAmount) { + address executor; + bytes4 decodedSelector; + bytes memory protocolData; + + (executor, decodedSelector, protocolData) = + _decodeExecutorAndSelector(data); + + bytes4 selector = decodedSelector == bytes4(0) + ? ISwapExecutor.swap.selector + : decodedSelector; + + if (!swapExecutors[executor]){ + revert SwapExecutionDispatcher__UnapprovedExecutor(); + } + + (bool success, bytes memory result) = executor.delegatecall( + abi.encodeWithSelector(selector, amount, protocolData) + ); + + if (!success) { + revert( + string( + result.length > 0 + ? result + : abi.encodePacked("Swap execution failed") + ) + ); + } + + calculatedAmount = abi.decode(result, (uint256)); + } + + function _decodeExecutorAndSelector(bytes calldata data) + internal + pure + returns (address executor, bytes4 selector, bytes memory protocolData) + { + require(data.length >= 24, "Invalid data length"); + executor = address(uint160(bytes20(data[:20]))); + selector = bytes4(data[20:24]); + protocolData = data[24:]; + } } diff --git a/foundry/test/SwapExecutionDispatcher.t.sol b/foundry/test/SwapExecutionDispatcher.t.sol new file mode 100644 index 0000000..0e81dfb --- /dev/null +++ b/foundry/test/SwapExecutionDispatcher.t.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.28; + +import "@src/SwapExecutionDispatcher.sol"; +import "./TychoRouterTestSetup.sol"; + + +contract SwapExecutionDispatcherExposed is SwapExecutionDispatcher { + function exposedDecodeExecutorAndSelector(bytes calldata data) + external + pure + returns (address executor, bytes4 selector, bytes memory protocolData) + { + return _decodeExecutorAndSelector(data); + } +} + +contract SwapExecutionDispatcherTest is TychoRouterTestSetup { + SwapExecutionDispatcherExposed dispatcherExposed; + + function setupExecutionDispatcher() public { + dispatcherExposed = new SwapExecutionDispatcherExposed(); + } + + function testDecodeExecutorAndSelector() public { + setupExecutionDispatcher(); + bytes memory data = + hex"6611e616d2db3244244a54c754a16dd3ac7ca7a2aabbccdd1111111111111111"; + (address executor, bytes4 selector, bytes memory protocolData) = + dispatcherExposed.exposedDecodeExecutorAndSelector(data); + assert(executor == address(0x6611e616d2db3244244A54c754A16dd3ac7cA7a2)); + assert(selector == bytes4(0xaabbccdd)); + // Direct bytes comparison not supported - must use keccak + assert(keccak256(protocolData) == keccak256(hex"1111111111111111")); + } +} From 825af0f8d73b08bedb10b89e9545a76f553d649c Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Thu, 23 Jan 2025 15:01:21 -0500 Subject: [PATCH 2/7] test: Thorough tests for main SwapExecutionDispatcher method --- foundry/src/SwapExecutionDispatcher.sol | 11 ++-- foundry/test/Constants.sol | 5 +- foundry/test/SwapExecutionDispatcher.t.sol | 60 +++++++++++++++++++++- 3 files changed, 68 insertions(+), 8 deletions(-) diff --git a/foundry/src/SwapExecutionDispatcher.sol b/foundry/src/SwapExecutionDispatcher.sol index 53435d7..b688bac 100644 --- a/foundry/src/SwapExecutionDispatcher.sol +++ b/foundry/src/SwapExecutionDispatcher.sol @@ -24,11 +24,10 @@ contract SwapExecutionDispatcher { * @dev Calls an executor, assumes swap.protocolData contains * token addresses if required by the executor. */ - function _callSwapExecutor( - uint8 exchange, - uint256 amount, - bytes calldata data - ) internal returns (uint256 calculatedAmount) { + function _callSwapExecutor(uint256 amount, bytes calldata data) + internal + returns (uint256 calculatedAmount) + { address executor; bytes4 decodedSelector; bytes memory protocolData; @@ -40,7 +39,7 @@ contract SwapExecutionDispatcher { ? ISwapExecutor.swap.selector : decodedSelector; - if (!swapExecutors[executor]){ + if (!swapExecutors[executor]) { revert SwapExecutionDispatcher__UnapprovedExecutor(); } diff --git a/foundry/test/Constants.sol b/foundry/test/Constants.sol index 521dff7..7a98676 100644 --- a/foundry/test/Constants.sol +++ b/foundry/test/Constants.sol @@ -8,7 +8,10 @@ contract Constants is Test { address BOB = makeAddr("bob"); //bob=someone!=us address FUND_RESCUER = makeAddr("fundRescuer"); address FEE_SETTER = makeAddr("feeSetter"); + address FEE_RECEIVER = makeAddr("feeReceiver"); + // dummy contracts address DUMMY = makeAddr("dummy"); - address FEE_RECEIVER = makeAddr("feeReceiver"); + + address WETH_ADDR = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); } diff --git a/foundry/test/SwapExecutionDispatcher.t.sol b/foundry/test/SwapExecutionDispatcher.t.sol index 0e81dfb..0e6139c 100644 --- a/foundry/test/SwapExecutionDispatcher.t.sol +++ b/foundry/test/SwapExecutionDispatcher.t.sol @@ -4,8 +4,14 @@ pragma solidity ^0.8.28; import "@src/SwapExecutionDispatcher.sol"; import "./TychoRouterTestSetup.sol"; - contract SwapExecutionDispatcherExposed is SwapExecutionDispatcher { + function exposedCallSwapExecutor(uint256 amount, bytes calldata data) + external + returns (uint256 calculatedAmount) + { + return _callSwapExecutor(amount, data); + } + function exposedDecodeExecutorAndSelector(bytes calldata data) external pure @@ -13,13 +19,65 @@ contract SwapExecutionDispatcherExposed is SwapExecutionDispatcher { { return _decodeExecutorAndSelector(data); } + + function setSwapExecutor(address target) external { + swapExecutors[target] = true; + } } contract SwapExecutionDispatcherTest is TychoRouterTestSetup { SwapExecutionDispatcherExposed dispatcherExposed; function setupExecutionDispatcher() public { + uint256 forkBlock = 20673900; + vm.createSelectFork(vm.rpcUrl("mainnet"), forkBlock); dispatcherExposed = new SwapExecutionDispatcherExposed(); + dispatcherExposed.setSwapExecutor( + address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) + ); + deal(WETH_ADDR, address(dispatcherExposed), 15000000000000000000); + } + + function testCallSwapExecutor1() public { + // Test case taken from existing transaction + // 0x755d603962b30f416cf3eefae8d55204d6ffdf746465b2a94aca216faab63804 + setupExecutionDispatcher(); + bytes memory data = + hex"e592557AB9F4A75D992283fD6066312FF013ba3dbd0625ab5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593c81c"; + uint256 givenAmount = 15000000000000000000; + uint256 amount = + dispatcherExposed.exposedCallSwapExecutor(givenAmount, data); + assert(amount == 35144641819); + } + + function testCallSwapExecutorNoSelector() public { + // Test case taken from existing transaction + // 0x755d603962b30f416cf3eefae8d55204d6ffdf746465b2a94aca216faab63804 + // No selector is passed, so the standard swap selector should be used + setupExecutionDispatcher(); + bytes memory data = + hex"e592557AB9F4A75D992283fD6066312FF013ba3d000000005615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593c81c"; + uint256 givenAmount = 15000000000000000000; + uint256 amount = + dispatcherExposed.exposedCallSwapExecutor(givenAmount, data); + assert(amount == 35144641819); + } + + function testCallSwapExecutorCallFailed() public { + // Bad data is provided to an approved swap executor - causing the call to fail + setupExecutionDispatcher(); + bytes memory data = + hex"e592557AB9F4A75D992283fD6066312FF013ba3dbd0625ab5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593"; + vm.expectRevert(); + dispatcherExposed.exposedCallSwapExecutor(0, data); + } + + function testCallSwapExecutorUnapprovedExecutor() public { + setupExecutionDispatcher(); + bytes memory data = + hex"5d622C9053b8FFB1B3465495C8a42E603632bA70aabbccdd1111111111111111"; + vm.expectRevert(); + dispatcherExposed.exposedCallSwapExecutor(0, data); } function testDecodeExecutorAndSelector() public { From b616e11354ee325dcbecff70caf4e7daf4d144d0 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Thu, 23 Jan 2025 15:07:56 -0500 Subject: [PATCH 3/7] fix: Silence slither warnings - low level calls are fine, since we are checking for success, and we have already checked for contract existence when setting swap executors - dead-code is silenced - fix solidity version --- foundry/interfaces/ISwapExecutor.sol | 2 +- foundry/src/SwapExecutionDispatcher.sol | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/foundry/interfaces/ISwapExecutor.sol b/foundry/interfaces/ISwapExecutor.sol index e79a868..db71f1a 100644 --- a/foundry/interfaces/ISwapExecutor.sol +++ b/foundry/interfaces/ISwapExecutor.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity >=0.7.5; +pragma solidity ^0.8.28; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; diff --git a/foundry/src/SwapExecutionDispatcher.sol b/foundry/src/SwapExecutionDispatcher.sol index b688bac..20e7e28 100644 --- a/foundry/src/SwapExecutionDispatcher.sol +++ b/foundry/src/SwapExecutionDispatcher.sol @@ -24,6 +24,7 @@ contract SwapExecutionDispatcher { * @dev Calls an executor, assumes swap.protocolData contains * token addresses if required by the executor. */ + // slither-disable-next-line dead-code function _callSwapExecutor(uint256 amount, bytes calldata data) internal returns (uint256 calculatedAmount) @@ -43,6 +44,7 @@ contract SwapExecutionDispatcher { revert SwapExecutionDispatcher__UnapprovedExecutor(); } + // slither-disable-next-line low-level-calls (bool success, bytes memory result) = executor.delegatecall( abi.encodeWithSelector(selector, amount, protocolData) ); @@ -60,6 +62,7 @@ contract SwapExecutionDispatcher { calculatedAmount = abi.decode(result, (uint256)); } + // slither-disable-next-line dead-code function _decodeExecutorAndSelector(bytes calldata data) internal pure From fb9f340cb798683d4e0eebc3fbfe628d510aa72b Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Thu, 23 Jan 2025 15:40:24 -0500 Subject: [PATCH 4/7] refactor: Move contract code-checking logic to SwapExecutionDispatcher I was inspired to do this because, when disabling the slither check for the delegatecall when calling the swap executor, I realized it's not clear from the same contract that we have already checked for contract code existence when setting the executor. This made me feel uneasy, as this contract can then not stand alone and must rely on the higher level contract to safely check for code existence, otherwise the delegatecall is unsafe. Keeping this logic in a separate contract seems error-prone to me, as we may remove the check for code existence without immediately realizing the implications of doing so. For this reason I have organized it as follows: - Logic/tests relating to proper roles/access control in the main TychoRouter. - Lower-level logic/tests that checks contract validity before setting the executor in the SwapExecutionDispatcher --- foundry/src/SwapExecutionDispatcher.sol | 24 +++++++++ foundry/src/TychoRouter.sol | 12 ++--- foundry/test/Constants.sol | 8 +++ foundry/test/SwapExecutionDispatcher.t.sol | 62 +++++++++++++++++----- foundry/test/TychoRouter.t.sol | 31 +---------- foundry/test/TychoRouterTestSetup.sol | 8 --- 6 files changed, 86 insertions(+), 59 deletions(-) diff --git a/foundry/src/SwapExecutionDispatcher.sol b/foundry/src/SwapExecutionDispatcher.sol index 20e7e28..c8edd7a 100644 --- a/foundry/src/SwapExecutionDispatcher.sol +++ b/foundry/src/SwapExecutionDispatcher.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.28; import "@interfaces/ISwapExecutor.sol"; error SwapExecutionDispatcher__UnapprovedExecutor(); +error SwapExecutionDispatcher__NonContractExecutor(); /** * @title SwapExecutionDispatcher - Dispatch swap execution to external contracts @@ -20,6 +21,29 @@ error SwapExecutionDispatcher__UnapprovedExecutor(); contract SwapExecutionDispatcher { mapping(address => bool) public swapExecutors; + event ExecutorSet(address indexed executor); + + /** + * @dev Adds or replace an approved swap executor contract address if it is a + * contract. + * @param target address of the swap executor contract + */ + function _setSwapExecutor(address target) internal { + if (target.code.length == 0) { + revert SwapExecutionDispatcher__NonContractExecutor(); + } + swapExecutors[target] = true; + emit ExecutorSet(target); + } + + /** + * @dev Remove an approved swap executor contract address + * @param target address of the swap executor contract + */ + function _removeSwapExecutor(address target) internal { + delete swapExecutors[target]; + } + /** * @dev Calls an executor, assumes swap.protocolData contains * token addresses if required by the executor. diff --git a/foundry/src/TychoRouter.sol b/foundry/src/TychoRouter.sol index 4383618..635c2ea 100644 --- a/foundry/src/TychoRouter.sol +++ b/foundry/src/TychoRouter.sol @@ -10,7 +10,6 @@ import "./CallbackVerificationDispatcher.sol"; error TychoRouter__WithdrawalFailed(); error TychoRouter__AddressZero(); -error TychoRouter__NonContractExecutor(); error TychoRouter__NonContractVerifier(); contract TychoRouter is @@ -45,7 +44,6 @@ contract TychoRouter is address indexed oldFeeReceiver, address indexed newFeeReceiver ); event FeeSet(uint256 indexed oldFee, uint256 indexed newFee); - event ExecutorSet(address indexed executor); event CallbackVerifierSet(address indexed callbackVerifier); constructor(address _permit2) { @@ -95,26 +93,24 @@ contract TychoRouter is /** * @dev Entrypoint to add or replace an approved swap executor contract address - * @param target address of the swap method contract + * @param target address of the swap executor contract */ function setSwapExecutor(address target) external onlyRole(EXECUTOR_SETTER_ROLE) { - if (target.code.length == 0) revert TychoRouter__NonContractExecutor(); - swapExecutors[target] = true; - emit ExecutorSet(target); + _setSwapExecutor(target); } /** * @dev Entrypoint to remove an approved swap executor contract address - * @param target address of the swap method contract + * @param target address of the swap executor contract */ function removeSwapExecutor(address target) external onlyRole(EXECUTOR_SETTER_ROLE) { - delete swapExecutors[target]; + _removeSwapExecutor(target); } /** diff --git a/foundry/test/Constants.sol b/foundry/test/Constants.sol index 7a98676..dfd2c1b 100644 --- a/foundry/test/Constants.sol +++ b/foundry/test/Constants.sol @@ -14,4 +14,12 @@ contract Constants is Test { address DUMMY = makeAddr("dummy"); address WETH_ADDR = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); + + /** + * @dev Deploys a dummy contract with non-empty bytecode + */ + function deployDummyContract() internal { + bytes memory minimalBytecode = hex"01"; // Single-byte bytecode + vm.etch(DUMMY, minimalBytecode); // Deploy minimal bytecode + } } diff --git a/foundry/test/SwapExecutionDispatcher.t.sol b/foundry/test/SwapExecutionDispatcher.t.sol index 0e6139c..6608c04 100644 --- a/foundry/test/SwapExecutionDispatcher.t.sol +++ b/foundry/test/SwapExecutionDispatcher.t.sol @@ -20,28 +20,62 @@ contract SwapExecutionDispatcherExposed is SwapExecutionDispatcher { return _decodeExecutorAndSelector(data); } - function setSwapExecutor(address target) external { - swapExecutors[target] = true; + function exposedSetSwapExecutor(address target) external { + _setSwapExecutor(target); + } + + function exposedRemoveSwapExecutor(address target) external { + _removeSwapExecutor(target); } } -contract SwapExecutionDispatcherTest is TychoRouterTestSetup { +contract SwapExecutionDispatcherTest is Constants { SwapExecutionDispatcherExposed dispatcherExposed; - function setupExecutionDispatcher() public { + event ExecutorSet(address indexed executor); + + function setUp() public { uint256 forkBlock = 20673900; vm.createSelectFork(vm.rpcUrl("mainnet"), forkBlock); dispatcherExposed = new SwapExecutionDispatcherExposed(); - dispatcherExposed.setSwapExecutor( - address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) - ); deal(WETH_ADDR, address(dispatcherExposed), 15000000000000000000); + deployDummyContract(); } - function testCallSwapExecutor1() public { + function testSetValidExecutor() public { + vm.expectEmit(); + // Define the event we expect to be emitted at the next step + emit ExecutorSet(DUMMY); + dispatcherExposed.exposedSetSwapExecutor(DUMMY); + assert(dispatcherExposed.swapExecutors(DUMMY) == true); + } + + function testRemoveExecutor() public { + dispatcherExposed.exposedSetSwapExecutor(DUMMY); + dispatcherExposed.exposedRemoveSwapExecutor(DUMMY); + assert(dispatcherExposed.swapExecutors(DUMMY) == false); + } + + function testRemoveUnSetExecutor() public { + dispatcherExposed.exposedRemoveSwapExecutor(BOB); + assert(dispatcherExposed.swapExecutors(BOB) == false); + } + + function testSetExecutorNonContract() public { + vm.expectRevert( + abi.encodeWithSelector( + SwapExecutionDispatcher__NonContractExecutor.selector + ) + ); + dispatcherExposed.exposedSetSwapExecutor(BOB); + } + + function testCallSwapExecutor() public { // Test case taken from existing transaction // 0x755d603962b30f416cf3eefae8d55204d6ffdf746465b2a94aca216faab63804 - setupExecutionDispatcher(); + dispatcherExposed.exposedSetSwapExecutor( + address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) + ); bytes memory data = hex"e592557AB9F4A75D992283fD6066312FF013ba3dbd0625ab5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593c81c"; uint256 givenAmount = 15000000000000000000; @@ -54,7 +88,9 @@ contract SwapExecutionDispatcherTest is TychoRouterTestSetup { // Test case taken from existing transaction // 0x755d603962b30f416cf3eefae8d55204d6ffdf746465b2a94aca216faab63804 // No selector is passed, so the standard swap selector should be used - setupExecutionDispatcher(); + dispatcherExposed.exposedSetSwapExecutor( + address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) + ); bytes memory data = hex"e592557AB9F4A75D992283fD6066312FF013ba3d000000005615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593c81c"; uint256 givenAmount = 15000000000000000000; @@ -65,7 +101,9 @@ contract SwapExecutionDispatcherTest is TychoRouterTestSetup { function testCallSwapExecutorCallFailed() public { // Bad data is provided to an approved swap executor - causing the call to fail - setupExecutionDispatcher(); + dispatcherExposed.exposedSetSwapExecutor( + address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) + ); bytes memory data = hex"e592557AB9F4A75D992283fD6066312FF013ba3dbd0625ab5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593"; vm.expectRevert(); @@ -73,7 +111,6 @@ contract SwapExecutionDispatcherTest is TychoRouterTestSetup { } function testCallSwapExecutorUnapprovedExecutor() public { - setupExecutionDispatcher(); bytes memory data = hex"5d622C9053b8FFB1B3465495C8a42E603632bA70aabbccdd1111111111111111"; vm.expectRevert(); @@ -81,7 +118,6 @@ contract SwapExecutionDispatcherTest is TychoRouterTestSetup { } function testDecodeExecutorAndSelector() public { - setupExecutionDispatcher(); bytes memory data = hex"6611e616d2db3244244a54c754a16dd3ac7ca7a2aabbccdd1111111111111111"; (address executor, bytes4 selector, bytes memory protocolData) = diff --git a/foundry/test/TychoRouter.t.sol b/foundry/test/TychoRouter.t.sol index fb9ad4b..2e808b7 100644 --- a/foundry/test/TychoRouter.t.sol +++ b/foundry/test/TychoRouter.t.sol @@ -20,33 +20,13 @@ contract TychoRouterTest is TychoRouterTestSetup { address indexed token, uint256 amount, address indexed receiver ); - function testSetValidExecutor() public { + function testSetExecutorValidRole() public { vm.startPrank(executorSetter); - vm.expectEmit(); - // Define the event we expect to be emitted at the next step - emit ExecutorSet(DUMMY); - tychoRouter.setSwapExecutor(DUMMY); vm.stopPrank(); - assert(tychoRouter.swapExecutors(DUMMY) == true); } - function testRemoveExecutor() public { - vm.startPrank(executorSetter); - tychoRouter.setSwapExecutor(DUMMY); - tychoRouter.removeSwapExecutor(DUMMY); - vm.stopPrank(); - assert(tychoRouter.swapExecutors(DUMMY) == false); - } - - function testRemoveUnSetExecutor() public { - vm.startPrank(executorSetter); - tychoRouter.removeSwapExecutor(BOB); - vm.stopPrank(); - assert(tychoRouter.swapExecutors(BOB) == false); - } - function testRemoveExecutorMissingSetterRole() public { vm.expectRevert(); tychoRouter.removeSwapExecutor(BOB); @@ -57,15 +37,6 @@ contract TychoRouterTest is TychoRouterTestSetup { tychoRouter.setSwapExecutor(DUMMY); } - function testSetExecutorNonContract() public { - vm.startPrank(executorSetter); - vm.expectRevert( - abi.encodeWithSelector(TychoRouter__NonContractExecutor.selector) - ); - tychoRouter.setSwapExecutor(BOB); - vm.stopPrank(); - } - function testSetValidVerifier() public { vm.startPrank(executorSetter); vm.expectEmit(); diff --git a/foundry/test/TychoRouterTestSetup.sol b/foundry/test/TychoRouterTestSetup.sol index 9765d76..a97e3a9 100644 --- a/foundry/test/TychoRouterTestSetup.sol +++ b/foundry/test/TychoRouterTestSetup.sol @@ -28,14 +28,6 @@ contract TychoRouterTestSetup is Test, Constants { vm.stopPrank(); } - /** - * @dev Deploys a dummy contract with non-empty bytecode - */ - function deployDummyContract() internal { - bytes memory minimalBytecode = hex"01"; // Single-byte bytecode - vm.etch(DUMMY, minimalBytecode); // Deploy minimal bytecode - } - /** * @dev Mints tokens to the given address * @param amount The amount of tokens to mint From 3df17e892491fbb47bf6ed03680b0fb7fbb68140 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Thu, 23 Jan 2025 15:43:16 -0500 Subject: [PATCH 5/7] fix: ISwapExecutor shouldn't be payable No reason for that. --- foundry/interfaces/ISwapExecutor.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/foundry/interfaces/ISwapExecutor.sol b/foundry/interfaces/ISwapExecutor.sol index db71f1a..d8bf82c 100644 --- a/foundry/interfaces/ISwapExecutor.sol +++ b/foundry/interfaces/ISwapExecutor.sol @@ -28,7 +28,6 @@ interface ISwapExecutor { */ function swap(uint256 givenAmount, bytes calldata data) external - payable returns (uint256 calculatedAmount); } From 5214710530ff01192f8124789a4a54ed0c773489 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Fri, 24 Jan 2025 11:39:49 -0500 Subject: [PATCH 6/7] chore: docstrings and other small improvements --- foundry/interfaces/ISwapExecutor.sol | 18 ++++++------------ foundry/src/SwapExecutionDispatcher.sol | 14 +++++++------- foundry/test/SwapExecutionDispatcher.t.sol | 21 ++++++++++++++++++--- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/foundry/interfaces/ISwapExecutor.sol b/foundry/interfaces/ISwapExecutor.sol index d8bf82c..fef5750 100644 --- a/foundry/interfaces/ISwapExecutor.sol +++ b/foundry/interfaces/ISwapExecutor.sol @@ -8,23 +8,17 @@ pragma abicoder v2; interface ISwapExecutor { /** * @notice Performs a swap on a liquidity pool. - * @dev This method can either take the amount of the input token or the amount - * of the output token that we would like to swap. If called with the amount of - * the input token, the amount of the output token will be returned, and vice - * versa. Whether it is the input or output that is given, is encoded in the data - * parameter. + * @dev This method takes the amount of the input token and returns the amount of + * the output token which has been swapped. * * Note Part of the informal interface is that the executor supports sending the received * tokens to a receiver address. If the underlying smart contract does not provide this * functionality consider adding an additional transfer in the implementation. * - * This function is marked as `payable` to accommodate delegatecalls, which can forward - * a potential `msg.value` to it. - * - * @param givenAmount The amount of either the input token or output token to swap. + * @param givenAmount The amount of the input token to swap. * @param data Data that holds information necessary to perform the swap. - * @return calculatedAmount The amount of either the input token or output token - * swapped, depending on the givenAmount inputted. + * @return calculatedAmount The amount of the output token swapped, depending on + * the givenAmount inputted. */ function swap(uint256 givenAmount, bytes calldata data) external @@ -33,5 +27,5 @@ interface ISwapExecutor { interface ISwapExecutorErrors { error InvalidParameterLength(uint256); - error UnknownCurveType(uint8); + error UnknownPoolType(uint8); } diff --git a/foundry/src/SwapExecutionDispatcher.sol b/foundry/src/SwapExecutionDispatcher.sol index c8edd7a..30f335a 100644 --- a/foundry/src/SwapExecutionDispatcher.sol +++ b/foundry/src/SwapExecutionDispatcher.sol @@ -24,7 +24,7 @@ contract SwapExecutionDispatcher { event ExecutorSet(address indexed executor); /** - * @dev Adds or replace an approved swap executor contract address if it is a + * @dev Adds or replaces an approved swap executor contract address if it is a * contract. * @param target address of the swap executor contract */ @@ -37,7 +37,7 @@ contract SwapExecutionDispatcher { } /** - * @dev Remove an approved swap executor contract address + * @dev Removes an approved swap executor contract address * @param target address of the swap executor contract */ function _removeSwapExecutor(address target) internal { @@ -46,7 +46,7 @@ contract SwapExecutionDispatcher { /** * @dev Calls an executor, assumes swap.protocolData contains - * token addresses if required by the executor. + * protocol-specific data required by the executor. */ // slither-disable-next-line dead-code function _callSwapExecutor(uint256 amount, bytes calldata data) @@ -60,14 +60,14 @@ contract SwapExecutionDispatcher { (executor, decodedSelector, protocolData) = _decodeExecutorAndSelector(data); - bytes4 selector = decodedSelector == bytes4(0) - ? ISwapExecutor.swap.selector - : decodedSelector; - if (!swapExecutors[executor]) { revert SwapExecutionDispatcher__UnapprovedExecutor(); } + bytes4 selector = decodedSelector == bytes4(0) + ? ISwapExecutor.swap.selector + : decodedSelector; + // slither-disable-next-line low-level-calls (bool success, bytes memory result) = executor.delegatecall( abi.encodeWithSelector(selector, amount, protocolData) diff --git a/foundry/test/SwapExecutionDispatcher.t.sol b/foundry/test/SwapExecutionDispatcher.t.sol index 6608c04..635b3a1 100644 --- a/foundry/test/SwapExecutionDispatcher.t.sol +++ b/foundry/test/SwapExecutionDispatcher.t.sol @@ -38,7 +38,7 @@ contract SwapExecutionDispatcherTest is Constants { uint256 forkBlock = 20673900; vm.createSelectFork(vm.rpcUrl("mainnet"), forkBlock); dispatcherExposed = new SwapExecutionDispatcherExposed(); - deal(WETH_ADDR, address(dispatcherExposed), 15000000000000000000); + deal(WETH_ADDR, address(dispatcherExposed), 15 ether); deployDummyContract(); } @@ -73,12 +73,19 @@ contract SwapExecutionDispatcherTest is Constants { function testCallSwapExecutor() public { // Test case taken from existing transaction // 0x755d603962b30f416cf3eefae8d55204d6ffdf746465b2a94aca216faab63804 + // For this test, we can use any executor and any calldata that we know works + // for this executor. We don't care about which calldata/executor, since we are + // only testing the functionality of the delegatecall and not the inner + // workings of the executor. + // Thus, we chose a previously-deployed Hashflow executor for simplicity. To + // change this test, we can find any of our transactions that succeeded, and + // obtain the calldata passed to the executor via Tenderly. dispatcherExposed.exposedSetSwapExecutor( address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) ); bytes memory data = hex"e592557AB9F4A75D992283fD6066312FF013ba3dbd0625ab5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593c81c"; - uint256 givenAmount = 15000000000000000000; + uint256 givenAmount = 15 ether; uint256 amount = dispatcherExposed.exposedCallSwapExecutor(givenAmount, data); assert(amount == 35144641819); @@ -88,12 +95,20 @@ contract SwapExecutionDispatcherTest is Constants { // Test case taken from existing transaction // 0x755d603962b30f416cf3eefae8d55204d6ffdf746465b2a94aca216faab63804 // No selector is passed, so the standard swap selector should be used + + // For this test, we can use any executor and any calldata that we know works + // for this executor. We don't care about which calldata/executor, since we are + // only testing the functionality of the delegatecall and not the inner + // workings of the executor. + // Thus, we chose a previously-deployed Hashflow executor for simplicity. To + // change this test, we can find any of our transactions that succeeded, and + // obtain the calldata passed to the executor via Tenderly. dispatcherExposed.exposedSetSwapExecutor( address(0xe592557AB9F4A75D992283fD6066312FF013ba3d) ); bytes memory data = hex"e592557AB9F4A75D992283fD6066312FF013ba3d000000005615dEB798BB3E4dFa0139dFa1b3D433Cc23b72fc8c39af7983bf329086de522229a7be5fc4e41cc51c72848c68a965f66fa7a88855f9f7784502a7f2606beffe61000613d6a25b5bfef4cd7652aa94777d4a46b39f2e206411280a12c9344b769ff1066c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48000000000000000000000000000000000000000000000000d02ab486cedc0000000000000000000000000000000000000000000000000000000000082ec8ad1b0000000000000000000000000000000000000000000000000000000066d7b65800000000000000000000000000000000000000000000000000000191ba9f843c125000064000640000d52de09955f0ffffffffffffff00225c389e595fe9000001fcc910754b349f821e4bb5d8444822a63920be943aba6f1b31ee14ef0fc6840b6d28d604e04a78834b668dba24a6c082ffb901e4fffa9600649e8d991af593c81c"; - uint256 givenAmount = 15000000000000000000; + uint256 givenAmount = 15 ether; uint256 amount = dispatcherExposed.exposedCallSwapExecutor(givenAmount, data); assert(amount == 35144641819); From 1fabff19c4427caee0a758e2f89336ea784462cb Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Fri, 24 Jan 2025 11:45:24 -0500 Subject: [PATCH 7/7] feat: Emit event when removing executor - This is not so relevant for security, but it would sabotage our performance if an executor was wrongly removed, so it's worth it to know every time this happens. --- foundry/src/SwapExecutionDispatcher.sol | 2 ++ foundry/test/SwapExecutionDispatcher.t.sol | 4 ++++ foundry/test/TychoRouter.t.sol | 1 - 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/foundry/src/SwapExecutionDispatcher.sol b/foundry/src/SwapExecutionDispatcher.sol index 30f335a..99f7743 100644 --- a/foundry/src/SwapExecutionDispatcher.sol +++ b/foundry/src/SwapExecutionDispatcher.sol @@ -22,6 +22,7 @@ contract SwapExecutionDispatcher { mapping(address => bool) public swapExecutors; event ExecutorSet(address indexed executor); + event ExecutorRemoved(address indexed executor); /** * @dev Adds or replaces an approved swap executor contract address if it is a @@ -42,6 +43,7 @@ contract SwapExecutionDispatcher { */ function _removeSwapExecutor(address target) internal { delete swapExecutors[target]; + emit ExecutorRemoved(target); } /** diff --git a/foundry/test/SwapExecutionDispatcher.t.sol b/foundry/test/SwapExecutionDispatcher.t.sol index 635b3a1..c313694 100644 --- a/foundry/test/SwapExecutionDispatcher.t.sol +++ b/foundry/test/SwapExecutionDispatcher.t.sol @@ -33,6 +33,7 @@ contract SwapExecutionDispatcherTest is Constants { SwapExecutionDispatcherExposed dispatcherExposed; event ExecutorSet(address indexed executor); + event ExecutorRemoved(address indexed executor); function setUp() public { uint256 forkBlock = 20673900; @@ -52,6 +53,9 @@ contract SwapExecutionDispatcherTest is Constants { function testRemoveExecutor() public { dispatcherExposed.exposedSetSwapExecutor(DUMMY); + vm.expectEmit(); + // Define the event we expect to be emitted at the next step + emit ExecutorRemoved(DUMMY); dispatcherExposed.exposedRemoveSwapExecutor(DUMMY); assert(dispatcherExposed.swapExecutors(DUMMY) == false); } diff --git a/foundry/test/TychoRouter.t.sol b/foundry/test/TychoRouter.t.sol index 2e808b7..809586f 100644 --- a/foundry/test/TychoRouter.t.sol +++ b/foundry/test/TychoRouter.t.sol @@ -14,7 +14,6 @@ contract TychoRouterTest is TychoRouterTestSetup { bytes32 public constant FUND_RESCUER_ROLE = 0x912e45d663a6f4cc1d0491d8f046e06c616f40352565ea1cdb86a0e1aaefa41b; - event ExecutorSet(address indexed executor); event CallbackVerifierSet(address indexed callbackVerifier); event Withdrawal( address indexed token, uint256 amount, address indexed receiver