Submission Details
Severity:
Open interest and skew are incorrectly set after liquidation
Summary
When trading accounts are liquidated for a particular perp market the self.openInterest and self.skew are set to zero even though there are still accounts with active postions.
Root Cause
function liquidateAccounts(uint128[] calldata accountsIds) external {
// return if no input accounts to process
if (accountsIds.length == 0) return;
// fetch storage slot for global config
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
// only authorized liquidators are able to liquidate
if (!globalConfiguration.isLiquidatorEnabled[msg.sender]) {
revert Errors.LiquidatorNotRegistered(msg.sender);
}
// working data
LiquidationContext memory ctx;
// load liquidation fee from global config; will be passed in as `settlementFeeUsdX18`
// to `TradingAccount::deductAccountMargin`. The user being liquidated has to pay
// this liquidation fee as a "settlement fee"
ctx.liquidationFeeUsdX18 = ud60x18(globalConfiguration.liquidationFeeUsdX18);
// iterate over every account being liquidated; intentionally not caching
// length as reading from calldata is faster
for (uint256 i; i < accountsIds.length; i++) {
// store current accountId being liquidated in working data
ctx.tradingAccountId = accountsIds[i];
// sanity check for non-sensical accountId; should never be true
if (ctx.tradingAccountId == 0) continue;
// load account's leaf (data + functions)
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(ctx.tradingAccountId);
// get account's required maintenance margin & unrealized PNL
(, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
// get then save margin balance into working data
ctx.marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
// if account is not liquidatable, skip to next account
// account is liquidatable if requiredMaintenanceMarginUsdX18 > ctx.marginBalanceUsdX18
if (!TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18)) {
continue;
}
// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
// clear pending order for account being liquidated
MarketOrder.load(ctx.tradingAccountId).clear();
// copy active market ids for account being liquidated
ctx.activeMarketsIds = tradingAccount.activeMarketsIds.values();
// iterate over memory copy of active market ids
// intentionally not caching length as expected size < 3 in most cases
for (uint256 j; j < ctx.activeMarketsIds.length; j++) {
// load current active market id into working data
ctx.marketId = ctx.activeMarketsIds[j].toUint128();
// fetch storage slot for perp market
PerpMarket.Data storage perpMarket = PerpMarket.load(ctx.marketId);
// load position data for user being liquidated in this market
Position.Data storage position = Position.load(ctx.tradingAccountId, ctx.marketId);
// save open position size
ctx.oldPositionSizeX18 = sd59x18(position.size);
// save inverted sign of open position size to prepare for closing the position
ctx.liquidationSizeX18 = -ctx.oldPositionSizeX18;
// calculate price impact of open position being closed
ctx.markPriceX18 = perpMarket.getMarkPrice(ctx.liquidationSizeX18, perpMarket.getIndexPrice());
// get current funding rates
ctx.fundingRateX18 = perpMarket.getCurrentFundingRate();
ctx.fundingFeePerUnitX18 = perpMarket.getNextFundingFeePerUnit(ctx.fundingRateX18, ctx.markPriceX18);
// update funding rates for this perpetual market
perpMarket.updateFunding(ctx.fundingRateX18, ctx.fundingFeePerUnitX18);
// reset the position
position.clear();
// update account's active markets; this calls EnumerableSet::remove which
// is why we are iterating over a memory copy of the trader's active markets
tradingAccount.updateActiveMarkets(ctx.marketId, ctx.oldPositionSizeX18, SD59x18_ZERO);
// update perp market's open interest and skew; we don't enforce ipen
// interest and skew caps during liquidations as:
// 1) open interest and skew are both decreased by liquidations
// 2) we don't want liquidation to be DoS'd in case somehow those cap
// checks would fail
perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18);
}
}
}
// update perp market's open interest and skew; we don't enforce ipen
// interest and skew caps during liquidations as:
// 1) open interest and skew are both decreased by liquidations
// 2) we don't want liquidation to be DoS'd in case somehow those cap
// checks would fail
perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18);
Vulnerability details
Protocol wishes to avoid liquidation DoS in case some cap checks would fail. It is understandable, however function responsible for those checks is also responsible for the calculation of newOpenInterestX18 and newSkewX18. In liquidateAccounts function ctx.newOpenInterestX18 and ctx.newSkewX18 are only initialized to 0. Their value is never modified thus these zeros will become new self.openInterest and new self.skew.
Proof of Concept
Add this test to liquidateAccounts.t.sol file. Run it with forge test --mt "testFuzz_OpenInterestAndSkew".
function testFuzz_OpenInterestAndSkew(
uint256 marketId,
uint256 secondMarketId,
bool isLong,
uint256 amountOfTradingAccounts,
uint256 timeDelta
)
external
givenTheSenderIsARegisteredLiquidator
whenTheAccountsIdsArrayIsNotEmpty
givenAllAccountsExist
{
TestFuzz_GivenThereAreLiquidatableAccountsInTheArray_Context memory ctx;
ctx.fuzzMarketConfig = getFuzzMarketConfig(marketId);
ctx.secondMarketConfig = getFuzzMarketConfig(secondMarketId);
vm.assume(ctx.fuzzMarketConfig.marketId != ctx.secondMarketConfig.marketId);
amountOfTradingAccounts = bound({ x: amountOfTradingAccounts, min: 1, max: 10 });
timeDelta = bound({ x: timeDelta, min: 1 seconds, max: 1 days });
ctx.marginValueUsd = 10_000e18 / amountOfTradingAccounts;
ctx.initialMarginRate = ctx.fuzzMarketConfig.imr;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
// last account id == 0
ctx.accountsIds = new uint128[](amountOfTradingAccounts + 2);
ctx.accountMarginValueUsd = ctx.marginValueUsd / (amountOfTradingAccounts + 1);
for (uint256 i; i < amountOfTradingAccounts; i++) {
ctx.tradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
openPosition(
ctx.fuzzMarketConfig,
ctx.tradingAccountId,
ctx.initialMarginRate,
ctx.accountMarginValueUsd / 2,
isLong
);
openPosition(
ctx.secondMarketConfig,
ctx.tradingAccountId,
ctx.secondMarketConfig.imr,
ctx.accountMarginValueUsd / 2,
isLong
);
ctx.accountsIds[i] = ctx.tradingAccountId;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
}
setAccountsAsLiquidatable(ctx.fuzzMarketConfig, isLong);
setAccountsAsLiquidatable(ctx.secondMarketConfig, isLong);
ctx.nonLiquidatableTradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
// Open postion but not liquidate this account. There should be non-zero open interest and skew.
openPosition(
ctx.fuzzMarketConfig,
ctx.nonLiquidatableTradingAccountId,
ctx.initialMarginRate,
ctx.accountMarginValueUsd / 4,
isLong
);
openPosition(
ctx.secondMarketConfig,
ctx.nonLiquidatableTradingAccountId,
ctx.secondMarketConfig.imr,
ctx.accountMarginValueUsd / 4,
isLong
);
ctx.accountsIds[amountOfTradingAccounts] = ctx.nonLiquidatableTradingAccountId;
changePrank({ msgSender: liquidationKeeper });
skip(timeDelta);
perpsEngine.liquidateAccounts(ctx.accountsIds);
(,, ctx.openInterestX18) = perpsEngine.getOpenInterest(ctx.fuzzMarketConfig.marketId);
ctx.expectedOpenInterest = sd59x18(
PositionHarness(address(perpsEngine)).exposed_Position_load(
ctx.nonLiquidatableTradingAccountId, ctx.fuzzMarketConfig.marketId
).size
).abs().intoUD60x18().intoUint256();
assertNotEq(ctx.expectedOpenInterest, 0);
assertEq(ctx.openInterestX18.intoUint256(), 0);
ctx.skewX18 = perpsEngine.getSkew(ctx.fuzzMarketConfig.marketId);
ctx.expectedSkew = PositionHarness(address(perpsEngine)).exposed_Position_load(
ctx.nonLiquidatableTradingAccountId, ctx.fuzzMarketConfig.marketId
).size;
assertNotEq(ctx.expectedSkew, 0);
assertEq(ctx.skewX18.intoUint256(), 0);
}
We can see that the open interest and skew are equal zero even though there are active positions in market.
Impact
After liquidation the open interest and the skew are not calculated. As a result the protocol will store incorrect values of open interest and skew. Wrong skew will affect funding fees being paid to traders. A profitable trader can lose part of their profits or some positions could become liquidable even though they were above maintenance margin threshold. This issue will generate loss for users.
Recommended Mitigation Steps
Calculate newOpenInterest and newSkew in liquidateAccounts function. To do that you can use code from checkOpenInterestLimits. Do not include checks.
function checkOpenInterestLimits(
Data storage self,
SD59x18 sizeDelta,
SD59x18 oldPositionSize,
SD59x18 newPositionSize
)
internal
view
returns (UD60x18 newOpenInterest, SD59x18 newSkew)
{
// load max & current open interest for this perp market uint128 -> UD60x18
UD60x18 maxOpenInterest = ud60x18(self.configuration.maxOpenInterest);
UD60x18 currentOpenInterest = ud60x18(self.openInterest);
// calculate new open interest which would result from proposed trade
// by subtracting old position size then adding new position size to
// current open interest
newOpenInterest =
currentOpenInterest.sub(oldPositionSize.abs().intoUD60x18()).add(newPositionSize.abs().intoUD60x18());
- // if new open interest would be greater than this market's max open interest,
- // we still want to allow trades as long as they decrease the open interest. This
- // allows traders to reduce/close their positions in markets where protocol admins
- // have reduced the max open interest to reduce the protocol's exposure to a given
- // perp market
- if (newOpenInterest.gt(maxOpenInterest)) {
- // is the proposed trade reducing open interest?
- bool isReducingOpenInterest = currentOpenInterest.gt(newOpenInterest);
- // revert if the proposed trade isn't reducing open interest
- if (!isReducingOpenInterest) {
- revert Errors.ExceedsOpenInterestLimit(
- self.id, maxOpenInterest.intoUint256(), newOpenInterest.intoUint256()
- );
- }
- }
// load max & current skew for this perp market uint128 -> UD60x18 -> SD59x18
SD59x18 maxSkew = ud60x18(self.configuration.maxSkew).intoSD59x18();
// int128 -> SD59x18
SD59x18 currentSkew = sd59x18(self.skew);
// calculate new skew
newSkew = currentSkew.add(sizeDelta);
- // similar logic to the open interest check; if the new skew is greater than
- // the max, we still want to allow trades as long as they decrease the skew
- if (newSkew.abs().gt(maxSkew)) {
- bool isReducingSkew = currentSkew.abs().gt(newSkew.abs());
- if (!isReducingSkew) {
- revert Errors.ExceedsSkewLimit(self.id, maxSkew.intoUint256(), newSkew.intoInt256());
- }
- }
}
Updates
Lead Judging Commences
inallhonesty about 1 year ago
Submission Judgement Published Assigned finding tags:
`liquidateAccounts` calls `updateOpenInterest` with uninitialized OI and skew)
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.