1inch Aggregation Router & Limit Order Protocol Update Security Review Report

AggregationRouterV6 has the permissioned rescueFunds() function callable by the contact's owner. It should allow to secure users' funds when they are accidentally transferred to the router, or there is an issue during a swap. Case in point for mentioned swap issues could be awry dex2 or dex3 parameters for unoswap2() or unoswap3().

 /**
 * @notice Retrieves funds accidently sent directly to the contract address
 * @param token ERC20 token to retrieve
 * @param amount amount to retrieve
 */
 function rescueFunds(IERC20 token, uint256 amount) external onlyOwner {
 token.uniTransfer(payable(msg.sender), amount);
 }

Yet the permissionless curveSwapCallback() and uniswapV3SwapCallback() functions of UnoswapRouter exist, offering the same functionality. Anyone could call aforementioned functions to get stuck user's funds.

 function curveSwapCallback(
 address /* sender */,
 address /* receiver */,
 address inCoin,
 uint256 dx,
 uint256 /* dy */
 ) external {
 IERC20(inCoin).safeTransfer(msg.sender, dx);
 }

The uniswapV3SwapCallback() function behaves the same way as curveSwapCallback() when the payer parameter from calldata equals the address of the UnoswapRouter contract.

function uniswapV3SwapCallback(
 int256 amount0Delta,
 int256 amount1Delta,
 bytes calldata /* data */
 ) external override {
 uint256 selectors = _SELECTORS;
 assembly ("memory-safe") { // solhint-disable-line no-inline-assembly
 function reRevert() {...}
 function safeERC20(target, value, mem, memLength, outLen) {...}
 let emptyPtr := mload(0x40)
 let resultPtr := add(emptyPtr, 0x15) // 0x15 = _FF_FACTORY size
 mstore(emptyPtr, selectors)
 let amount
 let token
 switch sgt(amount0Delta, 0)
 case 1 {
 if iszero(staticcall(gas(), caller(), add(emptyPtr, _TOKEN0_SELECTOR_OFFSET), 0x4, resultPtr, 0x20)) {
 reRevert()
 }
 token := mload(resultPtr)
 amount := amount0Delta
 }
 default {
 if iszero(staticcall(gas(), caller(), add(emptyPtr, _TOKEN1_SELECTOR_OFFSET), 0x4, add(resultPtr, 0x20), 0x20)) {
 reRevert()
 }
 token := mload(add(resultPtr, 0x20))
 amount := amount1Delta
 }
 let payer := calldataload(0x84)
 let usePermit2 := calldataload(0xa4)
 switch eq(payer, address())
 case 1 {
 // IERC20(token.get()).safeTransfer(msg.sender,amount)
 mstore(add(emptyPtr, add(_TRANSFER_SELECTOR_OFFSET, 0x04)), caller())
 mstore(add(emptyPtr, add(_TRANSFER_SELECTOR_OFFSET, 0x24)), amount)
 safeERC20(token, 0, add(emptyPtr, _TRANSFER_SELECTOR_OFFSET), 0x44, 0x20)
 }
 default {...}
 }

A possible attack scenario:

1. The user calls unoswap2() for Uniswap v3 protocol with correct dex but amiss dex2 parameter.
2. The first swap uses address of the UnoswapRouter contract as the destination for Uniswap v3 protocol.
3. Since dex2 is incorrect _unoswap() might execute without reverts if the protocol byte in dex2 doesn't match any if statements.
4. Eventually, funds remain on the router contract balance.
5. A threat actor could get tokens by calling curveSwapCallback or uniswapV3SwapCallback directly.

Rescue mechanism for stuck tokens should be secure and accessible to the owner only. The _unoswap function should revert for unsupported protocols.

In the current workflow for swapping on Curve, the user's funds are first transferred to the UnoswapRouter, and further UnoswapRouter approves token amount to the Curve pool inside _curfe().

function _unoswap(
 address spender,
 address recipient,
 Address token,
 uint256 amount,
 uint256 minReturn,
 Address dex
) private returns(uint256 returnAmount) {
 ProtocolLib.Protocol protocol = dex.protocol();
 ...
 } else if (protocol == ProtocolLib.Protocol.Curve) {
 if (spender == msg.sender && msg.value == 0) {
 IERC20(token.get()).safeTransferFromUniversal(msg.sender, address(this), amount, dex.usePermit2());
 }
 returnAmount = _curfe(recipient, amount, minReturn, dex);
 }
}

