Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Position.isIncreasing() can be easily bypassed by creating an order that flips from long to short

auditism

Summary

Position.isIncreasing(uint128 tradingAccountId, uint128 marketId, int128 sizeDelta)

is used to check whether the order will make the position grow in size. However this check can be easily bypassed by flipping a long position to short or short to long.

Vulnerability Details

In the snippet below:

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

We can see that isIncreasing will return true if the current order is > 0 and the sizeDelta is also > 0. Similarly if the current order is < 0 and sizeDelta is also < 0, isIncreasing will be true.
However this doesn't account for orders that flip from short to long and long to short.
This could be illustrated by the following example:

  • initial position: long order 100.

  • size delta is -500.

  • the current position is now -400. The position is greatly increased, however isIncreasing will return negative.

This will effectively allow traders to increase trading positions in disabled Markets and when settlement are disabled. Which goes against the intended design, when settlements and markets are disabled positions should only be reduced or closed, not increased.

if (ctx.isIncreasing) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}

Furthermore it will allow the trader to pass order with maintenance margin only instead of initial margin .

@> ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
&& ctx.isMarketWithActivePosition;
//@audit is increasing can be bypassed, thus reducing the necessary margin
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? ctx.requiredMaintenanceMarginUsdX18 : ctx.requiredInitialMarginUsdX18;

Impact

This vulnerability endangers the protocol's especially when markets and settlements are disabled for security reasons.
The required margin a user has to use will also decrease greatly depending on the ratio difference between initial and maintenance margin

Tools Used

Manual review

Recommendations

In order mitigate this issue, consider making sure the trader doesn't flip orders in order to bypass the isIncreasing check.

function isIncreasing(
uint128 tradingAccountId,
uint128 marketId,
int128 sizeDelta
)
internal
view
returns (bool)
{
Data storage self = Position.load(tradingAccountId, marketId);
​
// If position is being opened (size is 0) or if position is being increased (sizeDelta direction is same as
// position size)
// For a positive position, sizeDelta should be positive to increase, and for a negative position, sizeDelta
// should be negative to increase.
// This also implicitly covers the case where if it's a new position (size is 0), it's considered an increase.
++ if( self.size * sizeDelta < 0) return abs(self.size) < abs(sizeDelta);
return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
Updates

Lead Judging Commences

inallhonesty Lead Judge over 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.

Give us feedback!