Insufficient Slippage Check in `buybackTokenByV2` Function

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xfcda2650ca217b332b19c2c63bd3036b19c5bebb82a7dab5b04a5cdaab14e134 Severity: high

Description: Description: The buybackTokenByV2 function in the contract checks for slippage by calculating an amountOutQuote and then ensuring the token balance difference matches the amountsOut from the router. However, this approach does not correctly verify slippage and relies on external sources (the router) to ensure correctness. If the router forfeits amountOutQuote and sends fewer tokens to the contract, the check will still pass, resulting in an insufficient slippage check. slippage check must ensure that the difference in token balance after the swap is at least equal to amountOutQuote.

    amountOutQuote = amountOutQuote - (amountOutQuote * slippage_) / SLIPPAGE_PRECISION;
    .
    .
    .

    uint256 balanceBefore = IERC20(targetToken).balanceOf(address(this));

    uint256[] memory amountsOut =
        router.swapExactTokensForTokens(amountIn, amountOutQuote, optimalRoute, address(this), deadline_);

    uint256 amountOut = amountsOut[amountsOut.length - 1];

    assert(IERC20(targetToken).balanceOf(address(this)) - balanceBefore == amountOut); //@audit 
    assert(amountOut > 0);

Impact: Insufficient slippage check and reliance on external sources can lead to receiving fewer tokens than expected, resulting in potential financial loss.

Mitigation: Modify the slippage check to ensure that the difference in token balance after the swap is at least equal to amountOutQuote.

    assert(IERC20(targetToken).balanceOf(address(this)) - balanceBefore >= amountOutQuote);

Explanation:

1. Current Check Issue:

   • The current check assert(IERC20(targetToken).balanceOf(address(this)) - balanceBefore == amountOut) ensures that the amount received matches amountOut from the router.
   • This relies on the router to provide accurate amountOut, which may not always correctly reflect the slippage set by the user.

2. Mitigation Explanation:

   • By changing the check to assert(IERC20(targetToken).balanceOf(address(this)) - balanceBefore >= amountOutQuote), we ensure that the actual amount received is at least the slippage-adjusted quote.
   • This way, even if the router's output is incorrect or maliciously altered, the contract will not accept fewer tokens than expected based on the slippage tolerance.

[BohdanHrytsak]

Hi, tank you for submission

As you can see, buyback integrates the functions of a configured v2 router (fenix/uniswap v2 interface), and the entire mechanism of the buyback relies on this external source.

All these bugs and vulnerabilities are completely dependent on the implementation of the external source, which inherits the uniswap v2 interface. So, in a sense, we trust this source and expect it to work correctly.

Although you also point this out and suggest adding an additional check, it still doesn't solve the problem because amountOutQuote depends on an external source. So why should we trust amountOutQuote and not trust the other results of the function?)

I would say that at best it can only be seen as an improvement to make sure we get a certain amount, although then we are also saying that we trust this function and not another

amountOutQuote is passed by the 2nd parameter as the minimum expected amount where the check you specify should be performed, although this is the same point of trust in the external source.

So here, in general, the standard thing is that trust in some other contract/source is not a problem, especially when the question is to trust part of the functionality and not to trust another part

[0xmahdirostami]

thank you, yes you are right