Incorrect calculation of initialBalance in Slingshot.executeTrades()

[code423n4]

Handle

daejunpark

Vulnerability details

Impact

The Slingshot.executeTrades() incorrectly calculates initialBalance when toToken == nativeToken. It should have been the balance of wrapped native tokens (e.g., WETH), rather than that of native currencies (e.g., Ether). This incorrect behavior introduces various attack vectors. Below are example exploit scenarios.

Scenario 1

1. Suppose the Executioner contract happens to hold a good amount of WETH balance, say 10 WETH. (This could happen, for example, when poor users might have accidentally sent their WETH tokens to it, or some dust leftovers during the previous trades might have been accumulated for a long period of time.)
2. Suppose the Ether balance of the Executioner contract is zero at this point.
3. Now, adversaries can call Slingshot.executeTrades(), with fromToken being a dummy token, fromAmount being a tiny amount, and toToken being set to nativeToken.
4. Then, the returned amount will be the dummy trading output plus the existing 10 WETH tokens. This way, adversaries can drain any idle WETH tokens sitting in the Executioner contract.

Scenario 2

1. Suppose Alice accidentally or maliciously sends 7 Ether to the Executioner contract.
2. Suppose Bob calls Slingshot.executeTrades(), attempting to buy 10 WETH, where he accidentally sets finalAmountMin to zero.
3. Then, Bob will get only 3 WETH in return, and the remaining 7 WETH will stay in the Executioner contract. Note that the remaining 7 WETH tokens can be stolen by adversaries using scenario 1.
4. Later Alice may ask the Executioner contract owner to rescue her funds.

Remarks for the scenario 2:

• It is hard to tell if Alice is malicious or not. Thus, in most cases, the contract owner would end up rescuing her funds at step 4 to make her happy.
• In step 2, if Bob didn't make a mistake and set finalAmountMin large enough, then his trading request will fail, and he can avoid losing his funds, which is good. However, note that any such trades for WETH with a reasonable finalAmountMin value will always fail until Alice's funds are rescued. Thus this behavior can be exploited for griefing attacks.

Recommendation

For the short term, fix the following initialBalance calculation logic to be consistent with that of finalBalance below:

• initialBalance: Slingshot.sol#L81

      uint256 initialBalance = _getTokenBalance(toToken);

• finalBalance: Slingshot.sol#L86-L91

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

For the long term, refactor the contract to decouple the native token handling logic from the main business logic. This can minimize the case splits and reduce the likelihood of making a similar mistake again in the future when upgrading the code base.

[tommyz7]

Duplicate of #18