TheNaubit - Users trying to reduce their positions with market orders will always revert

[sherlock-admin]

TheNaubit

high

Users trying to reduce their positions with market orders will always revert

Summary

Reducing the users' positions with market orders will always revert due to a wrong value assigned to the min_amount_out variable when swapping part of the funds out.

Vulnerability Detail

In the Vault contract, there is the reduce_position function, which is called by the users to reduce their positions. When the function is called, the min_amount_out variable used by UniswapV3 as a slippage protection is calculated with the following code:

min_amount_out: uint256 = _min_amount_out
if min_amount_out == 0:
   # market order, add some slippage protection
   min_amount_out = self._market_order_min_amount_out(
       position.position_token, position.debt_token, position.position_amount
   )

When the function receives a min_amount_out equal to 0, it means the user wants to reduce the position with a market order (for example, to reduce it right now). But in that case, some slippage is calculated to bring some protection to the swap. That protection is calculated with the function _market_order_min_amount_out which basically does the following:

return (
        self._quote_token_to_token(_token_in, _token_out, _amount_in)
        * (PERCENTAGE_BASE - self.liquidate_slippage[_token_in][_token_out])
        / PERCENTAGE_BASE
    )

The important part (for us) in that code is the call to _quote_token_to_token, which basically queries the price relation of both tokens and multiply it by the _amount_in var. But... if we check the original function (the one I wrote at the beginning of this section), the value passed as _amount_in to _market_order_min_amount_out is the position_amount value instead of the amount of the position to be reduced (contained in the var _reduce_by_amount).

And since in the _swap function we are swapping only the _reduce_by_amount amount and position_amount is always greater than _reduce_by_amount, the _min_amount_out value that we send in the _swap function will be always greater than the greatest amount out we could receive, making the _swap function to always revert thus making the reduce_position to revert also.

Impact

Users won't be able to reduce their positions using market orders, potentially making them to lose funds due to not being able to reduce their positions in the right moment (since they should use non-market orders, which may not be right thing for their situation).

Code Snippet

https://github.com/sherlock-audit/2023-06-unstoppable/blob/main/unstoppable-dex-audit/contracts/margin-dex/Vault.vy#L309-L311

Tool used

Manual Review

Recommendation

Change the code like this:

if min_amount_out == 0:
   # market order, add some slippage protection
   min_amount_out = self._market_order_min_amount_out(
-       position.position_token, position.debt_token, position.position_amount
+       position.position_token, position.debt_token, _reduce_by_amount
   )

[Unstoppable-DeFi]

Unstoppable-DeFi/unstoppable-dex-audit#5

[maarcweiss]

Fixed by shifting the calculation from using the whole position position.position_amount to the actual amount the user is trying to reduce _reduce_by_amount