Slingshot Finance contest - cmichel's results

Handle

cmichel

Vulnerability details

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).safeApprove(address(operator), 0);
IERC20(token).safeApprove(address(operator), amount);

This leads to an issue in LibERC20Token.approveIfBelow's safeIncreaseAllowance call which does not safeApprove(0) first.

Impact

When using one of these unsupported tokens, all transactions revert and the protocol cannot be used.

Recommended Mitigation Steps

Safeapprove with a zero amount first before setting the actual amount. Or just approve the max amount once for each token to circumvent this issue and save gas.

Handle

cmichel

Vulnerability details

This is probably what is meant by:

"rescueTokens and rescueTokensFromExecutioner can be gamed however it is not a concern. They are in place "just in case" and should not be used in the first place."

However, this approach does not even call rescueTokens and rescueTokensFromExecutioner, so I thought I'd mention it just in case.

There are several ways how tokens can end up in the Executioner:

• wrong user input
• several trades and not using tradeAll which will leave dust

These left-over tokens can be stolen by a backrunner that does not provide tokens themselves (fromAmount = 0) but still trades these tokens using the encodedCalldata and receives the swap output.

Impact

Tokens that end up in the contract after a series of misconfigured trades can be stolen by anyone.

Recommended Mitigation Steps

This is rather cumbersome to completely prevent with the current opaque encodedCalldata and not knowing which tokens are touched.

Handle

cmichel

Vulnerability details

Instead of using block.timestamp for a deadline like in swap, consider using a constant in the future instead, like 1e10.