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

Decreasing the size of a liquidatable position

fyamf

Summary

It is possible to decrease the position size of a liquidatable account (marginBalance < requiredMaintenanceMargin ). This is not expected by the protocol.

In other words the following code does not properly disallow decrease of a liquidatable position:

{
// get account's current required margin maintenance (before this trade)
(, ctx.previousRequiredMaintenanceMarginUsdX18,) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
// prevent liquidatable accounts from trading
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
}

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L128-L137

Vulnerability Details

When decreasing size of a liquidatable position, it checks that the position is liquidatable or not in the following code:

// use unrealized PNL to calculate & output account's margin balance
marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(ctx.accountTotalUnrealizedPnlUsdX18);
{
// get account's current required margin maintenance (before this trade)
(, ctx.previousRequiredMaintenanceMarginUsdX18,) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
// prevent liquidatable accounts from trading
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
}

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L126

To calculate previousRequiredMaintenanceMarginUsdX18, it assumes that the entire position is closed. By this assumption, the mark price is calculated, let's name it markPriceA.

// calculate price impact as if trader were to close the entire position
UD60x18 markPrice = perpMarket.getMarkPrice(sd59x18(-position.size), perpMarket.getIndexPrice());

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L278

Moreover, another mark price is playing a role which is calculated based on the size delta (the amount of size that is going to be decreased from the position). It is calculated in the following code, let's call it markPriceB.

// calculate & output execution price
fillPriceX18 = perpMarket.getMarkPrice(ctx.sizeDeltaX18, perpMarket.getIndexPrice());

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L113

The issue is that although the protocol disallows liquidatable accounts from decreasing their position, it can be done.
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L133

The root cause of this issue is that, in case of a long liquidatable positions, the mark price used to calculate previousRequiredMaintenanceMarginUsdX18 is lower than the mark price used to calculate marginBalanceUsdX18. I.e. markPriceB > markPriceA. This is becasue the markPriceA is calculated based on assumption that the entire position is closed, while the markPriceB is calculated based on the size that is going to be decreased.

As a result, when the accountTotalUnrealizedPnlUsdX18 is calculated, it subtracts the lastInteractionPrice from the mark price. Since markPriceB is higher than markPriceA, the absolute value of accountTotalUnrealizedPnlUsdX18 associated with markPriceB is lower than the absolute value of accountTotalUnrealizedPnlUsdX18 associated with markPriceA. Note that since it is a long position, and we are decreasing the size, lastInteractionPrice > mark price.

For the case of a short liquidatable positions, the mark price used to calculate previousRequiredMaintenanceMarginUsdX18 is higher than the mark price used to calculate marginBalanceUsdX18. And the same scenario explained above is valid for short positions as well.

It can be seen from another point of view that the marginBalanceUsdX18 which is compared with previousRequiredMaintenanceMarginUsdX18 is calculated after the impact of size delta. I.e. previous marginBalanceUsdX18 should be compared with previousRequiredMaintenanceMarginUsdX18, not marginBalanceUsdX18 after the impact of size delta.

Example

Very simply let's give a example before the PoC. Note that the numbers are simplified for better understanding of the issue. The complete PoC includes the real numbers.

Suppose:

  • ETH price = 1000$

  • Collateral value = 1_000_000$

  • position size = 80k

  • markPrice:

skewScale = 10000000e18
priceImpaceBeforeDelta = 0/skewScale = 0
priceImpaceAfterDelta = 80k * 1e18 / skewScale = 0.008e18
priceBeforeDelta = 1000 (1 + 0) = 1000$
priceAfterDelta = 1000 (1 + 0.008) = 1008$
markPrice = (1000 + 1008) / 2 = 1004$

So, the fill price would be 1004$.

Then, suppose the price of ETH decreases by 8$, so the index price would be 992$. This position is liquidatable based on the function checkLiquidatableAccounts where it assumes that the entire position is closed when calculating the mark price.
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/LiquidationBranch.sol#L42

skewScale = 10000000e18
priceImpaceBeforeDelta = 80k * 1e18 / skewScale = 0.008e18
priceImpaceAfterDelta = (80k - 80k) * 1e18 / skewScale = 0 //// it assumes that the entire position is closed
priceBeforeDelta = 992 (1 + 0.008) = 999.936$
priceAfterDelta = 992 (1 + 0) = 992$
markPriceA = (999.936 + 992) / 2 = 995.968$
​
MMR = 0.005 * 80k * 995.968 = 398.387k //// this it he maintenance margin
​
marginBalance @ markPriceA = 1_000_000 - 80k * (1004 - 995.968) = 357.440k < 398.387k //// ignoring the fees for simpler calculations
​
marginBalance @ markPriceA < MMR ==> liquidatable

