stopthecap - Incorrect calculation of the slippage when reducing positions .

[sherlock-admin]

stopthecap

high

Incorrect calculation of the slippage when reducing positions .

Summary

Incorrect calculation of the slippage when reducing positions .

Vulnerability Detail

When reducing a position. Unstoppablecalculates the slippage (min_amount_out) for you if 0 was specified.
The calculation is wrong because when reducing position you are not swapping the whole position amount, that would be done in the close_position function. In the reduce_position function, you have to specify, as a trader, the amount that you want to reduce your position by _reduce_by_amount.

The error relies in the following lines:
https://github.com/sherlock-audit/2023-06-unstoppable/blob/94a68e49971bc6942c75da76720f7170d46c0150/unstoppable-dex-audit/contracts/margin-dex/Vault.vy#L307-L311

    if min_amount_out == 0: 
        min_amount_out = self._market_order_min_amount_out(
            position.position_token, position.debt_token, position.position_amount
        )

The slippage is being calculated with the entire position.position_amount instead of the actual _reduce_by_amount amount. This will cause a wrong slippage to be calculated, getting either a wrong amount back from the swap, or swaps not being able to take place because the slippage amount was to high.

Impact

A wrong slippage will cause users to get an incorrect amount funds back from the swap, or if it is too high, transactions will fail

Code Snippet

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

Tool used

Manual Review

Recommendation

Change the position.position_amount variable for _reduce_by_amount

    if min_amount_out == 0: 
        min_amount_out = self._market_order_min_amount_out(
            position.position_token, position.debt_token, _reduce_by_amount
        )

Duplicate of #120

[141345]

Duplicate of #120

[0xffff11]

Escalate

This should be a high together with issue #120

[sherlock-admin2]

Escalate

This should be a high together with issue #120

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[twicek]

Escalate for 10 USDC
Severity should be medium because it will only prevent position reduction in the worst case since min_amount_out will be higher than expected leading for the swap to revert. There is no loss of funds, in the worst case it reverts.

[sherlock-admin2]

Escalate for 10 USDC
Severity should be medium because it will only prevent position reduction in the worst case since min_amount_out will be higher than expected leading for the swap to revert. There is no loss of funds, in the worst case it reverts.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[141345]

Recommendation:
keep the original judging.

Escalate

This should be a high together with issue #120

Loss is limited, no material loss of fund, medium is appropriate

[141345]

Escalate for 10 USDC Severity should be medium because it will only prevent position reduction in the worst case since min_amount_out will be higher than expected leading for the swap to revert. There is no loss of funds, in the worst case it reverts.

same as above

[0xffff11]

Agree with @twicek . After reviewing I believe medium is the correct severity. Thanks

[hrishibhat]

Result:
Medium
Duplicate of #120

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• twicek: accepted
• 0xffff11: rejected