From fcd85c047f0de8765ba86f9041cd57ebfd030d3d Mon Sep 17 00:00:00 2001 From: Diana Carvalho Date: Fri, 16 May 2025 10:49:49 +0100 Subject: [PATCH] chore: Misc improvements: - Don't use payable(receiver).transfer(amount) and use OpenZeppelin's Address.sendValue instead - In Univ4Executor send funds to the poolManager and not msg.sender - In OneTransferFromOnly: - rename method name - don't pass the sender but hardcode it to caller() (msg.sender) - Move marking the transfer as done up (before we actually transfer) to prevent reentrancy attacks Took 18 minutes --- foundry/src/OneTransferFromOnly.sol | 17 ++++++----------- foundry/src/TychoRouter.sol | 12 ++++++------ foundry/src/executors/EkuboExecutor.sol | 7 ++----- foundry/src/executors/MaverickV2Executor.sol | 4 ++-- foundry/src/executors/UniswapV3Executor.sol | 5 +++-- foundry/src/executors/UniswapV4Executor.sol | 7 ++++--- 6 files changed, 23 insertions(+), 29 deletions(-) diff --git a/foundry/src/OneTransferFromOnly.sol b/foundry/src/OneTransferFromOnly.sol index 83731c2..32bcc70 100644 --- a/foundry/src/OneTransferFromOnly.sol +++ b/foundry/src/OneTransferFromOnly.sol @@ -43,17 +43,16 @@ contract OneTransferFromOnly { } // slither-disable-next-line assembly - function tstoreTransferFromInfo( + function _tstoreTransferFromInfo( address tokenIn, uint256 amountIn, - bool isPermit2, - address sender + bool isPermit2 ) internal { assembly { tstore(_TOKEN_IN_SLOT, tokenIn) tstore(_AMOUNT_IN_SLOT, amountIn) tstore(_IS_PERMIT2_SLOT, isPermit2) - tstore(_SENDER_SLOT, sender) + tstore(_SENDER_SLOT, caller()) tstore(_IS_TRANSFER_EXECUTED_SLOT, false) } } @@ -75,19 +74,15 @@ contract OneTransferFromOnly { if (isTransferExecuted) { revert OneTransferFromOnly__MultipleTransferFrom(); } - + assembly { + tstore(_IS_TRANSFER_EXECUTED_SLOT, true) + } if (isPermit2) { // Permit2.permit is already called from the TychoRouter permit2.transferFrom(sender, receiver, uint160(amount), tokenIn); - assembly { - tstore(_IS_TRANSFER_EXECUTED_SLOT, true) - } } else { // slither-disable-next-line arbitrary-send-erc20 IERC20(tokenIn).safeTransferFrom(sender, receiver, amount); - assembly { - tstore(_IS_TRANSFER_EXECUTED_SLOT, true) - } } } } diff --git a/foundry/src/TychoRouter.sol b/foundry/src/TychoRouter.sol index 9bfad5a..06213a2 100644 --- a/foundry/src/TychoRouter.sol +++ b/foundry/src/TychoRouter.sol @@ -140,7 +140,7 @@ contract TychoRouter is bytes calldata swaps ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) { uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver); - tstoreTransferFromInfo(tokenIn, amountIn, false, msg.sender); + _tstoreTransferFromInfo(tokenIn, amountIn, false); if (transferFromNeeded) { _transfer(address(this)); } @@ -205,7 +205,7 @@ contract TychoRouter is if (tokenIn != address(0)) { permit2.permit(msg.sender, permitSingle, signature); } - tstoreTransferFromInfo(tokenIn, amountIn, true, msg.sender); + _tstoreTransferFromInfo(tokenIn, amountIn, true); if (transferFromNeeded) { _transfer(address(this)); } @@ -262,7 +262,7 @@ contract TychoRouter is bytes calldata swaps ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) { uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver); - tstoreTransferFromInfo(tokenIn, amountIn, false, msg.sender); + _tstoreTransferFromInfo(tokenIn, amountIn, false); if (transferFromNeeded) { _transfer(tokenInReceiver); } @@ -325,7 +325,7 @@ contract TychoRouter is permit2.permit(msg.sender, permitSingle, signature); } - tstoreTransferFromInfo(tokenIn, amountIn, true, msg.sender); + _tstoreTransferFromInfo(tokenIn, amountIn, true); if (transferFromNeeded) { _transfer(tokenInReceiver); } @@ -377,7 +377,7 @@ contract TychoRouter is bytes calldata swapData ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) { uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver); - tstoreTransferFromInfo(tokenIn, amountIn, false, msg.sender); + _tstoreTransferFromInfo(tokenIn, amountIn, false); if (transferFromNeeded) { _transfer(tokenInReceiver); } @@ -439,7 +439,7 @@ contract TychoRouter is if (tokenIn != address(0)) { permit2.permit(msg.sender, permitSingle, signature); } - tstoreTransferFromInfo(tokenIn, amountIn, true, msg.sender); + _tstoreTransferFromInfo(tokenIn, amountIn, true); if (transferFromNeeded) { _transfer(tokenInReceiver); } diff --git a/foundry/src/executors/EkuboExecutor.sol b/foundry/src/executors/EkuboExecutor.sol index 90ee2f5..6c556f6 100644 --- a/foundry/src/executors/EkuboExecutor.sol +++ b/foundry/src/executors/EkuboExecutor.sol @@ -12,6 +12,7 @@ import {LibBytes} from "@solady/utils/LibBytes.sol"; import {Config, EkuboPoolKey} from "@ekubo/types/poolKey.sol"; import {MAX_SQRT_RATIO, MIN_SQRT_RATIO} from "@ekubo/types/sqrtRatio.sol"; import "../OneTransferFromOnly.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; contract EkuboExecutor is IExecutor, @@ -204,11 +205,7 @@ contract EkuboExecutor is if (transferFromNeeded) { _transfer(msg.sender); } else if (transferNeeded) { - if (token == address(0)) { - payable(msg.sender).transfer(amount); - } else { - IERC20(token).safeTransfer(msg.sender, amount); - } + IERC20(token).safeTransfer(msg.sender, amount); } } diff --git a/foundry/src/executors/MaverickV2Executor.sol b/foundry/src/executors/MaverickV2Executor.sol index f8acbb1..fab7dc8 100644 --- a/foundry/src/executors/MaverickV2Executor.sol +++ b/foundry/src/executors/MaverickV2Executor.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.26; import "@interfaces/IExecutor.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; error MaverickV2Executor__InvalidDataLength(); error MaverickV2Executor__InvalidTarget(); @@ -48,8 +49,7 @@ contract MaverickV2Executor is IExecutor { if (transferNeeded) { if (address(tokenIn) == address(0)) { - // slither-disable-next-line arbitrary-send-eth - payable(target).transfer(givenAmount); + Address.sendValue(payable(target), givenAmount); } else { // slither-disable-next-line arbitrary-send-erc20 tokenIn.safeTransfer(target, givenAmount); diff --git a/foundry/src/executors/UniswapV3Executor.sol b/foundry/src/executors/UniswapV3Executor.sol index 4f0a68a..a8a5d0f 100644 --- a/foundry/src/executors/UniswapV3Executor.sol +++ b/foundry/src/executors/UniswapV3Executor.sol @@ -6,6 +6,7 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol"; import "@interfaces/ICallback.sol"; import {OneTransferFromOnly} from "../OneTransferFromOnly.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; error UniswapV3Executor__InvalidDataLength(); error UniswapV3Executor__InvalidFactory(); @@ -111,7 +112,7 @@ contract UniswapV3Executor is IExecutor, ICallback, OneTransferFromOnly { _transfer(msg.sender); } else if (transferNeeded) { if (tokenIn == address(0)) { - payable(msg.sender).transfer(amountOwed); + Address.sendValue(payable(msg.sender), amountOwed); } else { IERC20(tokenIn).safeTransfer(msg.sender, amountOwed); } @@ -169,7 +170,7 @@ contract UniswapV3Executor is IExecutor, ICallback, OneTransferFromOnly { uint24 fee, bool transferFromNeeded, bool transferNeeded - ) internal view returns (bytes memory) { + ) internal pure returns (bytes memory) { return abi.encodePacked( tokenIn, tokenOut, fee, transferFromNeeded, transferNeeded ); diff --git a/foundry/src/executors/UniswapV4Executor.sol b/foundry/src/executors/UniswapV4Executor.sol index 7b30926..52a1910 100644 --- a/foundry/src/executors/UniswapV4Executor.sol +++ b/foundry/src/executors/UniswapV4Executor.sol @@ -23,6 +23,7 @@ import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol"; import "../OneTransferFromOnly.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; error UniswapV4Executor__InvalidDataLength(); error UniswapV4Executor__NotPoolManager(); @@ -410,14 +411,14 @@ contract UniswapV4Executor is } else { if (transferFromNeeded) { // transferFrom swapper's wallet into the core contract - _transfer(msg.sender); + _transfer(address(poolManager)); } else if (transferNeeded) { address tokenIn = Currency.unwrap(currency); // transfer from router contract into the core contract if (tokenIn == address(0)) { - payable(msg.sender).transfer(amount); + Address.sendValue(payable(address(poolManager)), amount); } else { - IERC20(tokenIn).safeTransfer(msg.sender, amount); + IERC20(tokenIn).safeTransfer(address(poolManager), amount); } } // slither-disable-next-line unused-return