1inch Aggregation Router & Limit Order Protocol Security Review Report

STATUS: Fixed

The function _curfe is called from _unoswap to facilitate a swap through a Curve pool.

Basically the function will call the right coins function on the Curve pool with the provided indices to get the from and to tokens. Then the function calls ERC20.approve on the from token with the pool and the specified amount.

However, this mechanism is not compatible with USDT, which reverts on an ERC20.approve if there already exists a non-zero allowance from the sender (the router) to the spender (the pool).

Therefore if such an allowance already exists before the swap, then any subsequent swap using USDT through the Curve swap function will revert on USDT.approve and fail.

Thanks to the customisability of the Unoswap Router it becomes possible for an attacker to create such a 'hanging' allowance (that will still exist after the swap). For example:

1. The attacker calls unoswap with USDT as token, 1 wei as value, 0 wei as minimum return amount and a Curve pool with USDT as DEX (e.g. DAI/USDC/USDT pool).
2. The DEX address would have the following encoded:
   • coins(uint256) as coins selector
   • 2 as from token (USDT)
   • 1 as to token (USDC)
   • HAVE_ARG_DESTINATION as true
   • coins(uint256) as swap selector
3. The router would now call ERC20.approve on USDT for the pool for 1 wei.
4. The call for the swap will actually use the coins(uint256) selector (the selector is fully controllable by the caller) and as a result nothing is swapped (and the allowance is not used) but the call does succeed.
5. Then the HAVE_ARG_DESINATION flag allows for skipping the block on line 467 so that the router doesn't try to send the fake amount back to the attacker and revert. Afterwards the call succeeds and the router has an existing allowance of 1 wei for the Curve pool. The attacker can then repeat this for every Curve pool with USDT.

Any subsequent swaps from other users would fail for USDT on any Curve pool.

if or(iszero(useEth), and(useEth, eq(toToken, _WETH))) {
 let fromSelectorOffset := and(shr(_CURVE_FROM_COINS_SELECTOR_OFFSET, dex), _CURVE_FROM_COINS_SELECTOR_MASK)
 let fromTokenIndex := and(shr(_CURVE_FROM_COINS_ARG_OFFSET, dex), _CURVE_FROM_COINS_ARG_MASK)
 let fromToken := curveCoins(pool, fromSelectorOffset, fromTokenIndex)
 if eq(fromToken, 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee) {
 fromToken := _WETH
 }
 // fromToken.approve(pool, amount)
 let ptr := mload(0x40)
 mstore(ptr, 0x095ea7b300000000000000000000000000000000000000000000000000000000) // IERC20.approve.selector
 mstore(add(ptr, 0x04), pool)
 mstore(add(ptr, 0x24), amount)
 safeERC20(fromToken, 0, ptr, 0x44)
}

Clear the approval of the from token for the pool after the swap.

When filling an order, the taker provides TakerTraits which includes a threshold. This threshold is used to calculate an acceptable ratio between the maker amount and the taker amount.

This is especially important because the maker or taker amount can be calculated on the spot through calculateMakingAmount or calculateTakingAmount, which can make a static call to a contract specified by the maker.

If the threshold is zero (which is the default), then this check is skipped. This allows the maker to change the maker or taker amount, such that the taker pays the full amount, while the maker only transfers an insignificant amount. This is effectively stealing.

Simulating the transaction for the taker also does not offer full protection (e.g. through eth_call). The maker has ways to bypass this, such as having the contract that calculates the amounts return a normal amount if it's a simulation and an almost zero amount if it is a real transaction.

