Submission Details
Severity:
`LiquidationBranch::liquidateAccounts()` executes `PerpMarket::updateOpenInterest()` with incorrect parameters causing `PerpMarket::Data::skew` and `PerpMarket::Data::openInterest` to be reset to `0`
Summary
LiquidationBranch::liquidateAccounts() executes PerpMarket::updateOpenInterest() with incorrect parameters causing PerpMarket::Data::skew and PerpMarket::Data::openInterest to be reset to 0
Vulnerability Details
As the @> annotation indicates, LiquidationContext memory ctx; is used to save working data, but there is nothing in the code for the update of ctx.newOpenInterestX18 and ctx.newSkewX18, and finally we call perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18); with the default value 0.
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);
}
emit LogLiquidateAccount(
msg.sender,
ctx.tradingAccountId,
ctx.activeMarketsIds.length,
requiredMaintenanceMarginUsdX18.intoUint256(),
ctx.marginBalanceUsdX18.intoInt256(),
ctx.liquidatedCollateralUsdX18.intoUint256(),
ctx.liquidationFeeUsdX18.intoUint128()
);
}
}
Combined with the target PerpMarket::updateOpenInterest() method, the 0 value will be used to directly overwrite the PerpMarket::Data::skew and PerpMarket::Data::openInterest in the storage.
function updateOpenInterest(Data storage self, UD60x18 newOpenInterest, SD59x18 newSkew) internal {
self.skew = newSkew.intoInt256().toInt128();
self.openInterest = newOpenInterest.intoUint128();
}
Poc
We add the following test code to LiquidationBranch::liquidateAccounts()
// 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
+ console.log("ctx.newOpenInterestX18:",ctx.newOpenInterestX18.intoUint256());
+ console.log("ctx.newSkewX18:",ctx.newSkewX18.intoInt256());
perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18);
Please add the test code to test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol and execute
function testLiquidationBranch_liquidateAccounts_calls_updateOpenInterest() public {
TestFuzz_GivenThereAreLiquidatableAccountsInTheArray_Context memory ctx;
// Get the market configuration for the test
ctx.fuzzMarketConfig = getFuzzMarketConfig(1);
// ctx.secondMarketConfig = getFuzzMarketConfig(0);
// We only use one account for testing
uint256 amountOfTradingAccounts = 10;
uint256 timeDelta = 1 days;
// Long
bool isLong = true;
ctx.marginValueUsd = 10_000e18 / amountOfTradingAccounts;
ctx.initialMarginRate = ctx.fuzzMarketConfig.imr;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
ctx.accountsIds = new uint128[](amountOfTradingAccounts + 2);
ctx.accountMarginValueUsd = ctx.marginValueUsd / (amountOfTradingAccounts + 1);
MockPriceFeed priceFeed = MockPriceFeed(marketsConfig[1].priceAdapter);
(, int256 openPrice, , , ) = priceFeed.latestRoundData();
console.log("The price at the time of opening the position is:",openPrice);
// Create an account and open a position
for (uint256 i; i < amountOfTradingAccounts; i++) {
ctx.tradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
openPosition(
ctx.fuzzMarketConfig,
ctx.tradingAccountId,
ctx.initialMarginRate,
ctx.accountMarginValueUsd,
isLong
);
ctx.accountsIds[i] = ctx.tradingAccountId;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
}
// Adjust price to liquidate
priceFeed.updateMockPrice(uint256(openPrice * 9800 / 10000));
changePrank({ msgSender: liquidationKeeper });
skip(timeDelta);
ctx.expectedLastFundingRate = perpsEngine.getFundingRate(ctx.fuzzMarketConfig.marketId).intoInt256();
ctx.expectedLastFundingTime = block.timestamp;
// liquidation list
uint128[] memory accountsIds = new uint128[](2);
accountsIds[0] = ctx.accountsIds[0];
accountsIds[1] = ctx.accountsIds[1];
// Execute liquidation
perpsEngine.liquidateAccounts(accountsIds);
}
// Ran 1 test for test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol:LiquidateAccounts_Integration_Test
// [PASS] testLiquidationBranch_liquidateAccounts_calls_updateOpenInterest() (gas: 1427055)
// Logs:
// ctx.newOpenInterestX18: 0
// ctx.newSkewX18: 0
// ctx.newOpenInterestX18: 0
// ctx.newSkewX18: 0
From the output we can see that when perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18); is executed, the parameter is 0, which will directly reset the corresponding parameter in the storage.
Code Snippet
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L105-L222
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/PerpMarket.sol#L354-L357
Impact
LiquidationBranch::liquidateAccounts() executes PerpMarket::updateOpenInterest() with incorrect parameters causing PerpMarket::Data::skew and PerpMarket::Data::openInterest to be reset to 0
Tools Used
Manual Review
Recommendations
Refer to the following code snippet of SettlementBranch::_fillOrder():
// SettlementBranch::_fillOrder()
ctx.newPositionSizeX18 = sd59x18(ctx.newPosition.size);
// enforce open interest and skew limits for target market and calculate
// new open interest and new skew
(ctx.newOpenInterestX18, ctx.newSkewX18) =
perpMarket.checkOpenInterestLimits(sizeDeltaX18, ctx.oldPositionSizeX18, ctx.newPositionSizeX18);
// update open interest and skew for this perp market
perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18);
We may consider making the following changes:
// 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);
// calculate size delta
+ SD59x18 sizeDeltaX18 = ctx.liquidationSizeX18; // This should be -ctx.oldPositionSizeX18
// enforce open interest and skew limits for target market and calculate
// new open interest and new skew
+ (ctx.newOpenInterestX18, ctx.newSkewX18) =
+ perpMarket.checkOpenInterestLimits(sizeDeltaX18, 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);
test again,it's work.
Ran 1 test for test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol:LiquidateAccounts_Integration_Test
[PASS] testLiquidationBranch_liquidateAccounts_calls_updateOpenInterest() (gas: 8001930)
Logs:
ctx.newOpenInterestX18: 734727236428118203
ctx.newSkewX18: 734727236428118203
ctx.newOpenInterestX18: 653090873921708277
ctx.newSkewX18: 653090873921708277
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.