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

After `LiquidationBranch::liquidateAccounts()`, the `skew` and `openInterest` of the market will become 0

crypt0n

Summary

LiquidationBranch::liquidateAccounts() will update market's skew and openInterest. However, at this step both ctx.newOpenInterestX18 and ctx.newSkewX18 are haven't been initialized, which leaves them to 0.

Vulnerability Details

ctx.newOpenInterestX18 and ctx.newSkewX18 should have been the original values minus those of the liquidated accounts like what they do in here.In fact this step is missed according to the comment we don't enforce open interest and skew caps.

There is another thing I would like to point out is that the developer do want to check the skew and openInterest are handled properly here.They create a nonLiquidatableTradingAccountId and want to make sure that after liquidation the skew and openInterest contributed by nonLiquidatableTradingAccount stays.But they forgot to open a position for that nonLiquidatableTradingAccount, which makes the nonLiquidatableTradingAccount's size is 0, and that happens to make the check passed because 0=0.

To prove the issue stands, we only need to add something in testFuzz_GivenThereAreLiquidatableAccountsInTheArray() as below:

openPosition(getFuzzMarketConfig(ctx.nonLiquidatableTradingAccountId),ctx.nonLiquidatableTradingAccountId,getFuzzMarketConfig(ctx.nonLiquidatableTradingAccountId).imr,ctx.accountMarginValueUsd / 2,isLong);

The whole POC is here, put it into test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol and then run forge test --match-path test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol --match-test testFuzz_LiqudateWillMakeOiAndSkewto0 -vvvvv

function testFuzz_LiqudateWillMakeOiAndSkewto0(
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));
ctx.accountsIds[amountOfTradingAccounts] = ctx.nonLiquidatableTradingAccountId;
openPosition(
getFuzzMarketConfig(ctx.nonLiquidatableTradingAccountId),
ctx.nonLiquidatableTradingAccountId,
getFuzzMarketConfig(ctx.nonLiquidatableTradingAccountId).imr,
ctx.accountMarginValueUsd / 2,
isLong
);
changePrank({ msgSender: liquidationKeeper });
​
for (uint256 i; i < ctx.accountsIds.length; i++) {
if (ctx.accountsIds[i] == ctx.nonLiquidatableTradingAccountId || ctx.accountsIds[i] == 0) {
continue;
}
​
// it should emit a {LogLiquidateAccount} event
vm.expectEmit({
checkTopic1: true,
checkTopic2: true,
checkTopic3: false,
checkData: false,
emitter: address(perpsEngine)
});
​
emit LiquidationBranch.LogLiquidateAccount({
keeper: liquidationKeeper,
tradingAccountId: ctx.accountsIds[i],
amountOfOpenPositions: 0,
requiredMaintenanceMarginUsd: 0,
marginBalanceUsd: 0,
liquidatedCollateralUsd: 0,
liquidationFeeUsd: 0
});
}
​
skip(timeDelta);
​
ctx.expectedLastFundingRate = perpsEngine.getFundingRate(ctx.fuzzMarketConfig.marketId).intoInt256();
ctx.expectedLastFundingTime = block.timestamp;
​
perpsEngine.liquidateAccounts(ctx.accountsIds);
​
// it should update the market's funding values
ctx.perpMarketData =
PerpMarketHarness(address(perpsEngine)).exposed_PerpMarket_load(ctx.fuzzMarketConfig.marketId);
assertEq(ctx.expectedLastFundingRate, ctx.perpMarketData.lastFundingRate, "last funding rate");
assertEq(ctx.expectedLastFundingTime, ctx.perpMarketData.lastFundingTime, "last funding time");
​
// it should update open interest value
(,, 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();
console2.log("oi expected: ", ctx.expectedOpenInterest);
console2.log("oi actually: ", ctx.openInterestX18.intoUint256());
assertEq(ctx.expectedOpenInterest, ctx.openInterestX18.intoUint256(), "open interest");
​
// it should update skew value
ctx.skewX18 = perpsEngine.getSkew(ctx.fuzzMarketConfig.marketId);
ctx.expectedSkew = PositionHarness(address(perpsEngine)).exposed_Position_load(
ctx.nonLiquidatableTradingAccountId, ctx.fuzzMarketConfig.marketId
).size;
assertEq(ctx.expectedSkew, ctx.skewX18.intoInt256(), "skew");

part of the result as follows:

├─ [0] console::log("oi expected: ", 3857704513096891611 [3.857e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("oi actually: ", 0) [staticcall]

Impact

The skew and openinterest are crucial parameters of a market.Especially the skew. According to the doc, the project use a special way to calculate mark price based on the skew. This value represent how many size the long and short have been opened in the market and if every liquidation event which happens all the time in any markets, will make the two values become 0.Then the whole functionality of the contract will be broken and any calculations based on this will go wrong which makes this issue as high.

Tools Used

Manually Review

Recommendations

Ensure that ctx.newOpenInterestX18 and ctx.newSkewX18 are correctly initialized and assigned.

Updates

Lead Judging Commences

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

`liquidateAccounts` calls `updateOpenInterest` with uninitialized OI and skew)

Support

FAQs

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

Give us feedback!