if (takerTraits.isMakingAmount()) {
 makingAmount = Math.min(amount, remainingMakingAmount);
 takingAmount = order.calculateTakingAmount(extension, makingAmount, remainingMakingAmount, orderHash);
 
 uint256 threshold = takerTraits.threshold();
 if (threshold > 0) {
 // Check rate: takingAmount / makingAmount <= threshold / amount
 if (amount == makingAmount) { // Gas optimization, no SafeMath.mul()
 if (takingAmount > threshold) revert TakingAmountTooHigh();
 } else {
 if (takingAmount * amount > threshold * makingAmount) revert TakingAmountTooHigh();
 }
 }
}
else {
 takingAmount = amount;
 makingAmount = order.calculateMakingAmount(extension, takingAmount, remainingMakingAmount, orderHash);
 if (makingAmount > remainingMakingAmount) {
 // Try to decrease taking amount because computed making amount exceeds remaining amount
 makingAmount = remainingMakingAmount;
 takingAmount = order.calculateTakingAmount(extension, makingAmount, remainingMakingAmount, orderHash);
 if (takingAmount > amount) revert TakingAmountExceeded();
 }
 uint256 threshold = takerTraits.threshold();
 if (threshold > 0) {
 // Check rate: makingAmount / takingAmount >= threshold / amount
 if (amount == takingAmount) { // Gas optimization, no SafeMath.mul()
 if (makingAmount < threshold) revert MakingAmountTooLow();
 } else {
 if (makingAmount * amount < threshold * takingAmount) revert MakingAmountTooLow();
 }
 }
}

