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

`LiquidationBranch::liquidateAccounts()` executed `TradingAccount::deductAccountMargin()` with incorrect parameters, causing the project party to lose a large amount of funds.

joicygiore

Summary

LiquidationBranch::liquidateAccounts() executed TradingAccount::deductAccountMargin() with incorrect parameters, causing the project party to lose a large amount of funds.

Vulnerability Details

During the execution of LiquidationBranch::liquidateAccounts(), requiredMaintenanceMarginUsdX18 was used as pnlUsdX18 to call TradingAccount::deductAccountMargin(). Since the value of requiredMaintenanceMarginUsdX18 is basically constant, it cannot reflect the actual loss of the user's account. No matter how much the user's actual loss is, in the end, only the funds corresponding to requiredMaintenanceMarginUsdX18 will be deducted, which will cause huge losses to the project party.

// LiquidationBranch::liquidateAccounts()
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()
);
}
}

Poc

Please add the test code to test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol and execute

import { MockPriceFeed } from "test/mocks/MockPriceFeed.sol";
import {console2,console} from "forge-std/Test.sol";
import { unwrap } from "@prb-math/SD59x18.sol";
import { unwrap } from "@prb-math/UD60x18.sol";
function testLiquidationBranch_liquidateAccounts_error() public {
TestFuzz_GivenThereAreLiquidatableAccountsInTheArray_Context memory ctx;
​
// Get the market configuration for the test
ctx.fuzzMarketConfig = getFuzzMarketConfig(0);
// ctx.secondMarketConfig = getFuzzMarketConfig(0);
​
// We only use one account for testing
uint256 amountOfTradingAccounts = 1;
​
uint256 timeDelta = 1 days;
// Long
bool isLong = true;
​
ctx.marginValueUsd = 10_000e18;
ctx.initialMarginRate = ctx.fuzzMarketConfig.imr;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
ctx.accountsIds = new uint128[](amountOfTradingAccounts);
ctx.accountMarginValueUsd = ctx.marginValueUsd;
​
// Get the current oracle quote
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 deposit token
ctx.tradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
​
console.log("---------- Account status before transaction ----------");
// The total margin balance of the account and the available margin for withdrawal
(SD59x18 marginBalanceUsdX18, , ,SD59x18 availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(ctx.tradingAccountId);
console2.log("marginBalanceUsdX18:",unwrap(marginBalanceUsdX18));
console2.log("availableMarginUsdX18:",unwrap(availableMarginUsdX18));
​
// Opening a Position
openPosition(
ctx.fuzzMarketConfig,
ctx.tradingAccountId,
ctx.initialMarginRate,
ctx.marginValueUsd,
isLong
);
// Check account's balance
(marginBalanceUsdX18, , ,availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(ctx.tradingAccountId);
// The account's total margin balance
console2.log("marginBalanceUsdX18:",unwrap(marginBalanceUsdX18));
// The account's withdrawable margin balance
console2.log("availableMarginUsdX18:",unwrap(availableMarginUsdX18));
ctx.accountsIds[0] = ctx.tradingAccountId;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
​
console.log("---------- Initial Position Status ----------");
Position.State memory positionState = perpsEngine.getPositionState(ctx.tradingAccountId,ctx.fuzzMarketConfig.marketId,uint256(openPrice));
console2.log("positionState.notionalValueX18:",unwrap(positionState.notionalValueX18));
console2.log("positionState.initialMarginUsdX18:",unwrap(positionState.initialMarginUsdX18));
console2.log("positionState.maintenanceMarginUsdX18:",unwrap(positionState.maintenanceMarginUsdX18));
​
// The position direction is long. For better comparison results, we save the snapshot here and then compare the results of liquidating the target account with 3 decline rates of 0.6%, 0.8%, and 1%.
​
uint256 snapshot = vm.snapshot(); // saves the state
///////////////////////////////////
// The first price drop is 0.6% //
///////////////////////////////////
vm.revertTo(snapshot);
console.log("---------- The first price drop is 0.6% ----------");
// Adjust the price
priceFeed.updateMockPrice(uint256(openPrice * 9940 / 10000));
(, int256 answer, , , ) = priceFeed.latestRoundData();
console.log("The price of the first liquidation:",answer);
changePrank({ msgSender: liquidationKeeper });
skip(timeDelta);
ctx.expectedLastFundingRate = perpsEngine.getFundingRate(ctx.fuzzMarketConfig.marketId).intoInt256();
ctx.expectedLastFundingTime = block.timestamp;
// Unrealized profit or loss before the account is liquidated
console2.log("Account Total Unrealized Pnl:",unwrap(perpsEngine.getAccountTotalUnrealizedPnl(ctx.tradingAccountId)));
// Execute liquidation
perpsEngine.liquidateAccounts(ctx.accountsIds);
​
// Confirmation that liquidation is complete
positionState = perpsEngine.getPositionState(ctx.tradingAccountId,ctx.fuzzMarketConfig.marketId,uint256(openPrice));
assertEq(0,unwrap(positionState.notionalValueX18));
​
// Check account's balance
(marginBalanceUsdX18, , ,availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(ctx.tradingAccountId);
// The account's total margin balance
console2.log("marginBalanceUsdX18:",unwrap(marginBalanceUsdX18));
// The account's withdrawable margin balance
console2.log("availableMarginUsdX18:",unwrap(availableMarginUsdX18));
​
///////////////////////////////////
// The second price drop is 0.8% //
///////////////////////////////////
vm.revertTo(snapshot);
console.log("---------- The second price drop is 0.8% ----------");
// Adjust the price
priceFeed.updateMockPrice(uint256(openPrice * 9920 / 10000));
(, answer, , , ) = priceFeed.latestRoundData();
console.log("The price of the second liquidation:",answer);
​
changePrank({ msgSender: liquidationKeeper });
skip(timeDelta);
ctx.expectedLastFundingRate = perpsEngine.getFundingRate(ctx.fuzzMarketConfig.marketId).intoInt256();
ctx.expectedLastFundingTime = block.timestamp;
​
// Unrealized profit or loss before the account is liquidated
console2.log("Account Total Unrealized Pnl:",unwrap(perpsEngine.getAccountTotalUnrealizedPnl(ctx.tradingAccountId)));
// Execute liquidation
perpsEngine.liquidateAccounts(ctx.accountsIds);
// Confirmation that liquidation is complete
positionState = perpsEngine.getPositionState(ctx.tradingAccountId,ctx.fuzzMarketConfig.marketId,uint256(openPrice));
assertEq(0,unwrap(positionState.notionalValueX18));
​
// Check account's balance
(marginBalanceUsdX18, , ,availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(ctx.tradingAccountId);
// The account's total margin balance
console2.log("marginBalanceUsdX18:",unwrap(marginBalanceUsdX18));
// The account's withdrawable margin balance
console2.log("availableMarginUsdX18:",unwrap(availableMarginUsdX18));
​
/////////////////////////////////
// The third price drop is 1% //
/////////////////////////////////
vm.revertTo(snapshot);
console.log("---------- The third price drop is 1% ----------");
// Adjust the price
priceFeed.updateMockPrice(uint256(openPrice * 9900 / 10000));
(, answer, , , ) = priceFeed.latestRoundData();
console.log("The price of the third liquidation:",answer);
changePrank({ msgSender: liquidationKeeper });
skip(timeDelta);
ctx.expectedLastFundingRate = perpsEngine.getFundingRate(ctx.fuzzMarketConfig.marketId).intoInt256();
ctx.expectedLastFundingTime = block.timestamp;
​
// Unrealized profit or loss before the account is liquidated
console2.log("Account Total Unrealized Pnl:",unwrap(perpsEngine.getAccountTotalUnrealizedPnl(ctx.tradingAccountId)));
// Execute liquidation
perpsEngine.liquidateAccounts(ctx.accountsIds);
// Confirmation that liquidation is complete
positionState = perpsEngine.getPositionState(ctx.tradingAccountId,ctx.fuzzMarketConfig.marketId,uint256(openPrice));
assertEq(0,unwrap(positionState.notionalValueX18));
​
// Check account's balance
(marginBalanceUsdX18, , ,availableMarginUsdX18) = perpsEngine.getAccountMarginBreakdown(ctx.tradingAccountId);
// The account's total margin balance
console2.log("marginBalanceUsdX18:",unwrap(marginBalanceUsdX18));
// The account's withdrawable margin balance
console2.log("availableMarginUsdX18:",unwrap(availableMarginUsdX18));
}

The output is as follows. We compare the status before and after liquidation in three cases of decline. It can be found that no matter which state, due to the incorrect use of requiredMaintenanceMarginUsdX18, the actual amount liquidated is far less than the loss amount in the account, which will cause serious losses to the project's funds.
output:

Ran 1 test for test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol:LiquidateAccounts_Integration_Test
[PASS] testLiquidationBranch_liquidateAccounts_error() (gas: 2028826)
Logs:
The price at the time of opening the position is: 100000000000000000000000
---------- Account status before transaction ----------
marginBalanceUsdX18: 10000000000000000000000
availableMarginUsdX18: 10000000000000000000000
sd59x18(position.size) 0
changePrank is deprecated. Please use vm.startPrank instead.
changePrank is deprecated. Please use vm.startPrank instead.
marginBalanceUsdX18: 9262160061507351887025
availableMarginUsdX18: 64160830349250474826
---------- Initial Position Status ----------
positionState.notionalValueX18: 919799923115810141219901
positionState.initialMarginUsdX18: 9197999231158101412199
positionState.maintenanceMarginUsdX18: 4598999615579050706099
---------- The first price drop is 0.6% ----------
The price of the first liquidation: 99400000000000000000000
changePrank is deprecated. Please use vm.startPrank instead.
@> Account Total Unrealized Pnl: -5518812153024666646630
@> marginBalanceUsdX18: 4685754443621775485163
@> availableMarginUsdX18: 4685754443621775485163
---------- The second price drop is 0.8% ----------
The price of the second liquidation: 99200000000000000000000
changePrank is deprecated. Please use vm.startPrank instead.
@> Account Total Unrealized Pnl: -7358411973875341645769
@> marginBalanceUsdX18: 4694952442852933586575
@> availableMarginUsdX18: 4694952442852933586575
---------- The third price drop is 1% ----------
The price of the third liquidation: 99000000000000000000000
changePrank is deprecated. Please use vm.startPrank instead.
@> Account Total Unrealized Pnl: -9198011794726016644908
@> marginBalanceUsdX18: 4704150442084091687987
@> availableMarginUsdX18: 4704150442084091687987

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/TradingAccount.sol#L481-L578

Impact

LiquidationBranch::liquidateAccounts() executed TradingAccount::deductAccountMargin() with incorrect parameters, causing the project party to lose a large amount of funds.

Tools Used

Manual Review

Recommendations

Combined with the following comment of the TradingAccount::deductAccountMargin() function, we should consider using accountTotalUnrealizedPnlUsdX18 to replace requiredMaintenanceMarginUsdX18 as the calling parameter.

/// @notice Deducts the account's margin to pay for the settlement fee, order fee, and realize the pnl.
/// @param self The trading account storage pointer.
/// @param feeRecipients The fee recipients.
@> /// @param pnlUsdX18 The total unrealized PnL of the account.
/// @param settlementFeeUsdX18 The total settlement fee to be deducted from the account.
/// @param orderFeeUsdX18 The total order fee to be deducted from the account.
/// @return marginDeductedUsdX18 The total margin deducted from the account.
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
//SNIP...
// pnl logic same as settlement & order fee above
@> if (pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(pnlUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
feeRecipients.marginCollateralRecipient
);
ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
// SNIP...
// output total margin deducted
marginDeductedUsdX18 =
ctx.settlementFeeDeductedUsdX18.add(ctx.orderFeeDeductedUsdX18).add(ctx.pnlDeductedUsdX18);
}

For example:

  1. Check the unrealized profit and loss value

  2. Reverse the sign

  3. SD59x18 -> UD60x18

// The unrealized profit and loss of an account that can be liquidated should be expected to be less than 0. We still perform checks to ensure safety.
+ require(accountTotalUnrealizedPnlUsdX18.lt(SD59x18_ZERO),"accountTotalUnrealizedPnlUsdX18 error");
// Flip Symbol
+ accountTotalUnrealizedPnlUsdX18 = unary(accountTotalUnrealizedPnlUsdX18);
// 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,
+ pnlUsdX18: ud60x18(uint256(accountTotalUnrealizedPnlUsdX18.intoInt256())), // SD59x18 -> UD60x18
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});

test again,it's work

Ran 1 test for test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol:LiquidateAccounts_Integration_Test
[PASS] testLiquidationBranch_liquidateAccounts_error() (gas: 2050012)
The price at the time of opening the position is: 100000000000000000000000
---------- Account status before transaction ----------
marginBalanceUsdX18: 10000000000000000000000
availableMarginUsdX18: 10000000000000000000000
sd59x18(position.size) 0
changePrank is deprecated. Please use vm.startPrank instead.
changePrank is deprecated. Please use vm.startPrank instead.
marginBalanceUsdX18: 9262160061507351887025
availableMarginUsdX18: 64160830349250474826
---------- Initial Position Status ----------
positionState.notionalValueX18: 919799923115810141219901
positionState.initialMarginUsdX18: 9197999231158101412199
positionState.maintenanceMarginUsdX18: 4598999615579050706099
---------- The first price drop is 0.6% ----------
The price of the first liquidation: 99400000000000000000000
changePrank is deprecated. Please use vm.startPrank instead.
@> Account Total Unrealized Pnl: -5518812153024666646630
requiredMaintenanceMarginUsdX18 4571405617885576401862
ctx.marginBalanceUsdX18 3743347908482685240395
@> marginBalanceUsdX18: 3738347908482685240395
availableMarginUsdX18: 3738347908482685240395
# 3738347908482685240395 - -5518812153024666646630 = 9257160061507351887025
---------- The second price drop is 0.8% ----------
The price of the second liquidation: 99200000000000000000000
changePrank is deprecated. Please use vm.startPrank instead.
@> Account Total Unrealized Pnl: -7358411973875341645769
requiredMaintenanceMarginUsdX18 4562207618654418300450
ctx.marginBalanceUsdX18 1903748087632010241256
@> marginBalanceUsdX18: 1898748087632010241256
availableMarginUsdX18: 1898748087632010241256
# 1898748087632010241256 - -7358411973875341645769 = 9257160061507351887025
---------- The third price drop is 1% ----------
The price of the third liquidation: 99000000000000000000000
changePrank is deprecated. Please use vm.startPrank instead.
@> Account Total Unrealized Pnl: -9198011794726016644908
requiredMaintenanceMarginUsdX18 4553009619423260199038
ctx.marginBalanceUsdX18 64148266781335242117
@> marginBalanceUsdX18: 59148266781335242117
availableMarginUsdX18: 59148266781335242117
# 59148266781335242117 - -9198011794726016644908 = 9257160061507351887025
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.