Considering pool, fromToken, and amount are fully controlled by the user for asmApprove():

1. pool comes from the dex input parameter.
2. amount is an input parameter.
3. fromToken is retrieved by calling pool.coins(). Additionally, there is a mismatch between the token input parameter for _unoswap() and fromToken for asmApprove(), allowing to present a fake token for _unoswap() and a real one for asmApprove().

Therefore, it's feasible for a threat actor to make unlimited approval from UnoswapRouter to a malicious contract for any token by calling unoswap() with selected input parameters. Eventually, any tokens on the router balance could be stolen. This might happen when someone transfers tokens to the router accidentally, or there is an issue during a multi-step swap (unoswap2 or unoswap3).

function _curfe(
 address recipient,
 uint256 amount,
 uint256 minReturn,
 Address dex
) internal returns(uint256 ret) {
 ...
 if or(iszero(useEth), and(useEth, eq(toToken, _WETH))) {
 ...
 if iszero(hasCallback) {
 asmApprove(fromToken, pool, amount, mload(0x40))
 }
 }
 ...
}

To mitigate the issue, asmApprove() should use the same token address as safeTransferFromUniversal() inside _unoswap().

STATUS: Fixed

There is no need to check conditions srcToken_ == _WETH and dstToken == _WETH inside the if statement since WETH case is seamlessly handled by clipperExchange.swap(). Currently, when treating the WETH case specifically, it creates superfluous rounds of conversion between ETH and WETH.

In srcToken_ == _WETH case, WETH is converted to ETH in the begining of clipperSwapTo().

 function clipperSwapTo(
 IClipperExchange clipperExchange,
 address payable recipient,
 Address srcToken,
 IERC20 dstToken,
 uint256 inputAmount,
 uint256 outputAmount,
 uint256 goodUntil,
 bytes32 r,
 bytes32 vs
 ) public payable returns(uint256 returnAmount) {
 IERC20 srcToken_ = IERC20(srcToken.get());
 if (srcToken_ == _ETH) {
 if (msg.value != inputAmount) revert RouterErrors.InvalidMsgValue();
 } else if (srcToken_ == _WETH) { // @audit from WETH to ETH
 if (msg.value != 0) revert RouterErrors.InvalidMsgValue();
 _WETH.safeTransferFrom(msg.sender, address(this), inputAmount);
 _WETH.safeWithdraw(inputAmount);
 } else {
 if (msg.value != 0) revert RouterErrors.InvalidMsgValue();
 srcToken_.safeTransferFromUniversal(msg.sender, address(clipperExchange), inputAmount, srcToken.getFlag(_PERMIT2_FLAG));
 }
 ...
 }

