sherlock-audit / 2024-08-velar-artha-judging

8 stars 3 forks source link

KupiaSec - The `api.close` function does not check the slippage for the paid amount of base and quote tokens. #116

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 2 months ago

KupiaSec

Medium

The api.close function does not check the slippage for the paid amount of base and quote tokens.

Summary

In the api.close function, it also checks slippage for price variance, but not for paid amounts of base and quote tokens. The token amount that user should pay to close position can be changed by price variance and funding fees based on the imbalance between long and short interest. Attackers can increase the funding fees user should pay by frontrunning. As a result, position creator may have to pay more base and quote tokens than they expected.

Root Cause

There is no slippage check of based token and quote tokens in the api.close.

Internal pre-conditions

None

External pre-conditions

None

Attack Path

  1. Alice opens the long position and there is no price movement for all steps of this scenario.
  2. After some time, Alice is about to close the position, allowing her to pay pay imbalance funding fees.
  3. Bob(attacker) frontruns Alice : he opens the long position with large interest.
  4. Next, Alice's transaction is processed, and she has to pay more imbalance funding fees by Bob.

Impact

Position creators may have to pay more tokens than they expected.

PoC

None

Mitigation

Add the slippage checking of received amount of tokens in the api.close function.

KupiaSecAdmin commented 2 months ago

Escalate

This issue deserves Medium. Information required for this issue to be rejected.

sherlock-admin3 commented 2 months ago

Escalate

This issue deserves Medium. Information required for this issue to be rejected.

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.

WangSecurity commented 1 month ago

But isn't it how the protocol should work? Moreover, the mempool is private on Bob, if I'm not mistaken and for this scenario to happen, the user has to just open a large, long position, and anyone closing their position after this, would have to pay a larger funding fee. Hence, it looks to be working as expected that every new position affects the funding fee, what's broken here?

KupiaSecAdmin commented 1 month ago

However, in this case, the amount to be paid is greater than the user expects. Furthermore, after some time, the amount to be paid can decrease due to the profits and funding fees received.

spacegliderrrr commented 1 month ago

Issue does not even make any sense. Funding fees are paid based on the time duration and size of imbalance. If a transaction slips just before Alice closes her position, it would not make a difference on the fees she'll pay or receive. Also, the amount of funding fees that the user will pay can never decrease, as each position pays funding fees in their collateral token and receives funding fees in the other token.

WangSecurity commented 1 month ago

Based on the above comment, this issue should remain invalid as this scenario would make a difference on the fees and other arguments mentioned in the comment. Planning to reject the escalation and leave the issue as it is.

WangSecurity commented 1 month ago

Result: Invalid Unique

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status: