DeFiFoundry
60,000 USDC
View results
Submission Details
Severity: high
Valid

Multiple Checks Can Be Bypassed Due to Faulty `isIncreasing` Function

Summary

The isIncreasing check can return false (decreasing) for an increasing order, allowing any user to bypass multiple checks and have orders be settled that should not be.

These checks include:

  • checkMarketIsEnabled: By bypassing this, a user can increase a position in a disabled market.

  • checkIsSettlementEnabled: Allowing a user to settle an increase order despite that functionality being disabled.

  • shouldUseMaintenanceMargin: Allowing a user to increase a position while only needing to meet the maintenance margin instead of the (higher) initial margin.

Vulnerability Details

Any user can bypass the isIncreasing check because the function assumes that if the size of the position and the delta are of different signs (positive & negative), then the position is decreasing.

However, this is not always the case, specifically when a user increases the absolute value of the position while switching from a long to a short.

In this instance, the total open interest of the market is increasing, and the user has a larger position than they did previously. But the isIncreasing check will return false despite all of this.

return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);

As you can see, there are three possible conditions in which if the user passes any of these, the return value will be true. If none of the conditions are met, then the return value will be false. This check accurately catches three possible cases in which a position is increasing; however, there is a fourth.

Take for example:

  • Alice opens a long position with a size of 100.

  • The price decreases, putting Alice at a loss of, say, $20. At this point, Alice would no longer meet the Initial Margin check.

  • Zaros sees instability with the particular market Alice is in and decides to disable the market and disable all settlements to prevent the instability from being exploited.

Alice still wants to exploit this instability, so Alice creates another order on her position, this time with a size of -10,000.

self.size = 100
sizeDelta = -10,000

Looking again at the isIncreasing check, you will see that a clear increase in position is not caught by this check.

return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
return 100 == 0 || (100 > 0 && -10,000 > 0) || (100 < 0 && -10,000 < 0);
100 == 0 // False
(100 > 0 && -10,000 > 0) // False
(100 < 0 && -10,000 < 0) // False

With everything returning false, Alice is able to have her position settled, bypassing all three checks.

With a high likelihood and medium impact, a high severity rating makes sense here.

The high likelihood comes from this being easy to pull off at any given moment and not dependent on any preconditions.

The medium impact comes from protocol safeguards being bypassed in multiple areas, allowing for a variety of unexpected behaviors that would end up hurting the protocol.

Impact

Three checks that are meant to protect the market can be bypassed, hurting the protocol overall and benefiting the attacker.

Tools Used

  • Manual analysis

Recommendations

Modify the check from

return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);

to

return self.size == 0 || abs(self.size) < abs(self.size + sizeDelta);
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Disable market limitation can be bypassed

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.