Trades where toToken is feeOnTransferToken might send user less tokens than finalAmountMin

[code423n4]

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

[tommyz7]

Slingshot concern is to execute the trade as promised and make sure we are sending to the user what has been promised in trade estimation. If the token adds additional taxation on transfer, it is on the user side and users understand and accept this. We have seen this play out on production for the previous version of the contracts and we decided not to make that check. It seems the most practical decision.

Personally, I think this is non-critical.

[alcueca]

The severity for the issue is right. The sponsor should add documentation to the fact that some tokens might not conform to common expectations.