Yet ClipperCaravelExchange.sellEthForToken() (https://remix.ethereum.org/address/0xe7b0ce0526fbe3969035a145c9e9691d4d9d216c) converts ETH to WETH back. So, we have two conversions, but we can have 0 if clipperExchange.swap() is used.

 function sellEthForToken(address outputToken, uint256 inputAmount, uint256 outputAmount, uint256 goodUntil, address destinationAddress, Signature calldata theSignature, bytes calldata auxiliaryData) external virtual override receivedInTime(goodUntil) payable {
 /* CHECKS */
 require(isToken(outputToken), "Clipper: Invalid token");
 // Wrap ETH (as balance or value) as input. This will revert if insufficient balance is provided
 safeEthSend(WRAPPER_CONTRACT, inputAmount); // @audit - convert from ETH to WETH
 // Revert if it's signed by the wrong address 
 bytes32 digest = createSwapDigest(WRAPPER_CONTRACT, outputToken, inputAmount, outputAmount, goodUntil, destinationAddress);
 verifyDigestSignature(digest, theSignature);
 /* EFFECTS */
 increaseBalance(WRAPPER_CONTRACT, inputAmount);
 decreaseBalance(outputToken, outputAmount);
 /* INTERACTIONS */
 IERC20(outputToken).safeTransfer(destinationAddress, outputAmount);
 emit Swapped(WRAPPER_CONTRACT, outputToken, destinationAddress, inputAmount, outputAmount, auxiliaryData);
 }

Moreover, case dstToken == _WETH requires destinationAddress pointing to the ClipperRouter contract instead of the user's address, which entails a user should request signed swap parameters from Clipper backend with different destinationAddress in such a case.

...
switch iszero(dstToken)
case 1 {
 mstore(add(ptr, 0x84), recipient)
}
default {
 mstore(add(ptr, 0x84), address())
}
...

STATUS: Fixed

The _curfe() function allows to call any address on behalf of the UnoswapRouter with an arbitrary function selector both encoded into the dex parameter.

...
// Swap
let ptr := mload(0x40)
{ // stack too deep
 let swapSelector := shl(224, and(shr(_CURVE_SWAP_SELECTOR_OFFSET, dex), _CURVE_SWAP_SELECTOR_MASK))
 mstore(ptr, swapSelector)
}
...

The issue couldn't be exploited in a straightforward way by calling transferFrom() on the ERC20 token since the first two parameters are encoded into the dex input parameter and limited to 1 byte.

...
mstore(add(ptr, 0x04), and(shr(_CURVE_FROM_TOKEN_OFFSET, dex), _CURVE_FROM_TOKEN_MASK))
mstore(add(ptr, 0x24), and(shr(_CURVE_TO_TOKEN_OFFSET, dex), _CURVE_TO_TOKEN_MASK))
...

Yet the third and forth parameters could have arbitrary values.

...
mstore(add(ptr, 0x44), amount)
mstore(add(ptr, 0x64), minReturn)
...

The recommendation is to employ a whitelist for the selector to mitigate future attacks, taking into account possible new integrations and router functionality.

STATUS: Fixed

Natspec for TakerTraitsLib doesn't describe precisely all the flags and args packed into TakerTraits.

type TakerTraits is uint256;
/**
 * @title TakerTraitsLib
 * @notice This library to manage and check TakerTraits, which are used to encode the taker's preferences for an order in a single uint256.
 * @dev The TakerTraits are structured as follows:
 * High bits are used for flags
 * 255 bit `_MAKER_AMOUNT_FLAG` - If set, the taking amount is calculated based on making amount, otherwise making amount is calculated based on taking amount.
 * 254 bit `_UNWRAP_WETH_FLAG` - If set, the WETH will be unwrapped into ETH before sending to taker.
 * 253 bit `_SKIP_ORDER_PERMIT_FLAG` - If set, the order skips maker's permit execution.
 * 252 bit `_USE_PERMIT2_FLAG` - If set, the order uses the permit2 function for authorization.
 * The remaining bits are used to store the threshold amount (the maximum amount a taker agrees to give in exchange for a making amount).
 */
library TakerTraitsLib {
 uint256 private constant _MAKER_AMOUNT_FLAG = 1 << 255;
 uint256 private constant _UNWRAP_WETH_FLAG = 1 << 254;
 uint256 private constant _SKIP_ORDER_PERMIT_FLAG = 1 << 253;
 uint256 private constant _USE_PERMIT2_FLAG = 1 << 252;
 uint256 private constant _ARGS_HAS_TARGET = 1 << 251;
 uint256 private constant _ARGS_EXTENSION_LENGTH_OFFSET = 224;
 uint256 private constant _ARGS_EXTENSION_LENGTH_MASK = 0xffffff;
 uint256 private constant _ARGS_INTERACTION_LENGTH_OFFSET = 200;
 uint256 private constant _ARGS_INTERACTION_LENGTH_MASK = 0xffffff;
 uint256 private constant _AMOUNT_MASK = 0x000000000000000000ffffffffffffffffffffffffffffffffffffffffffffff;
 ...
}

AggregationRouterV6 inherits OrderMixin functionality while only the permitAndCall() function is required for the router. We recommend decoupling the rest of OrderMixin functionality from AggregationRouterV6 since it's unused and exposes an additional attack surface for the router.

contract AggregationRouterV6 is EIP712("1inch Aggregation Router", "6"), Ownable,
 ClipperRouter, GenericRouter, UnoswapRouter, OrderMixin {
 ...
}