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

Users can increase their position in a disabled market

tedox

Summary

In OrderBranch::createMarketOrder users are only allowed to decrease/close their position when the market or settlement is disabled. But due to a insufficient check inside Position::isIncreasing a user increase their position in the opposite side (e.g turn a short into a long or a long into a short).

Vulnerability Details

When a user calls OrderBranch::createMarketOrder the following checks are made:

​
// determine whether position is being increased or not
ctx.isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
​
// both markets and settlement can be disabled, however when this happens we want to:
// 1) allow open positions not subject to liquidation to decrease their size or close
// 2) prevent new positions from being opened & existing positions being increased
//
// the idea is to prevent a state where traders have open positions but are unable
// to reduce size or close even though they can still be liquidated; such a state
// would severly disadvantage traders
if (ctx.isIncreasing) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}

That is in order to prevent users from increasing their position in a disabled market or when settlement is disabled which can happen for multiple reasons and is always necessary to have. If we take a look in the Position::isIncreasing function we can see it determines the direction of the change by the signs of the size and sizeDelta variables but it ignores their magnitude

​
/// @notice Determines if a given position size change is an increase in the position.
/// @dev This function checks if the position is being opened or increased based on the size delta and the current
/// position size.
/// @param tradingAccountId The unique identifier for the trading account.
/// @param marketId The market identifier where the position is held.
/// @param sizeDelta The change in position size; positive for an increase, negative for a decrease.
/// @return isBeingIncreased Returns true if the position is being opened (size is 0) or increased (sizeDelta
/// direction matches position size), otherwise false.
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.
return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
}
}
​

That means that if sizeDelta is big enough it could create a position on the opposite side.
Example:
size = 1BTC, sizeDelta = -3BTC
After the check self.size == 0 is false (self.size > 0 && sizeDelta > 0) is also false and (self.size < 0 && sizeDelta < 0) is false making the entire return value false making the contract believe that the user is reducing/closing their position while they are actually increasing the amount of liquidity they are using in their trade.

Impact

Users are allowed to increase their positions despite the fact that they should not be able to which can lead to unexpected behavior and a likely loss of funds as market are usually disabled due to market manipulations and other security concerns.

Tools Used

Manual review
VS Code

Recommendations

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.
- return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
+ return self.size == 0 || (abs(self.size + sizeDelta) > abs(self.size));
}
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.