Either make threshold mandatory (e.g. revert if it's equal to zero) or to use a default threshold if it's equal to zero (e.g. 1.05 times the specified amount at the initial rate as defined by the maker).

There are multiple locations in the OrderMixin.sol where an external call is made to a contract controlled by the maker when the taker wants to fulfil an order:

1. In _applyMakerPermit on line 441 a permit call is made to an arbitrary address.
2. In _fillOrderTo on line 266 the predicate is checked which can make an arbitrary static call.
3. In _fillOrderTo on line 273, line 287 and line 291 a static call is made to an arbitrary address.
4. In _fillOrderTo on line 323 and line 420 an interaction call is made to an arbitrary address. Each of these calls do not enforce a gas limit and the maker can therefore spend all of the taker's gas in one of these calls, effectively gas griefing the taker.

Transaction simulation (e.g. through eth_estimateGas and eth_call) does not protect from this type of attack. A contract is able to know whether execution is a simulation or a real transaction and act accordingly to hide malicious behaviour with regards to gas griefing.

if (order.makerTraits.needPreInteractionCall()) {
 bytes calldata data = extension.preInteractionTargetAndData();
 address listener = order.maker.get();
 if (data.length > 0) {
 listener = address(bytes20(data));
 data = data[20:];
 }
 IPreInteraction(listener).preInteraction(
 order, orderHash, msg.sender, makingAmount, takingAmount, remainingMakingAmount, data
 );
}
[...]
if (order.makerTraits.needPreInteractionCall()) {
 bytes calldata data = extension.preInteractionTargetAndData();
 address listener = order.maker.get();
 if (data.length > 0) {
 listener = address(bytes20(data));
 data = data[20:];
 }
 IPreInteraction(listener).preInteraction(
 order, orderHash, msg.sender, makingAmount, takingAmount, remainingMakingAmount, data
 );
}

Add some gas limits for external calls to any address controlled by the maker.

We found that the limit order type hash in in the OrderLib library is incorrectly generated.

It uses address for the maker, makerAsset and takerAsset fields. However, these fields are actually of type Address, which is uint256 as defined in AddressLib.sol.

bytes32 constant internal _LIMIT_ORDER_TYPEHASH = keccak256(
 "Order("
 "uint256 salt,"
 "address maker,"
 "address makerAsset,"
 "address takerAsset,"
 "uint256 makingAmount,"
 "uint256 takingAmount,"
 "uint256 makerTraits"
 ")"
);

Change the type address to uint256 for the _LIMIT_ORDER_TYPEHASH.

The OrderLib allows for dynamically getting the taker or maker amount from a maker-defined address. The data is encoded in the extension.

However, the function only checks whether the length is at least 20 bytes for the target of the call and as a result, the selector could potentially be missing. In that case, the first 4 bytes of the requestedAmount would become the selector and the last 4 bytes would be shifted into the next parameter.

function _callGetter(
 bytes calldata getter,
 uint256 requestedAmount,
 uint256 remainingMakingAmount,
 bytes32 orderHash
) private view returns(uint256) {
 if (getter.length < 20) revert WrongGetter();
 (bool success, bytes memory result) = address(bytes20(getter)).staticcall(abi.encodePacked(getter[20:], requestedAmount, remainingMakingAmount, orderHash));
 if (!success || result.length != 32) revert GetAmountCallFailed();
 return abi.decode(result, (uint256));
}

change line 85 in OrderLib.sol to:

if (getter.length < 24) revert WrongGetter();

STATUS: Fixed

In _fillOrderTo on line 263, the order's maker traits are checked for an extension and only if it exist, will it perform the predicate check.

This check can be optimised by directly checking extension, because it was already validated whether there should be an extension in fillOrderToExt and fillContractOrderExt with order.validateExtension(extension). In other words, if the extension is not empty, then makerTraits.hasExtension() is true.

The check can be optimised to:

if (extension.length != 0) {
 bytes calldata predicate = extension.predicate();
 if (predicate.length > 0) {
 if (!checkPredicate(predicate)) revert PredicateIsNotTrue();
 }
}

The check is already redundant, because extension.predicate() would return an empty predicate if extension is also empty. But it is more gas efficient to first check extension if not having a predicate is the more frequent case.

if (order.makerTraits.hasExtension()) {
 bytes calldata predicate = extension.predicate();
 if (predicate.length > 0) {
 if (!checkPredicate(predicate)) revert PredicateIsNotTrue();
 }
}

STATUS: Fixed

In _fillOrderTo on line 306, both the maker and taker amount are checked against zero.

We found that this check could be optimised to save gas on each call:

if (makingAmount & takingAmount == 0) revert SwapWithZeroAmount();

if (makingAmount == 0 || takingAmount == 0) revert SwapWithZeroAmount();

The OrderMixin contract defines a private immutable variable for WETH. However, it also inherits from OnlyWethReceiver which also defines a private immutable variable for WETH, which gets set through the constructor.

The same applies to limit-order-protocol/helpers/WethUnwrapper.sol and limit-order-protocol/helpers/ETHOrders.sol.

By changing the _WETH variable in OnlyWethReceiver to internal, removing the private immutable variable _WETH from OrderMixin, WethUnwrapper and ETHOrders, and changing address(_WETH) to _WETH and _WETH. to IWETH(_WETH). almost 100k gas can be saved for deployment.

IWETH private immutable _WETH;
constructor(IWETH weth) OnlyWethReceiver(address(weth)) {
 _WETH = weth;
}

We have identified incorrect documentation in the code:

1. solidity-utils/contracts/libraries/SafeERC20.sol the documentation string functions safeTransferFromUniversal, safeTransferFrom and safeTransfer on line 24, 39 and 94 respectively it states that the function allows for calling to non-smart-contracts addresses. However this is false, as the code size of the target is checked to be greater than 0 upon success of the call and if so, it returns false.
2. limit-order-protocol/contracts/LimitOrderProtocol.sol on line 9 the version should be 4 instead of 3, so it is in accordance with the parameter values in the EIP712 constructor.

/// @dev Ensures method do not revert or return boolean `true`, admits call to non-smart-contract.
* @title ##1inch Limit Order Protocol v3

Fix the incorrect comments.

We have identified the following locations where checks for the zero address are missing:

1. solidity-utils/OnlyWethReceiver.sol: There is no check for the parameter weth in the constructor.

constructor(address weth) {
 _WETH = address(weth);
}

Check each mentioned parameter for address zero.

The event OrderFilled is emitted when a part of or the full order is fulfilled. It takes the hash and the making amount that was fulfilled in the order.

Because orders can be filled partially and so multiple times per hash, we would recommend to add indexed to the orderHash field, to allow searching and aggregating of amounts for each order.

event OrderFilled(
 bytes32 orderHash,
 uint256 makingAmount
);

STATUS: Fixed

When fulfilling an order, the signature is checked against the provided order's hash and the signer should be equal to the order's maker. The library that is used is solidity-utils/libraries/ECDSA.sol and the function called is recover(bytes32, bytes32, bytes32), which uses the built-in ecrecover.

This function does not check whether the return value of ecrecover is address(0), which happens when the signature is invalid. As a result, it is possible to create and 'sign' orders for the zero address.

The orders can still be executed with a custom maker asset, so it is possible to set order bits or remaining amounts for the zero address.

This could potentially cause problems on off-chain or front-end applications.

function fillOrderToExt(
 IOrderMixin.Order calldata order,
 bytes32 r,
 bytes32 vs,
 uint256 amount,
 TakerTraits takerTraits,
 address target,
 bytes calldata interaction,
 bytes calldata extension
) public payable returns(uint256 makingAmount, uint256 takingAmount, bytes32 orderHash) {
 order.validateExtension(extension);
 orderHash = order.hash(_domainSeparatorV4());
 // Check signature and apply order permit only on the first fill
 uint256 remainingMakingAmount = _checkRemainingMakingAmount(order, orderHash);
 if (remainingMakingAmount == order.makingAmount) {
 if (order.maker.get() != ECDSA.recover(orderHash, r, vs)) revert BadSignature();
 if (!takerTraits.skipMakerPermit()) {
 _applyMakerPermit(order, orderHash, extension);
 }
 }
 (makingAmount, takingAmount) = _fillOrderTo(order, orderHash, extension, remainingMakingAmount, amount, takerTraits, target, _wrap(interaction));
}

function recover(
 bytes32 hash,
 bytes32 r,
 bytes32 vs
) internal view returns (address signer) {
 assembly ("memory-safe") { // solhint-disable-line no-inline-assembly
 let s := and(vs, _COMPACT_S_MASK)
 if lt(s, _S_BOUNDARY) {
 let ptr := mload(0x40)
 mstore(ptr, hash)
 mstore(add(ptr, 0x20), add(27, shr(_COMPACT_V_SHIFT, vs)))
 mstore(add(ptr, 0x40), r)
 mstore(add(ptr, 0x60), s)
 mstore(0, 0)
 pop(staticcall(gas(), 0x1, ptr, 0x80, 0, 0x20))
 signer := mload(0)
 }
 }
}

Check that maker's address is not address(0).

We found that there are underflows possible in the function _curfe that executes a swap on a Curve pool. More specifically:

1. On line 457 the returned swap amount is calculated from the contract's ETH balance minus 1. If the swap returned 0, then this will underflow to maximum uint256.
2. On line 460 and then line 363 the returned swap amount is calculated using ERC20.balanceOf minus 1. Again, it the swap returned 0, then this will also underflow to maximum uint256. If due to some configuration or other factors a swap would return 0, then the router would note a swap amount of maximum uint256 and the minReturn check would pass. Instead, the router would revert trying to send maximum uint256 back to the user.

This will lead to incorrect error messages and a worse user experience.

function tokenBalanceOf(tokenAddress, accountAddress) -> tokenBalance {
 mstore(0, 0x70a0823100000000000000000000000000000000000000000000000000000000)
 mstore(4, accountAddress)
 if iszero(callReturnSize(staticcall(gas(), tokenAddress, 0, 0x24, 0, 0x20))) {
 revert(0, 0)
 }
 tokenBalance := sub(mload(0), 1) // Keep 1 wei
}
[...]
switch and(useEth, eq(toToken, _WETH))
case true {
 ret := sub(balance(address()), 1) // Keep 1 wei
}
default {
 ret := tokenBalanceOf(toToken, address())
}

Considering reverting if the returned swap amount is 0 (before subtracting the 1, or maximum uint256 after subtracting).

STATUS: Fixed

The function isAllowedSender is used to check the taker's address against the allowed sender mask in the Maker Traits.

We found that this function contains a number of redundant conversions and masks and could be simplified to:

function isAllowedSender(MakerTraits makerTraits, address sender) internal pure returns (bool) {
 uint160 allowedSender = uint160(MakerTraits.unwrap(makerTraits) & _ALLOWED_SENDER_MASK);
 return allowedSender == 0 || allowedSender == uint160(sender) & _ALLOWED_SENDER_MASK;
}

function isAllowedSender(MakerTraits makerTraits, address sender) internal pure returns (bool) {
 address allowedSender = address(uint160(MakerTraits.unwrap(makerTraits) & _ALLOWED_SENDER_MASK));
 return allowedSender == address(0) || (uint160(allowedSender) & _ALLOWED_SENDER_MASK) == uint160(sender) & _ALLOWED_SENDER_MASK;
}