Change `require` conditions can prevent fund loss when called with mistaken input data

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-tally/blob/c585c214edb58486e0564cb53d87e4831959c08b/contracts/swap/Swap.sol#L106-L123

function swapByQuote(
    address zrxSellTokenAddress,
    uint256 amountToSell,
    address zrxBuyTokenAddress,
    uint256 minimumAmountReceived,
    address zrxAllowanceTarget,
    address payable zrxTo,
    bytes calldata zrxData,
    uint256 deadline
) external payable whenNotPaused nonReentrant {
    require(
        block.timestamp <= deadline,
        "Swap::swapByQuote: Deadline exceeded"
    );
    require(
        !signifiesETHOrZero(zrxSellTokenAddress) || msg.value > 0,
        "Swap::swapByQuote: Unwrapped ETH must be swapped via msg.value"
    );

There is a potential mistake that can be made on the input data for swapByQuote() which uses the address of WETH as zrxSellTokenAddress and send unwrapped ETH via msg.value.

This is how the input data of the Uni v2 Router is formatted. Considering the popularity of Uni v2, it might be possible that some user or contract will call Tally's Swap.sol contract using calldata formatted in such a way.

https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L260-L263

function swapExactETHForTokens(uint amountOutMin, address[] calldata path, address to, uint deadline)
    external
    virtual
    override
    payable
    ensure(deadline)
    returns (uint[] memory amounts)
{
    require(path[0] == WETH, 'UniswapV2Router: INVALID_PATH');
    amounts = UniswapV2Library.getAmountsOut(factory, msg.value, path);
    require(amounts[amounts.length - 1] >= amountOutMin, 'UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT');
    IWETH(WETH).deposit{value: amounts[0]}();
    assert(IWETH(WETH).transfer(UniswapV2Library.pairFor(factory, path[0], path[1]), amounts[0]));
    _swap(amounts, path, to);
}

PoC

Given:

• The caller has approved WETH to Swap.sol;

When the caller calls Swap.sol#swapByQuote() with msg.value set to 1e18, and zrxSellTokenAddress set to WETH. The swap will go through and besides the 1 ether sent via msg.value, 1 WETH will be transferred from the caller to the contract mistakenly.

The caller will suffer a fund loss of 1 WETH.

Recommendation

Change to:

function swapByQuote(
    address zrxSellTokenAddress,
    uint256 amountToSell,
    address zrxBuyTokenAddress,
    uint256 minimumAmountReceived,
    address zrxAllowanceTarget,
    address payable zrxTo,
    bytes calldata zrxData,
    uint256 deadline
) external payable whenNotPaused nonReentrant {
    require(
        block.timestamp <= deadline,
        "Swap::swapByQuote: Deadline exceeded"
    );
    require(
        signifiesETHOrZero(zrxSellTokenAddress) ? msg.value > 0 : msg.value == 0,
        "Swap::swapByQuote: Unwrapped ETH must be swapped via msg.value"
    );

[Shadowfiend]

A few notes here:

• The suggested solution will not work, as the 0x protocol occasionally requires fees paid in ETH (see https://0x.org/docs/guides/swap-tokens-with-0x-api#paying-protocol-fees ).
• I believe the 0x API will return any unused ETH, and will not send ETH to a WETH swap, but this needs further investigation.
• The remainder boils down to ensuring that leftover ETH is refunded in all call paths. Indeed, both the ERC20 and ETH swap code paths refund leftover ETH as reported by fillZrxQuote.

Therefore, while it's possible to call this incorrectly sending ETH for a WETH swap, I don't believe it will result in fund loss.

[0xean]

Based on the 0x documentation I agree with the sponsor on the solution not working. Sponsor is encouraged to create a unit test to mimic this behavior and confirm that the asset flow is as expected.

Closing as proposed solution will not work with 0x specification.