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

Executing `LiquidationBranch.liquidateAccounts()` sets value of `openInterest` and `skew` to 0.

baz1ka

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L209

Summary

During the execution of LiquidationBranch.liquidateAccounts() function, it interacts with perp market storage to update values depending on the liquidated user position. One of the updates is perpMarket.updateOpenInterest() which should update current market open interest and skew. Values of newOpenInterestX18 and newSkewX18 are stored in ctx variable which is LiquidationContext struct. But these values are not set to ctx, and because of that to perpMarket.updateOpenInterest() as values of newOpenInterestX18 and newSkewX18 variables are passed default values which are 0. As a result open interest and skew for the current market are 0.

Impact

Open interest and skew are used in every market-changing function like filling orders or liquidating accounts. Incorrect values of their variables would result in incorrect calculation of funding rate (PerpMarket.getCurrentFundingRate(), PerpMarket.getCurrentFundingVelocity) and user fees (SettlementBranch._fillOrder()).

Proof of Concept

Function LiquidationBranch.liquidateAccounts() in this line calls perpMarket.updateOpenInterest():

// 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);
}

But values ctx.newOpenInterestX18 and ctx.newSkewX18 are not set in the function.

Coded PoC

Modify test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol test with next code and execute this test with forge test --match-test testFuzz_OpenInterestAndSkewAreResettedAfterLiquidation:

function testFuzz_OpenInterestAndSkewAreResettedAfterLiquidation(uint256 marketId, uint256 amountOfHealthyPositions) external {
amountOfHealthyPositions = bound(amountOfHealthyPositions, 1, 10);
​
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
uint256 marginValueUsd = amountOfHealthyPositions * 5_000e18;
uint256 initialMarginRate = fuzzMarketConfig.imr;
uint256 accountMarginValueUsd = marginValueUsd / 2;
​
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
​
for (uint256 i = 0; i < amountOfHealthyPositions; i++) {
uint128 account = createAccountAndDeposit(accountMarginValueUsd, address(usdz));
openPosition(fuzzMarketConfig, account, initialMarginRate, accountMarginValueUsd, false);
}
​
uint128 liquidatableAccount = createAccountAndDeposit(accountMarginValueUsd, address(usdz));
openPosition(fuzzMarketConfig, liquidatableAccount, initialMarginRate, accountMarginValueUsd, true);
​
setAccountsAsLiquidatable(fuzzMarketConfig, true); // Make long positions liquidatable
​
uint128[] memory accountsIds = new uint128[](1);
accountsIds[0] = liquidatableAccount;
​
changePrank({ msgSender: liquidationKeeper });
perpsEngine.liquidateAccounts(accountsIds);
​
SD59x18 skew = perpsEngine.getSkew(uint128(marketId));
(,, UD60x18 openInterestX18) = perpsEngine.getOpenInterest(fuzzMarketConfig.marketId);
​
// Open interest and skew are 0
assertEq(openInterestX18.intoUint128(), 0);
assertEq(skew.intoInt256(), 0);
}

Recommended Mitigation Steps

Not sure what update of open interest and skew was intended, but looking by SettlementBranch._fillOrder() I can believe this mitigation is what it should be:

@@ -16,7 +16,7 @@ import { SafeCast } from "@openzeppelin/utils/math/SafeCast.sol";
// PRB Math dependencies
import { UD60x18, ud60x18, ZERO as UD60x18_ZERO } from "@prb-math/UD60x18.sol";
-import { SD59x18, sd59x18, ZERO as SD59x18_ZERO } from "@prb-math/SD59x18.sol";
+import { SD59x18, sd59x18, unary, ZERO as SD59x18_ZERO } from "@prb-math/SD59x18.sol";
​
@@ -206,6 +213,8 @@ contract LiquidationBranch {
// 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
+ (ctx.newOpenInterestX18, ctx.newSkewX18) =
+ perpMarket.checkOpenInterestLimits(unary(ctx.oldPositionSizeX18), ctx.oldPositionSizeX18, SD59x18_ZERO);
perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18);
}
Updates

Lead Judging Commences

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