ctf_sec - Issue when handling native ETH trade and WETH trade in DODO RouterProxy#externalSwap

[github-actions[bot]]

ctf_sec

medium

Issue when handling native ETH trade and WETH trade in DODO RouterProxy#externalSwap

Summary

Lack of logic to wrap the native ETH to WETH in function externalSwap

Vulnerability Detail

The function exeternalSwap can handle external swaps with 0x, 1inch and paraswap or other external resources.

    function externalSwap(
        address fromToken,
        address toToken,
        address approveTarget,
        address swapTarget,
        uint256 fromTokenAmount,
        uint256 minReturnAmount,
        bytes memory feeData,
        bytes memory callDataConcat,
        uint256 deadLine
    ) external payable judgeExpired(deadLine) returns (uint256 receiveAmount) {      
        require(isWhiteListedContract[swapTarget], "DODORouteProxy: Not Whitelist Contract");  
        require(isApproveWhiteListedContract[approveTarget], "DODORouteProxy: Not Whitelist Appprove Contract");  

        // transfer in fromToken
        if (fromToken != _ETH_ADDRESS_) {
            // approve if needed
            if (approveTarget != address(0)) {
                IERC20(fromToken).universalApproveMax(approveTarget, fromTokenAmount);
            }

            IDODOApproveProxy(_DODO_APPROVE_PROXY_).claimTokens(
                fromToken,
                msg.sender,
                address(this),
                fromTokenAmount
            );
        }

        // swap
        uint256 toTokenOriginBalance;
        if(toToken != _ETH_ADDRESS_) {
            toTokenOriginBalance = IERC20(toToken).universalBalanceOf(address(this));
        } else {
            toTokenOriginBalance = IERC20(_WETH_).universalBalanceOf(address(this));
        }

note the code above, if the fromToken is set to _ETH_ADDRESS, indicating the user wants to trade with native ETH pair. the function does has payable modifier and user can send ETH along when calling this function.

However, the toTokenOriginBalance is check the only WETH balance instead of ETH balance.

  if(toToken != _ETH_ADDRESS_) {
      toTokenOriginBalance = IERC20(toToken).universalBalanceOf(address(this));
  } else {
      toTokenOriginBalance = IERC20(_WETH_).universalBalanceOf(address(this));
  }

Then we do the swap:

(bool success, bytes memory result) = swapTarget.call{
    value: fromToken == _ETH_ADDRESS_ ? fromTokenAmount : 0
}(callDataConcat);

If the fromToken is _ETH_ADDRESS, we send the user supplied fromTokenAmount without verifying that the fromTokenAmount.

Finally, we use the before and after balance to get the amount with received.

// calculate toToken amount
  if(toToken != _ETH_ADDRESS_) {
      receiveAmount = IERC20(toToken).universalBalanceOf(address(this)) - (
          toTokenOriginBalance
      );
  } else {
      receiveAmount = IERC20(_WETH_).universalBalanceOf(address(this)) - (
          toTokenOriginBalance
      );
  }

We are checking the WETH amount instead of ETH amount again.

The issue is that some trades may settle the trade in native ETH, for example

https://developers.paraswap.network/smart-contracts

we can look into the Paraswap contract

https://etherscan.io/address/0xDEF171Fe48CF0115B1d80b88dc8eAB59176FEe57#writeProxyContract

If we click the implementation contract and see the method swapOnUniswapV2Fork

https://etherscan.io/address/0x4ff0dec5f9a763aa1e5c2a962aa6f4edfee4f9ea#code

Code line 927 - 944, which calls the function

function swapOnUniswapV2Fork(
    address tokenIn,
    uint256 amountIn,
    uint256 amountOutMin,
    address weth,
    uint256[] calldata pools
)
    external
    payable
{
    _swap(
        tokenIn,
        amountIn,
        amountOutMin,
        weth,
        pools
    );
}

which calls:

  function _swap(
        address tokenIn,
        uint256 amountIn,
        uint256 amountOutMin,
        address weth,
        uint256[] memory pools
    )
        private
        returns (uint256 tokensBought)
    {
        uint256 pairs = pools.length;

        require(pairs != 0, "At least one pool required");

        bool tokensBoughtEth;

        if (tokenIn == ETH_IDENTIFIER) {
            require(amountIn == msg.value, "Incorrect msg.value");
            IWETH(weth).deposit{value: msg.value}();
            require(IWETH(weth).transfer(address(pools[0]), msg.value));
        } else {
            require(msg.value == 0, "Incorrect msg.value");
            transferTokens(tokenIn, msg.sender, address(pools[0]), amountIn);
            tokensBoughtEth = weth != address(0);
        }

        tokensBought = amountIn;

        for (uint256 i = 0; i < pairs; ++i) {
            uint256 p = pools[i];
            address pool = address(p);
            bool direction = p & DIRECTION_FLAG == 0;

            tokensBought = NewUniswapV2Lib.getAmountOut(
                tokensBought, pool, direction, p >> FEE_OFFSET
            );
            (uint256 amount0Out, uint256 amount1Out) = direction
                ? (uint256(0), tokensBought) : (tokensBought, uint256(0));
            IUniswapV2Pair(pool).swap(
                amount0Out,
                amount1Out,
                i + 1 == pairs
                    ? (tokensBoughtEth ? address(this) : msg.sender)
                    : address(pools[i + 1]),
                ""
            );
        }

        if (tokensBoughtEth) {
            IWETH(weth).withdraw(tokensBought);
            TransferHelper.safeTransferETH(msg.sender, tokensBought);
        }

        require(tokensBought >= amountOutMin, "UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT");
    }

as can clearly see, the code first receive ETH, wrap ETH to WETH, then instead end, unwrap the WETH to ETH and the send the ETH back to complete the trade.

if (tokensBoughtEth) {
    IWETH(weth).withdraw(tokensBought);
    TransferHelper.safeTransferETH(msg.sender, tokensBought);
}

In DODORouterProxy.sol#ExternalSwap however, we are using WETH balance before and after to check the received amount,

but if we call swapOnUniswapV2Fork on Paraswap router, the balance change for WETH would be 0

because as we see above, the method on paraswap side wrap ETH to WETH but in the end unwrap WETH and send ETH back.

There is also a lack of a method to wrap the ETH to WETH before the trade. making the ETH-related order not tradeable.

Impact

A lot of method that does not use WETH to settle the trade will not be callable.

Code Snippet

https://github.com/sherlock-audit/2022-11-dodo/blob/main/contracts/SmartRoute/DODORouteProxy.sol#L158-L230

Tool used

Manual Review

Recommendation

We recommend the project change from

  // swap
  uint256 toTokenOriginBalance;
  if(toToken != _ETH_ADDRESS_) {
      toTokenOriginBalance = IERC20(toToken).universalBalanceOf(address(this));
  } else {
      toTokenOriginBalance = IERC20(_WETH_).universalBalanceOf(address(this));
  }

  // swap
  uint256 toTokenOriginBalance;
  if(toToken != _ETH_ADDRESS_) {
      toTokenOriginBalance = IERC20(toToken).universalBalanceOf(address(this));
  } else {
      toTokenOriginBalance = IERC20(_ETH_ADDRESS).universalBalanceOf(address(this));
  }

If we want to use WETH to do the balance check, we can help the user wrap the ETH to WETH by calling before do the balance check.

IWETH(_WETH_).deposit(receiveAmount);

If we want to use WETH as the reference to trade, we also need to approve external contract to spend our WETH.

We can add

if(fromToken == _ETH_ADDRESS) {
   IERC20(_WETH_).universalApproveMax(approveTarget, fromTokenAmount);
}

We also need to verify the fromTokenAmount for

(bool success, bytes memory result) = swapTarget.call{
    value: fromToken == _ETH_ADDRESS_ ? fromTokenAmount : 0
}(callDataConcat);

we can add the check:

require(msg.value == fromTokenAmount, "invalid ETH amount");

[Attens1423]

In our api, we require toToken is WETH when contructing callData. We will add some notes here. Thanks for noticing

[Evert0x]

Even tough the API is requiring WETH we still think it's a valid issue as the contract has a payable modifier.

[Attens1423]

We will add this check: require(msg.value == fromTokenAmount, "invalid ETH amount"); As the fromToken is ETH, we won't deposit it into routeProxy, we will transfer the ETH amount directly.

[Attens1423]

https://github.com/DODOEX/dodo-route-contract/pull/2

[aktech297]

comments are added at the start of the function. Fixes are done as mentioned in, We will add this check: require(msg.value == fromTokenAmount, "invalid ETH amount");