Summary
An account can sidestep being liquidated, even if they are non-healthy which leaves protocol with bad debts.
Vulnerability Details
Inorder to liquidate the liquidateAccounts() is queried by accepted liquidators.
So as not to liquidate users that are safe not liquidatable LTV wise, a check is being implemented, see https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L127-L148
function liquidateAccounts(uint128[] calldata accountsIds) external {
..snip
for (uint256 i; i < accountsIds.length; i++) {
ctx.tradingAccountId = accountsIds[i];
if (ctx.tradingAccountId == 0) continue;
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(ctx.tradingAccountId);
(, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
ctx.marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
if (!TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18)) {
continue;
..snip
}
Now see https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L342-L352
function isLiquidatable(
UD60x18 requiredMaintenanceMarginUsdX18,
SD59x18 marginBalanceUsdX18
)
internal
pure
returns (bool)
{
return requiredMaintenanceMarginUsdX18.intoSD59x18().gt(marginBalanceUsdX18);
}
Thsi is the function that checks if the account is liquidatable and to do this the requiredMaintenanceMarginUsdX18 & marginBalanceUsdX18 need to be provided and queried from both tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd() & tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18).
Issue however is that both these functions need to undergo a heavy loop that eats up gas, which is asides other aspects of the liquidation process.
Here is getAccountMarginRequirementUsdAndUnrealizedPnlUsd() https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L210-L304
function getAccountMarginRequirementUsdAndUnrealizedPnlUsd(
Data storage self,
uint128 targetMarketId,
SD59x18 sizeDeltaX18
)
internal
view
returns (
UD60x18 requiredInitialMarginUsdX18,
UD60x18 requiredMaintenanceMarginUsdX18,
SD59x18 accountTotalUnrealizedPnlUsdX18
)
{
..snip
uint256 cachedActiveMarketsIdsLength = self.activeMarketsIds.length();
for (uint256 i; i < cachedActiveMarketsIdsLength; i++) {
uint128 marketId = self.activeMarketsIds.at(i).toUint128();
if (marketId == targetMarketId) {
continue;
}
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
Position.Data storage position = Position.load(self.id, marketId);
UD60x18 markPrice = perpMarket.getMarkPrice(sd59x18(-position.size), perpMarket.getIndexPrice());
SD59x18 fundingFeePerUnit =
perpMarket.getNextFundingFeePerUnit(perpMarket.getCurrentFundingRate(), markPrice);
UD60x18 notionalValueX18 = position.getNotionalValue(markPrice);
(UD60x18 positionInitialMarginUsdX18, UD60x18 positionMaintenanceMarginUsdX18) = Position
.getMarginRequirement(
notionalValueX18,
ud60x18(perpMarket.configuration.initialMarginRateX18),
ud60x18(perpMarket.configuration.maintenanceMarginRateX18)
);
SD59x18 positionUnrealizedPnl =
position.getUnrealizedPnl(markPrice).add(position.getAccruedFunding(fundingFeePerUnit));
requiredInitialMarginUsdX18 = requiredInitialMarginUsdX18.add(positionInitialMarginUsdX18);
requiredMaintenanceMarginUsdX18 = requiredMaintenanceMarginUsdX18.add(positionMaintenanceMarginUsdX18);
accountTotalUnrealizedPnlUsdX18 = accountTotalUnrealizedPnlUsdX18.add(positionUnrealizedPnl);
}
.snip
}
In the same execution a call is still made to getMarginBalanceUsd()https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L169-L201
function getMarginBalanceUsd(
Data storage self,
SD59x18 activePositionsUnrealizedPnlUsdX18
)
internal
view
returns (SD59x18 marginBalanceUsdX18)
{
uint256 cachedMarginCollateralBalanceLength = self.marginCollateralBalanceX18.length();
for (uint256 i; i < cachedMarginCollateralBalanceLength; i++) {
(address collateralType, uint256 balance) = self.marginCollateralBalanceX18.at(i);
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
UD60x18 adjustedBalanceUsdX18 = marginCollateralConfiguration.getPrice().mul(ud60x18(balance)).mul(
ud60x18(marginCollateralConfiguration.loanToValue)
);
marginBalanceUsdX18 = marginBalanceUsdX18.add(adjustedBalanceUsdX18.intoSD59x18());
}
marginBalanceUsdX18 = marginBalanceUsdX18.add(activePositionsUnrealizedPnlUsdX18);
}
This then allows a malicious user to open countless positions in different markets and also deposit all collateral possible to ensure the attempt at liquidating them reverts with an OOG error.
Impact
As hinted under Vulnerability Details, a malicious tech savvy user can have their accounts sidestep liquidations by opening countless positions in different markets and also deposit all collateral possible to ensure the attempt at liquidating them reverts with an OOG error, due to how heavy the gas cost of the execution would be, essentially leaving the protocol with bad debts and making risk free decisions in-protocol or elsewhere.
Tools Used
Manual review
Recommendations
Consider having a max amount of markets an account can integrate with.
