From 3fb17c71da192463b0c6b15dea9a2bae47832ef5 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Thu, 24 Apr 2025 16:57:08 -0400 Subject: [PATCH 1/3] fix: Remove tload from executor - Store the executor address when deploying instead. - We would like to keep all instances of tload and tstore within the callback mechanism of our main TychoRouter contract for security reasons and to prevent any unexpected behaviour - This way it's easy to reason that UniswapV4Executor will only ever execute a delegatecall to itself. Before it could in theory execute a delegatecall to any address. One had to look at all occurences of tstore(0, x) to ensure the address was constrained. --- foundry/src/executors/UniswapV4Executor.sol | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/foundry/src/executors/UniswapV4Executor.sol b/foundry/src/executors/UniswapV4Executor.sol index aa9cfe7..790b3c1 100644 --- a/foundry/src/executors/UniswapV4Executor.sol +++ b/foundry/src/executors/UniswapV4Executor.sol @@ -44,6 +44,7 @@ contract UniswapV4Executor is using TransientStateLibrary for IPoolManager; IPoolManager public immutable poolManager; + address private immutable _self; struct UniswapV4Pool { address intermediaryToken; @@ -55,6 +56,7 @@ contract UniswapV4Executor is TokenTransfer(_permit2) { poolManager = _poolManager; + _self = address(this); } /** @@ -204,18 +206,9 @@ contract UniswapV4Executor is internal returns (bytes memory) { - address executor; - // slither-disable-next-line assembly - assembly { - executor := tload(0) - } - - if (executor == address(0)) { - executor = address(this); - } // here we expect to call either `swapExactInputSingle` or `swapExactInput`. See `swap` to see how we encode the selector and the calldata // slither-disable-next-line low-level-calls - (bool success, bytes memory returnData) = executor.delegatecall(data); + (bool success, bytes memory returnData) = _self.delegatecall(data); if (!success) { revert( string( From 4de1d104062d4aefbde22031d9f31884be5d49ad Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Fri, 25 Apr 2025 11:02:12 -0400 Subject: [PATCH 2/3] feat: Add security check for callback selector - Do not allow any callback to be chosen, for security and clarity purposes --- foundry/src/executors/UniswapV4Executor.sol | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/foundry/src/executors/UniswapV4Executor.sol b/foundry/src/executors/UniswapV4Executor.sol index 790b3c1..a09225f 100644 --- a/foundry/src/executors/UniswapV4Executor.sol +++ b/foundry/src/executors/UniswapV4Executor.sol @@ -26,6 +26,7 @@ import {TransientStateLibrary} from error UniswapV4Executor__InvalidDataLength(); error UniswapV4Executor__NotPoolManager(); +error UniswapV4Executor__UnknownCallback(bytes4 selector); error UniswapV4Executor__DeltaNotPositive(Currency currency); error UniswapV4Executor__DeltaNotNegative(Currency currency); error UniswapV4Executor__V4TooMuchRequested( @@ -46,6 +47,9 @@ contract UniswapV4Executor is IPoolManager public immutable poolManager; address private immutable _self; + bytes4 constant SWAP_EXACT_INPUT_SINGLE_SELECTOR = 0x8bc6d0d7; + bytes4 constant SWAP_EXACT_INPUT_SELECTOR = 0xaf90aeb1; + struct UniswapV4Pool { address intermediaryToken; uint24 fee; @@ -206,6 +210,14 @@ contract UniswapV4Executor is internal returns (bytes memory) { + bytes4 selector = bytes4(data[:4]); + if ( + selector != SWAP_EXACT_INPUT_SELECTOR + && selector != SWAP_EXACT_INPUT_SINGLE_SELECTOR + ) { + revert UniswapV4Executor__UnknownCallback(selector); + } + // here we expect to call either `swapExactInputSingle` or `swapExactInput`. See `swap` to see how we encode the selector and the calldata // slither-disable-next-line low-level-calls (bool success, bytes memory returnData) = _self.delegatecall(data); From 732450670f91fe7f854e5aa5cb9b36f1cbb2a204 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Fri, 25 Apr 2025 18:33:14 -0400 Subject: [PATCH 3/3] chore: remove outdated docstring --- foundry/src/executors/UniswapV4Executor.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/foundry/src/executors/UniswapV4Executor.sol b/foundry/src/executors/UniswapV4Executor.sol index a09225f..f594adc 100644 --- a/foundry/src/executors/UniswapV4Executor.sol +++ b/foundry/src/executors/UniswapV4Executor.sol @@ -203,8 +203,6 @@ contract UniswapV4Executor is /** * @dev Internal function to handle the unlock callback. - * The executor address is needed to perform the call. If the router is being used, the executor address is in - * transient storage. If it is not, then address(this) should be used. */ function _unlockCallback(bytes calldata data) internal