Ineffective slippage protection allows for sandwich attacks

[hats-bug-reporter[bot]]

Github username: @0xEVom Twitter username: 0xEV_om Submission hash (on-chain): 0xfb6d84599a705570ed42038ea783841ff0f1fb4361604aec2036480cc78cb588 Severity: high

Description: Description\ The buybackTokenByV2() function in the SingelTokenBuybackUpgradeable contract is designed to perform token buybacks using a DEX router. It calculates the expected output amount based on the current market conditions and applies a user-specified slippage tolerance. However, the current implementation of slippage protection is ineffective and leaves users vulnerable to sandwich attacks.

The function calculates the minimum output amount (amountOutQuote) within the same transaction as the swap execution. This approach has two major flaws:

1. The calculation is based on the current state of the liquidity pool, which can be manipulated by an attacker just before the transaction is executed.
2. The slippage is applied as a percentage of this potentially manipulated quote, rather than being an absolute minimum output amount provided by the user.

As a result, an attacker can front-run the transaction with a large swap to significantly impact the price, causing the user's transaction to receive far fewer tokens than expected, even with the applied slippage protection.

The relevant part of the buybackTokenByV2() function is:

File: SingelTokenBuybackUpgradeable.sol
161:         amountOutQuote = amountOutQuote - (amountOutQuote * slippage_) / SLIPPAGE_PRECISION;
162:         if (amountOutQuote == 0) {
163:             revert RouteNotFound();
164:         }
165: 
166:         IRouterV2 router = IRouterV2(routerV2PathProviderCache.router());
167:         inputTokenCache.safeApprove(address(router), amountIn);
168: 
169:         uint256 balanceBefore = IERC20(targetToken).balanceOf(address(this));
170: 
171:         uint256[] memory amountsOut = router.swapExactTokensForTokens(amountIn, amountOutQuote, optimalRoute, address(this), deadline_);

Attack Scenario\

1. Alice initiates a buyback transaction with a 1% slippage tolerance.
2. The contract calculates amountOutQuote based on the current pool state, expecting to receive 1000 tokens.
3. An attacker front-runs Alice's transaction with a large swap, significantly impacting the price.
4. Alice's transaction is executed, but due to the price manipulation, the new amountOutQuote is now only 800 tokens.
5. The contract applies the 1% slippage to this manipulated quote, setting the minimum output to 792 tokens.
6. The swap executes, and Alice receives only 800 tokens instead of the expected 1000, despite setting a 1% slippage tolerance.
7. The attacker back-runs the transaction, reversing their initial swap and profiting from the price manipulation.

Recommended Mitigation\ To properly protect users from sandwich attacks and unexpected slippage, implement the following changes:

1. Require users to provide a minimum output amount instead of a percentage-based slippage.
2. Update the interface and any calling contracts to provide the minimum output amount instead of a slippage percentage.
3. Consider implementing a separate off-chain quoting mechanism that users can call before submitting their transaction, allowing them to calculate an appropriate minimum output amount based on current market conditions and their desired slippage tolerance.

These changes will ensure that users have full control over their acceptable slippage and are protected from sandwich attacks and unexpected price movements.

[0xmahdirostami]

Thanks, yes, all slippage parameters must be calculated off-chain, valid issue.

[BohdanHrytsak]

@0xmahdirostami

1. As you can see, slippage_ is an input parameter that is determined off-chain

   function buybackTokenByV2(
       address inputToken_,
       IRouterV2.route[] calldata inputRouters_,
       uint256 slippage_,
       uint256 deadline_
   ) external

2. The amountOutQuote is a value defined by the RouterV2PathProvider, and is based on the historical price:

   
   File: RouterV2PathProviderUpgradeable.sol

291: amountIn = IPairQuote(pair).quote(routes[i].from, amountIn_, PAIR_QUOTE_GRANULARITY);

3. At the end, slippage is calculated from the historical price

Therefore, I think that the measures taken to prevent the described attack

Therefore, the statement: `The calculation is based on the current state of the liquidity pool, which can be manipulated by an attacker just before the transaction is executed.` is false.

Since manipulating several price blocks is an expensive operation

[0xmahdirostami]

Lead: Upon thoroughly investigating the issue, the scenario for price manipulation is deemed incorrect. The price fetching mechanism relies on the quote function of the pair, which uses the average price rather than the spot price. This approach makes the issue invalid, as manipulating the average price is impractical and would incur a high cost for the attacker. Consequently, the potential for successful price manipulation under these conditions is highly unlikely. besides that if there are multiple route, attacker must manipulate all routes.

[0xEVom]

The way I see it, that twap implementation is highly vulnerable.

The pool calls _update() on any swap, which will record a price after a given interval has passed since the last record. Then, the last 3 such observations are used to derive the historical price.

Therefore all an attacker needs to do to manipulate the price is perform an "empty sandwich" at 3 specific times at almost no cost.

Furthermore, if the next transaction is due to record an observation, the attacker can also sandwich the victim's swap as described in this finding. Since the manipulated price will have as much weight as the past 2, the impact is largely the same.

This implementation will also not allow the caller to protect themselves from slippage at all if the price has gone down w.r.t the historical average. In that case, getAmountOutQuote() will return a smaller amount hence even with slippage_ == 0, they can be sandwiched also as described in the finding by the change w.r.t the average. So if the current price is 20% lower than the historical average, they can be sandwiched by up to 25%.

The finding may not have been entirely accurate, but there are serious issues with this implementation and in some situations the exact attack described is possible.

Slippage must be calculated offchain to provide a concrete min output amount and never be calculated on-chain based on a percentage parameter as is done here, no matter how that percentage was calculated.

[0xEVom]

@0xmahdirostami