If the user creates an order to decrease its liquidatable position, it is expected that it will be reverted. Because, the protocol intends to disallow any liquidatable position from being decreased. But, this is not implemented properly.

Suppose the user intends to reduce its liquidatable position by 50k.

skewScale = 10000000e18
priceImpaceBeforeDelta = 80k * 1e18 / 10000000e18 = 0.008e18
priceImpaceAfterDelta = (80k - 50k) * 1e18 / skewScale = 0.003e18 //// 50k size is going to be decreased from the position
priceBeforeDelta = 992 (1 + 0.008) = 999.936$
priceAfterDelta = 992 (1 + 0.003) = 994.976$ //// note that this value is bigger than 992 which is calculated above
markPriceB = (999.936 + 994.976) / 2 = 997.456$ //// markPriceB > markPriceA
​
marginBalance @ markPriceB = 1_000_000 - 80k * (1004 - 997.456) = 476.480k > 398.387k //// ignoring the fees for simpler calculations
​
marginBalance @ markPriceB > MMR ==> it allows to decrease the position although it was liquidatable in the previous calculation

PoC

In the following test, the account's collateral value is 1M, and the position created is 80k. After the ETH price is decreased from 1000$ to 992$, the account is liquidatable. Then the account creates an order to decrease size by 50k. It is expected to revert, because the account is liquidatable, but it does not revert due to the bug explained. So, the account position is decreased, and afterwards it is not liquidatable any more. Note that if the account decreases its position by small amount 1k, then decreasing position is still allowed, but after the reduction still the account is liquidatable.

Please note that the collateral value is the large number 1M. This is not necessary for validity of this vulnerability. For example, assuming to have just a collateral of 1000$, and long position size 90, then if ETH price decreases by 5.319$, the account becomes liquidatable but it is still possible to reduce the position size by 10.

function test_userCanDecreaseALiquidatablePosition()
external
givenTheSenderIsTheKeeper
givenTheMarketOrderExists
givenThePerpMarketIsEnabled
givenTheSettlementStrategyIsEnabled
givenTheReportVerificationPasses
whenTheMarketOrderIdMatches
givenTheDataStreamsReportIsValid
givenTheAccountWillMeetTheMarginRequirement
givenTheMarketsOILimitWontBeExceeded
{
TestFuzz_GivenThePnlIsPositive_Context memory ctx;
ctx.fuzzMarketConfig = getFuzzMarketConfig(ETH_USD_MARKET_ID);
​
uint256 marginValueUsd = 1_000_000e18; // 1M$
​
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
​
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
​
ctx.tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdz));
​
// Creating the first order
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(80_000e18)
})
);
​
// Filling the first order
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice);
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
​
// Price decreases
updateMockPriceFeed(ctx.fuzzMarketConfig.marketId, (1000 - 8) * 1e18);
​
// Checking the account is liquidatable or not
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
console.log("account id: ", ctx.tradingAccountId);
console.log("liquidatableAccountsIds: ", liquidatableAccountsIds[0]); // so the account is liquidatable
​
changePrank({ msgSender: users.naruto.account });
​
// Creating the second order
// It is expected to revert because the account is liquidatable, but it does not
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(-50_000e18) // decreasing 50k size
})
);
​
// Filling the second order
changePrank({ msgSender: ctx.marketOrderKeeper });
ctx.firstMockSignedReport = getMockedSignedReport(ctx.fuzzMarketConfig.streamId, (1000 - 8) * 1e18);
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
​
// Checking the account is liquidatable or not
liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
console.log("liquidatableAccountsIds: ", liquidatableAccountsIds[0]);
}

Impact

  • Liquidatable positions can reduce their position size against the protocol expectation

Tools Used

Recommendations

The following modification is recommended:

{
// get account's current required margin maintenance (before this trade)
SD59x18 accountTotalUnrealizedPnlUsdX18;
(, ctx.previousRequiredMaintenanceMarginUsdX18, accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
// prevent liquidatable accounts from trading
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18))) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
}

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L128-L137

Updates

Lead Judging Commences

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

getAccountMarginRequirementUsdAndUnrealizedPnlUsd function returns incorrect margin requirement values when a position is being changed

Support

FAQs

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