`require(msg.value == 0)` should be added to prevent fund loss when called with mistaken input data

[code423n4]

Handle

WatchPug

Vulnerability details

There is a potential mistake that can be made on the input data of executeTrades() which uses the address of wrappedNativeToken as fromToken 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 with 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 ApprovalHandler.sol;

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

The caller will suffer a fund loss of 1 WETH.

Recommendation

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L143-L152

function _transferFromOrWrap(address fromToken, address from, uint256 amount) internal {
    // transfer tokens or wrap ETH
    if (fromToken == nativeToken) {
        require(msg.value == amount, "Slingshot: incorrect ETH value");
        wrappedNativeToken.deposit{value: amount}();
        wrappedNativeToken.safeTransfer(address(executioner), amount);
    } else {
        require(msg.value == 0, "Slingshot: incorrect ETH value");
        approvalHandler.transferFrom(fromToken, from, address(executioner), amount);
    }
}

[tommyz7]

Duplicate of #95