Liquidity.Party/tycho-execution
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor: Move contract code-checking logic to SwapExecutionDispatcher

Browse Source 
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
This commit is contained in:
TAMARA LIPOWSKI
2025-01-23 15:40:24 -05:00
parent b616e11354
commit fb9f340cb7
6 changed files with 86 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
foundry/src/SwapExecutionDispatcher.sol
View File

@@ -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.

12
foundry/src/TychoRouter.sol
View File

@@ -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);
}
/**

8
foundry/test/Constants.sol
View File

@@ -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
}
}

62
foundry/test/SwapExecutionDispatcher.t.sol
View File

@@ -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) =

31
foundry/test/TychoRouter.t.sol
View File

@@ -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();

8
foundry/test/TychoRouterTestSetup.sol
View File

@@ -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