Slingshot: Incorrect initial balance fetched for native token in executeTrades()

[code423n4]

Handle

hickuphh3

Vulnerability details

Impact

The executioner contract only supports ERC20<>ERC20 token trades. Native token swaps are supported by either wrapping / unwrapping the ERC20 wrapped native token before / after the trades respectively.

When exchanging from the native token, the wrapping occurs in the slingshot contract before it is transferred to the executioner contract. When exchanging to the native token, the unwrapping occurs in the executioner contract.

Examine how the initial and final balances are fetched in executeTrades().

Initial Balance

uint256 initialBalance = _getTokenBalance(toToken);

Final Balance

if (toToken == nativeToken) {
    finalBalance = _getTokenBalance(address(wrappedNativeToken));
} else {
  finalBalance = _getTokenBalance(toToken);
}

_getTokenBalance()

function _getTokenBalance(address token) internal view returns (uint256) {
  if (token == nativeToken) {
    return address(executioner).balance;
  } else {
    return IERC20(token).balanceOf(address(executioner));
  }
}

Notice the discrepancy between the initial and final balance fetched if toToken is the native token. The balance before is the actual native token balance in the executioner contract, but the balance after is the wrapped token balance.

Effects

This leads to potentially wrong comparisons.

1. A malicious user can cause griefing by sending native tokens directly to the executioner contract (note that it has a receiver function) that exceeds the finalAmountMin of a user.
2. Wrapped tokens that are accidentally sent to the executioner contract can be exploited as they can be considered to be part of the attacker's trade output. For instance,
   • Alice accidentally sends WETH to the executioner contract
   • Mallory specifies a minimal swap with ETH_ADDRESS (nativeToken) as the toToken. Alice's stuck WETH in the executioner contract are included and sent to Mallory's trade.

Recommended Mitigation Steps

Based on the design, the balances fetched should be from the wrapped token if toToken is specified as the native token.

function _getTokenBalance(address token) internal view returns (uint256) {
  if (token == nativeToken) {
      return IERC20(address(wrappedNativeToken)).balanceOf(address(executioner));
  } else {
      return IERC20(token).balanceOf(address(executioner));
  }
}

Then both initial and final balances can be fetched by simply calling _getTokenBalance()

uint256 initialBalance = _getTokenBalance(toToken);
uint256 finalBalance = _getTokenBalance(toToken);

[tommyz7]

Duplicate of #18