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
This commit is contained in:
Diana Carvalho
2025-05-16 10:49:49 +01:00
parent 99d5df4f77
commit fcd85c047f
6 changed files with 23 additions and 29 deletions

View File

@@ -43,17 +43,16 @@ contract OneTransferFromOnly {
} }
// slither-disable-next-line assembly // slither-disable-next-line assembly
function tstoreTransferFromInfo( function _tstoreTransferFromInfo(
address tokenIn, address tokenIn,
uint256 amountIn, uint256 amountIn,
bool isPermit2, bool isPermit2
address sender
) internal { ) internal {
assembly { assembly {
tstore(_TOKEN_IN_SLOT, tokenIn) tstore(_TOKEN_IN_SLOT, tokenIn)
tstore(_AMOUNT_IN_SLOT, amountIn) tstore(_AMOUNT_IN_SLOT, amountIn)
tstore(_IS_PERMIT2_SLOT, isPermit2) tstore(_IS_PERMIT2_SLOT, isPermit2)
tstore(_SENDER_SLOT, sender) tstore(_SENDER_SLOT, caller())
tstore(_IS_TRANSFER_EXECUTED_SLOT, false) tstore(_IS_TRANSFER_EXECUTED_SLOT, false)
} }
} }
@@ -75,19 +74,15 @@ contract OneTransferFromOnly {
if (isTransferExecuted) { if (isTransferExecuted) {
revert OneTransferFromOnly__MultipleTransferFrom(); revert OneTransferFromOnly__MultipleTransferFrom();
} }
assembly {
tstore(_IS_TRANSFER_EXECUTED_SLOT, true)
}
if (isPermit2) { if (isPermit2) {
// Permit2.permit is already called from the TychoRouter // Permit2.permit is already called from the TychoRouter
permit2.transferFrom(sender, receiver, uint160(amount), tokenIn); permit2.transferFrom(sender, receiver, uint160(amount), tokenIn);
assembly {
tstore(_IS_TRANSFER_EXECUTED_SLOT, true)
}
} else { } else {
// slither-disable-next-line arbitrary-send-erc20 // slither-disable-next-line arbitrary-send-erc20
IERC20(tokenIn).safeTransferFrom(sender, receiver, amount); IERC20(tokenIn).safeTransferFrom(sender, receiver, amount);
assembly {
tstore(_IS_TRANSFER_EXECUTED_SLOT, true)
}
} }
} }
} }

View File

@@ -140,7 +140,7 @@ contract TychoRouter is
bytes calldata swaps bytes calldata swaps
) public payable whenNotPaused nonReentrant returns (uint256 amountOut) { ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) {
uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver); uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver);
tstoreTransferFromInfo(tokenIn, amountIn, false, msg.sender); _tstoreTransferFromInfo(tokenIn, amountIn, false);
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(address(this)); _transfer(address(this));
} }
@@ -205,7 +205,7 @@ contract TychoRouter is
if (tokenIn != address(0)) { if (tokenIn != address(0)) {
permit2.permit(msg.sender, permitSingle, signature); permit2.permit(msg.sender, permitSingle, signature);
} }
tstoreTransferFromInfo(tokenIn, amountIn, true, msg.sender); _tstoreTransferFromInfo(tokenIn, amountIn, true);
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(address(this)); _transfer(address(this));
} }
@@ -262,7 +262,7 @@ contract TychoRouter is
bytes calldata swaps bytes calldata swaps
) public payable whenNotPaused nonReentrant returns (uint256 amountOut) { ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) {
uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver); uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver);
tstoreTransferFromInfo(tokenIn, amountIn, false, msg.sender); _tstoreTransferFromInfo(tokenIn, amountIn, false);
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(tokenInReceiver); _transfer(tokenInReceiver);
} }
@@ -325,7 +325,7 @@ contract TychoRouter is
permit2.permit(msg.sender, permitSingle, signature); permit2.permit(msg.sender, permitSingle, signature);
} }
tstoreTransferFromInfo(tokenIn, amountIn, true, msg.sender); _tstoreTransferFromInfo(tokenIn, amountIn, true);
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(tokenInReceiver); _transfer(tokenInReceiver);
} }
@@ -377,7 +377,7 @@ contract TychoRouter is
bytes calldata swapData bytes calldata swapData
) public payable whenNotPaused nonReentrant returns (uint256 amountOut) { ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) {
uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver); uint256 initialBalanceTokenOut = _balanceOf(tokenOut, receiver);
tstoreTransferFromInfo(tokenIn, amountIn, false, msg.sender); _tstoreTransferFromInfo(tokenIn, amountIn, false);
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(tokenInReceiver); _transfer(tokenInReceiver);
} }
@@ -439,7 +439,7 @@ contract TychoRouter is
if (tokenIn != address(0)) { if (tokenIn != address(0)) {
permit2.permit(msg.sender, permitSingle, signature); permit2.permit(msg.sender, permitSingle, signature);
} }
tstoreTransferFromInfo(tokenIn, amountIn, true, msg.sender); _tstoreTransferFromInfo(tokenIn, amountIn, true);
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(tokenInReceiver); _transfer(tokenInReceiver);
} }

View File

