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

`LiquidationBranch.liquidateAccounts` should deduct `accountTotalUnrealizedPnlUsdX18.abs()` instead of `requiredMaintenanceMarginUsdX18` from the collateral.

qpzm

Summary

LiquidationBranch.liquidateAccounts should deduct accountTotalUnrealizedPnlUsdX18.abs() instead of requiredMaintenanceMarginUsdX18 from the collateral.

Vulnerability Details

A liquidation should decrease the account's unrealized loss from the collateral. However, the current implementation deducts the required maintenance margin from the collateral.

POC

Procedure

  1. Account id 1 deposits 2500 USDz

  2. Account id 1 opens a long position with the size of 5 ETH at the price of $2000.

    1. requiredInitialMarginUsdX18: 5000.0012500000000000e18

    2. requiredMaintenanceMarginUsdX18: 2500.0006250000000000e18

  3. The price drops to $502.

  4. Account id 1 is liquidated but the margin balance decreases from $2500 to $2476.

How to run

  1. Create this file to test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccountsPOC.t.sol.

  2. Run forge test --mt testPOC_liquidateAccounts.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.25;
​
// Zaros dependencies
import { Errors } from "@zaros/utils/Errors.sol";
import { LiquidationBranch } from "@zaros/perpetuals/branches/LiquidationBranch.sol";
import { MarketOrder } from "@zaros/perpetuals/leaves/MarketOrder.sol";
import { Position } from "@zaros/perpetuals/leaves/Position.sol";
import { Base_Test } from "test/Base.t.sol";
import { PerpMarket } from "@zaros/perpetuals/leaves/PerpMarket.sol";
import { OrderBranch } from "@zaros/perpetuals/branches/OrderBranch.sol";
import { TradingAccountHarness } from "test/harnesses/perpetuals/leaves/TradingAccountHarness.sol";
import { GlobalConfigurationHarness } from "test/harnesses/perpetuals/leaves/GlobalConfigurationHarness.sol";
import { PerpMarketHarness } from "test/harnesses/perpetuals/leaves/PerpMarketHarness.sol";
import { PositionHarness } from "test/harnesses/perpetuals/leaves/PositionHarness.sol";
​
// PRB Math dependencies
import { UD60x18 } from "@prb-math/UD60x18.sol";
import { SD59x18, sd59x18 } from "@prb-math/SD59x18.sol";
​
contract LiquidateAccounts_POC_Integration_Test is Base_Test {
function setUp() public override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
configureSystemParameters();
createPerpMarkets();
changePrank({ msgSender: users.naruto.account });
}
​
struct TestFuzz_GivenThereAreLiquidatableAccountsInTheArray_Context {
MarketConfig fuzzMarketConfig;
MarketConfig secondMarketConfig;
uint256 marginValueUsd;
uint256 initialMarginRate;
uint128[] accountsIds;
uint256 accountMarginValueUsd;
uint128 tradingAccountId;
PerpMarket.Data perpMarketData;
int256 expectedLastFundingRate;
int256 expectedLastFundingFeePerUnit;
uint256 expectedLastFundingTime;
uint128 nonLiquidatableTradingAccountId;
MarketOrder.Data marketOrder;
Position.Data expectedPosition;
Position.Data position;
UD60x18 openInterestX18;
uint256 expectedOpenInterest;
SD59x18 skewX18;
int256 expectedSkew;
}
​
function simpleOpenPosition(
MarketConfig memory fuzzMarketConfig,
uint128 tradingAccountId,
uint256 initialMarginRate,
uint256 marginValueUsd,
bool isLong
)
internal
{
address marketOrderKeeper = marketOrderKeepers[fuzzMarketConfig.marketId];
​
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
​
int128 sizeDelta = 5e18;
​
// first market order
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: fuzzMarketConfig.marketId,
sizeDelta: sizeDelta
})
);
​
bytes memory mockSignedReport =
getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
​
changePrank({ msgSender: marketOrderKeeper });
​
// fill first order and open position
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
​
changePrank({ msgSender: users.naruto.account });
}
​
function testPOC_liquidateAccounts() external {
TestFuzz_GivenThereAreLiquidatableAccountsInTheArray_Context memory ctx;
​
uint128 marketId = uint128(INITIAL_MARKET_ID) + 1;
bool isLong = true;
uint256 amountOfTradingAccounts = 2;
uint256 timeDelta = 1 days;
ctx.fuzzMarketConfig = getFuzzMarketConfig(marketId);
​
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);
​
// @audit accountMarginValueUsd = 2500e18
ctx.accountMarginValueUsd = ctx.marginValueUsd / (amountOfTradingAccounts);
​
for (uint256 i; i < amountOfTradingAccounts; i++) {
ctx.tradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
​
simpleOpenPosition(
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 });
}
​
// @audit Decrease the price to make account1,2 liquidatable
uint256 newIndexPrice = 502e18;
updateMockPriceFeed(marketId, newIndexPrice);
​
ctx.nonLiquidatableTradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
ctx.accountsIds[amountOfTradingAccounts] = ctx.nonLiquidatableTradingAccountId;
​
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
});
}
​
uint128 accountId = ctx.accountsIds[0];
uint256 marginBalance = UD60x18.unwrap(TradingAccountHarness(address(perpsEngine)).exposed_getMarginCollateralBalance({
tradingAccountId: accountId,
collateralType: address(usdz)
}));
uint256 orderFee = 4000001000000000000;
uint256 settlementFee = 2000000000000000000;
assertEq(
UD60x18.unwrap(TradingAccountHarness(address(perpsEngine)).exposed_getMarginCollateralBalance({
tradingAccountId: accountId,
collateralType: address(usdz)
})),
2500e18 - orderFee - settlementFee, // 2493.999999e18,
"Wrong margin collateral balance before liquidation"
);
​
skip(timeDelta);
​
ctx.expectedLastFundingRate = perpsEngine.getFundingRate(ctx.fuzzMarketConfig.marketId).intoInt256();
ctx.expectedLastFundingTime = block.timestamp;
​
perpsEngine.liquidateAccounts(ctx.accountsIds);
​
// @audit account id 1 is liquidated but the margin balance decreases from $2500 to $2476.
assertEq(
UD60x18.unwrap(TradingAccountHarness(address(perpsEngine)).exposed_getMarginCollateralBalance({
tradingAccountId: accountId,
collateralType: address(usdz)
}))
, 2476.4499895875e18,
"Wrong margin collateral balance after liquidation"
);
}
}

Impact

A liquidation does not give a proper amount of profit to LP.

Tools Used

Manual, foundry

Recommendations

- pnlUsdX18: requiredMaintenanceMarginUsdX18,
+ pnlUsdX18: accountTotalUnrealizedPnlUsdX18.abs().intoUD60x18(),

https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/branches/LiquidationBranch.sol#L158

Updates

Lead Judging Commences

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

deductAccountMargin() treats `required maintenance margin` as the `unrealized PnL` because it uses `requiredMaintenanceMarginUsdX18` in place of `pnlUsdX18`

Support

FAQs

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