code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/Router.sol#L132-L140 https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/Router.sol#L122

Vulnerability details

Impact

If user calls Router.exactInputSingle swap with params.recipient == 0 by mistake, all swapping funds will be sent to Router and anyone can claim them to himself.

Proof of Concept

Router.exactInputSingle function calls exactInputInternal function and pases recipient param. https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/Router.sol#L121-L129

    function exactInputInternal(uint256 amountIn, address recipient, uint256 sqrtPriceLimitD18, SwapCallbackData memory data) private returns (uint256 amountOut) {
        if (recipient == address(0)) recipient = address(this);

        (IERC20 tokenIn, IERC20 tokenOut, IPool pool) = data.path.decodeFirstPool();

        bool tokenAIn = tokenIn < tokenOut;

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

As you can see if recipient is 0, then it is set to address(this). That means that receiver of trade will be Router. Router contract has several methods that allow anyone to transfer controlled by Router tokens to himself, like refundETH, sweepToken, unwrapWETH9. https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/Router.sol#L70-L77

    function sweepToken(IERC20 token, uint256 amountMinimum, address recipient) public payable {
        uint256 balanceToken = token.balanceOf(address(this));
        require(balanceToken >= amountMinimum, "Insufficient token");

        if (balanceToken > 0) {
            TransferHelper.safeTransfer(address(token), recipient, balanceToken);
        }
    }

No restrictions here, so anyone can call.

This creates possiblity that after user will make exactInputSingle swap with recipient 0 address and in another tx will try to sweep those tokens, someone will frontrun user and will send tokens to himself. User will lost swapped funds.

This test will show that if recipient is 0 then after the swap Router controls swapped tokens. You can run it inside Router.ts.

it.only("swap recipient 0", async () => {
      let _fee = 0.01 / 100;
      let pool: string = await getPool(_fee, tokenA, tokenB, 500, 500);
      const prePoolBalance = await balances(pool);
      const preRouterBalance = await balances(router.address);
      //Router balance of b token is 0
      expect(preRouterBalance.tokenB).to.be.eq(0);
      console.log(preRouterBalance);
      expect(
        await router.exactInputSingle([
          tokenA.address,
          tokenB.address,
          pool,
          ethers.constants.AddressZero,
          maxDeadline,
          floatToFixed(10),
          0,
          0,
        ])
      )
        .to.emit(tokenA, "Transfer")
        .to.emit(tokenB, "Transfer");
      const postPoolBalance = await balances(pool);
      expect(postPoolBalance.tokenA - prePoolBalance.tokenA).to.eq(10);
      expect(postPoolBalance.tokenB - prePoolBalance.tokenB).to.be.lessThan(-4);

      const postRouterBalance = await balances(router.address);
      console.log(postRouterBalance);
      //Router balance of b token is not 0 anymore
      //now anyone can sweep it
      expect(postRouterBalance.tokenB).to.be.not.eq(0);
    });

Tools Used

VsCode

Recommended Mitigation Steps

Add check to exactInputSingle and exactOutputSingle that params.recipient is not 0.

Jeiwan commented 1 year ago

This is an expected behavior. It makes swaps calldata portable and Router-agnostic, which allows to seamlessly migrate to a new Router. This was initially introduced in Uniswap V3: https://github.com/Uniswap/v3-periphery/commit/a0e0e5817528f0b810583c04feea17b696a16755

It also gives a chance to withdraw tokens thanks to the public sweepTokens function: https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/Router.sol#L70 Otherwise, tokens would've been sent to the zero address and lost forever.

kirk-baird commented 1 year ago

This is clearly a design feature to use recipient = address(0) to have funds transferred to the Router. I'm going to mark this as a QA.

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)