@@ -12,6 +12,7 @@ import {LibBytes} from "@solady/utils/LibBytes.sol";
import {Config, EkuboPoolKey} from "@ekubo/types/poolKey.sol"; import {Config, EkuboPoolKey} from "@ekubo/types/poolKey.sol";
import {MAX_SQRT_RATIO, MIN_SQRT_RATIO} from "@ekubo/types/sqrtRatio.sol"; import {MAX_SQRT_RATIO, MIN_SQRT_RATIO} from "@ekubo/types/sqrtRatio.sol";
import "../OneTransferFromOnly.sol"; import "../OneTransferFromOnly.sol";
import "@openzeppelin/contracts/utils/Address.sol";
contract EkuboExecutor is contract EkuboExecutor is
IExecutor, IExecutor,
@@ -204,11 +205,7 @@ contract EkuboExecutor is
if (transferFromNeeded) { if (transferFromNeeded) {
_transfer(msg.sender); _transfer(msg.sender);
} else if (transferNeeded) { } else if (transferNeeded) {
if (token == address(0)) { IERC20(token).safeTransfer(msg.sender, amount);
payable(msg.sender).transfer(amount);
} else {
IERC20(token).safeTransfer(msg.sender, amount);
}
} }
} }

View File

@@ -3,6 +3,7 @@ pragma solidity ^0.8.26;
import "@interfaces/IExecutor.sol"; import "@interfaces/IExecutor.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/utils/Address.sol";
error MaverickV2Executor__InvalidDataLength(); error MaverickV2Executor__InvalidDataLength();
error MaverickV2Executor__InvalidTarget(); error MaverickV2Executor__InvalidTarget();
@@ -48,8 +49,7 @@ contract MaverickV2Executor is IExecutor {
if (transferNeeded) { if (transferNeeded) {
if (address(tokenIn) == address(0)) { if (address(tokenIn) == address(0)) {
// slither-disable-next-line arbitrary-send-eth Address.sendValue(payable(target), givenAmount);
payable(target).transfer(givenAmount);
} else { } else {
// slither-disable-next-line arbitrary-send-erc20 // slither-disable-next-line arbitrary-send-erc20
tokenIn.safeTransfer(target, givenAmount); tokenIn.safeTransfer(target, givenAmount);

View File

@@ -6,6 +6,7 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol"; import "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol";
import "@interfaces/ICallback.sol"; import "@interfaces/ICallback.sol";
import {OneTransferFromOnly} from "../OneTransferFromOnly.sol"; import {OneTransferFromOnly} from "../OneTransferFromOnly.sol";
import "@openzeppelin/contracts/utils/Address.sol";
error UniswapV3Executor__InvalidDataLength(); error UniswapV3Executor__InvalidDataLength();
error UniswapV3Executor__InvalidFactory(); error UniswapV3Executor__InvalidFactory();
@@ -111,7 +112,7 @@ contract UniswapV3Executor is IExecutor, ICallback, OneTransferFromOnly {
_transfer(msg.sender); _transfer(msg.sender);
} else if (transferNeeded) { } else if (transferNeeded) {
if (tokenIn == address(0)) { if (tokenIn == address(0)) {
payable(msg.sender).transfer(amountOwed); Address.sendValue(payable(msg.sender), amountOwed);
} else { } else {
IERC20(tokenIn).safeTransfer(msg.sender, amountOwed); IERC20(tokenIn).safeTransfer(msg.sender, amountOwed);
} }
@@ -169,7 +170,7 @@ contract UniswapV3Executor is IExecutor, ICallback, OneTransferFromOnly {
uint24 fee, uint24 fee,
bool transferFromNeeded, bool transferFromNeeded,
bool transferNeeded bool transferNeeded
) internal view returns (bytes memory) { ) internal pure returns (bytes memory) {
return abi.encodePacked( return abi.encodePacked(
tokenIn, tokenOut, fee, transferFromNeeded, transferNeeded tokenIn, tokenOut, fee, transferFromNeeded, transferNeeded
); );

View File

@@ -23,6 +23,7 @@ import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol";
import {TransientStateLibrary} from import {TransientStateLibrary} from
"@uniswap/v4-core/src/libraries/TransientStateLibrary.sol"; "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol";
import "../OneTransferFromOnly.sol"; import "../OneTransferFromOnly.sol";
import "@openzeppelin/contracts/utils/Address.sol";
error UniswapV4Executor__InvalidDataLength(); error UniswapV4Executor__InvalidDataLength();
error UniswapV4Executor__NotPoolManager(); error UniswapV4Executor__NotPoolManager();
@@ -410,14 +411,14 @@ contract UniswapV4Executor is
} else { } else {
if (transferFromNeeded) { if (transferFromNeeded) {
// transferFrom swapper's wallet into the core contract // transferFrom swapper's wallet into the core contract
_transfer(msg.sender); _transfer(address(poolManager));
} else if (transferNeeded) { } else if (transferNeeded) {
address tokenIn = Currency.unwrap(currency); address tokenIn = Currency.unwrap(currency);
// transfer from router contract into the core contract // transfer from router contract into the core contract
if (tokenIn == address(0)) { if (tokenIn == address(0)) {
payable(msg.sender).transfer(amount); Address.sendValue(payable(address(poolManager)), amount);
} else { } else {
IERC20(tokenIn).safeTransfer(msg.sender, amount); IERC20(tokenIn).safeTransfer(address(poolManager), amount);
} }
} }
// slither-disable-next-line unused-return // slither-disable-next-line unused-return