Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L304-L389 https://github.com/code-423n4/2024-06-thorchain/blob/0e02d3c221d7e546a73f25183e29500eaa63f4cf/chain/ethereum/contracts/THORChain_Aggregator.sol#L144-L216

Vulnerability details

Impact

According to this protocol's token whitelist, this protocol intends to support fee-on-transfer tokens, such as STA. As shown by STA's transfer and cut functions below, _balances[to] would be increased by a value that is less than the token amount that is intended to be transferred.

https://etherscan.io/address/0xa7de087329bfcda5639247f96140f9dabe3deed1#code#L115

  function transfer(address to, uint256 value) public returns (bool) {
    ...
    uint256 tokensToBurn = cut(value);
    uint256 tokensToTransfer = value.sub(tokensToBurn);

    _balances[msg.sender] = _balances[msg.sender].sub(value);
    _balances[to] = _balances[to].add(tokensToTransfer);

    _totalSupply = _totalSupply.sub(tokensToBurn);
    ...
    return true;
  }

https://etherscan.io/address/0xa7de087329bfcda5639247f96140f9dabe3deed1#code#L109

  function cut(uint256 value) public view returns (uint256)  {
    uint256 roundValue = value.ceil(basePercent);
    uint256 cutValue = roundValue.mul(basePercent).div(10000);
    return cutValue;
  }

For such token, when calling the following _transferOutAndCallV5 function, executing aggregationPayload.fromAsset.call(abi.encodeWithSignature("transfer(address,uint256)", aggregationPayload.target, aggregationPayload.fromAmount) would transfer a token amount that is less than aggregationPayload.fromAmount to the aggregator. Yet, when calling the aggregator's swapOutV5 function below, aggregationPayload.fromAmount is used as the amountIn input for calling the UniswapV2Router02.swapExactTokensForETH function below. Because the aggregator's balance of such token is less than aggregationPayload.fromAmount, executing TransferHelper.safeTransferFrom(path[0], msg.sender, UniswapV2Library.pairFor(factory, path[0], path[1]), amounts[0]) that attempts to transfer aggregationPayload.fromAmount from the aggregator in the UniswapV2Router02.swapExactTokensForETH function reverts. Hence, when the corresponding ERC20 token is a fee-on-transfer token like STA, calling the _transferOutAndCallV5 function always reverts, and such functionality is always DOS'ed for all vaults.

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L304-L389

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][
        aggregationPayload.fromAsset
      ] -= aggregationPayload.fromAmount; // Reduce allowance

      // send ERC20 to aggregator contract so it can do its thing
      (bool transferSuccess, bytes memory data) = aggregationPayload
        .fromAsset
        .call(
          abi.encodeWithSignature(
            "transfer(address,uint256)",
            aggregationPayload.target,
            aggregationPayload.fromAmount
          )
        );
      ...
      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
      ...
    }
  }

https://github.com/code-423n4/2024-06-thorchain/blob/0e02d3c221d7e546a73f25183e29500eaa63f4cf/chain/ethereum/contracts/THORChain_Aggregator.sol#L144-L216

  function swapOutV5(
    address fromAsset,
    uint256 fromAmount,
    address toAsset,
    address recipient,
    uint256 amountOutMin,
    bytes memory payload,
    string memory originAddress
  ) public payable nonReentrant {
    address[] memory path = new address[](2);
    ...
    if (fromAsset == address(0)) {
      ...
    } else {
      // received an ERC20, swap to ETH or do as you please
      path[0] = fromAsset;
      path[1] = WETH;
      safeApprove(fromAsset, address(swapRouter), 0); // best practice
      safeApprove(fromAsset, address(swapRouter), fromAmount);

      if (payload.length == 0) {
        // no payload, process without wasting gas
        // UniV2 transfers from 'msg.sender', so we need a low-level `.call` to change the msg.sender from the target's perspective
        (bool aggSuccess, ) = address(swapRouter).call(
          abi.encodeWithSignature(
            "swapExactTokensForETH(uint256,uint256,address[],address,uint256)",
            fromAmount,
            amountOutMin,
            path,
            recipient,
            type(uint).max // Assuming using the maximum uint256 value as the deadline
          )
        );
        require(aggSuccess, "swapExactTokensForETH failed");
      } else {
        // do something with your payload like parse to an aggregator specific structure
        // UniV2 transfers from 'msg.sender', so we need a low-level `.call` to change the msg.sender from the target's perspective
        (bool success, ) = address(swapRouter).call(
          abi.encodeWithSignature(
            "swapExactTokensForETH(uint256,uint256,address[],address,uint256)",
            fromAmount,
            amountOutMin,
            path,
            recipient,
            type(uint).max // Assuming using the maximum uint256 value as the deadline
          )
        );
        require(success, "swapExactTokensForETH failed");
      }
    }
  }

