Slingshot Finance contest - kenzo's results

Handle

kenzo

Vulnerability details

When toToken == nativeToken, executeTrades compares Executioner's starting nativeToken balance to Executioner's ending wrappedNativeToken balance.

Impact

• Loss of user funds or DOS of executeTrades: if there is extra ETH in Executioner contract, finalOutputAmount will be calculated to be smaller than it really is, thereby sending less tokens than deserved to the user, or failing the >= finalAmountMin check and erroneously failing the trade.
• Skimming of accidently-sent WETH: if Executioner's WETH balance is > 0, a user can craft a trade that will send these tokens to him.

Proof of Concept

Slingshot saves the initialBalance as: uint256 initialBalance = _getTokenBalance(toToken); https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L81 If toToken == nativeToken, initialBalance will be Executioner's nativeToken (ETH) balance. https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L158:#L159

Then after the trades, if toToken == nativeToken, Slingshot sends wrappedNativeToken to _getTokenBalance. https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L87:#L88 Therefore, finalBalance will be Executioner's wrappedNativeToken balance (WETH), unlike initialBalance (ETH). This inconsistency manifests the two scenarios mentioned in Impact above. Loss of user funds or DOS of executeTrades - if there is extra ETH in Executioner, uint finalOutputAmount = finalBalance - initialBalance gives a wrong result and might send user less tokens than deserved or fail the finalOutputAmount >= finalAmountMin check. https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L92 Skimming of accidently-sent WETH: If a user sends a trade where toToken == nativeToken, he will transfer to himself all the extra wrappedNativeToken in Executioner as initialBalance is the Executioner's nativeToken balance, not wrappedNativeToken.

Tools Used

Manual analysis

Recommended Mitigation Steps

In initialBalance, instead of passing toToken to _getTokenBalance: https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L81 Align it with the same mechanism that is used to check finalBalance: https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L87:#L91

Handle

kenzo

Vulnerability details

Slingshot's executeTrades checks that the trade result amount (to be sent to the user) is bigger than finalAmountMin, and after that sends the user the amount. But if the token charges fee on transfer, the final transfer to the user will decrease the amount the user is getting, maybe below finalAmountMin.

Proof of Concept

Slingshot requires finalOutputAmount >= finalAmountMin before sending the funds to the user: https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L93:#L98 So if the token charges fees on transfer, the user will get less tokens than finalOutputAmount . The check of finalOutputAmount against finalAmountMin is premature.

Tools Used

Manual analysis

Recommended Mitigation Steps

Save the user's (not Executioner's) toToken balance in the beginning of executeTrades after _transferFromOrWrap(fromToken, _msgSender(), fromAmount), and also in the very end, after executioner.sendFunds(toToken, _msgSender(), finalOutputAmount) has been called. The subtraction of user's initial balance from ending balance should be bigger than finalAmountMin. https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol#L65:#L99