https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L284-L300

    function swapExactTokensForETH(uint amountIn, uint amountOutMin, address[] calldata path, address to, uint deadline)
        external
        virtual
        override
        ensure(deadline)
        returns (uint[] memory amounts)
    {
        require(path[path.length - 1] == WETH, 'UniswapV2Router: INVALID_PATH');
        amounts = UniswapV2Library.getAmountsOut(factory, amountIn, path);
        require(amounts[amounts.length - 1] >= amountOutMin, 'UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT');
        TransferHelper.safeTransferFrom(
            path[0], msg.sender, UniswapV2Library.pairFor(factory, path[0], path[1]), amounts[0]
        );
        _swap(amounts, path, address(this));
        IWETH(WETH).withdraw(amounts[amounts.length - 1]);
        TransferHelper.safeTransferETH(to, amounts[amounts.length - 1]);
    }

Proof of Concept

The following steps can occur for the described scenario.

Currently, 1% of the STA token amount to be transferred would be cut and burned.
A user calls the depositWithExpiry function with 100 STA as the amount input for Vault A.
- After such depositWithExpiry function call, due to the 1% cut, _vaultAllowance for STA and Vault A is increased to 99 STA, and the router holds 99 STA.
Vault A calls the transferOutAndCallV5 function, which further calls the _transferOutAndCallV5 function that eventually calls the UniswapV2Router02.swapExactTokensForETH function, with 99 STA as aggregationPayload.fromAmount.
- Because of the 1% cut, 98 STA is transferred to the aggregator.
- Yet, when UniswapV2Router02.swapExactTokensForETH function is eventually called, aggregationPayload.fromAmount that is 99 STA is used as the amountIn input.
Because the UniswapV2Router02.swapExactTokensForETH function attempts to transfer 99 STA from the aggregator but the aggregator only holds 98 STA, calling such function reverts.
- Thus, calling the _transferOutAndCallV5 function for STA is DOS'ed for Vault A.

Tools Used

Manual Review

Recommended Mitigation Steps

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L342-L375 can be updated to the following code.

      _vaultAllowance[msg.sender][
        aggregationPayload.fromAsset
      ] -= aggregationPayload.fromAmount; // Reduce allowance

      uint256 _startBal = iERC20(aggregationPayload.fromAsset).balanceOf(aggregationPayload.target);

      // send ERC20 to aggregator contract so it can do its thing
      (bool transferSuccess, bytes memory data) = aggregationPayload
        .fromAsset
        .call(
          abi.encodeWithSignature(
            "transfer(address,uint256)",
            aggregationPayload.target,
            aggregationPayload.fromAmount
          )
        );

      require(
        transferSuccess && (data.length == 0 || abi.decode(data, (bool))),
        "Failed to transfer token before dex agg call"
      );

      // add test case if aggregator fails, it should not revert the whole transaction (transferOutAndCallV5 call succeeds)
      // call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert
      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          iERC20(aggregationPayload.fromAsset).balanceOf(aggregationPayload.target) - _startBal,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );

Assessed type

DoS

code-423n4 / 2024-06-thorchain-findings

Calling `_transferOutAndCallV5` function is always DOS'ed for all vaults when corresponding ERC20 token is a fee-on-transfer token, such as STA #15