Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Perpetuals::Liquidate function unfairly liquidates all of an account positions when in reality its possible to save some healthy positions

0xswahili

Summary

The Perpetuals::Liquidate function liquidates all of an account positions and seizes their collateral.I have found that this is not necessary and some positions can be spared and still laeve the account in a healty position.

Vulnerability Details

The Perpetuals::Liquidate function liquidates all of an account positions and seizes their collateral:

function liquidateAccounts(uint128[] calldata accountsIds) external {
...
//for each account
for (uint256 i; i < accountsIds.length; i++) {
​
//get all an accounts positions
for (uint256 j; j < ctx.activeMarketsIds.length; j++) {
//liquidate position
}
}
}

The problem is that this is not necessary since its possible to only close some positions and still leave the account healty. Consider a case where a trading account has 2 positions- a small position and a large position. Instead of closing the 2 positions, it is possible to only close 1 position and still leave the account healty.

For a POC, add this comment to src/perpetuals/branches/LiquidationBranch.sol::Ln177 at the start of the activeMarketsIds loop:

// get account's required maintenance margin & unrealized PNL
(, ctx.requiredMaintenanceMarginUsdX18, ctx.accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
// get then save margin balance into working data
ctx.marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(ctx.accountTotalUnrealizedPnlUsdX18);
// account is liquidatable if requiredMaintenanceMarginUsdX18 > ctx.marginBalanceUsdX18
bool isAccountLiquidatable = TradingAccount.isLiquidatable(
ctx.requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18, ctx.liquidationFeeUsdX18
);
console.log(isAccountLiquidatable);

Add this test to test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol:

​
function testLiquidateOneOff()
external
givenTheSenderIsARegisteredLiquidator
whenTheAccountsIdsArrayIsNotEmpty
givenAllAccountsExist
{
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(1);
MarketConfig memory secondMarketConfig = getFuzzMarketConfig(2);
uint256 initialMarginRate = fuzzMarketConfig.imr;
uint256 marginValueUsd = 5_000e18;
bool isLong = true;
uint256 timeDelta = 200 seconds;
deal({ token: address(usdToken), to: users.naruto.account, give: marginValueUsd });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdToken));
​
openPosition(
fuzzMarketConfig,
tradingAccountId,
fuzzMarketConfig.imr,
2_000e18,
isLong
);
​
openPosition(
secondMarketConfig,
tradingAccountId,
secondMarketConfig.imr,
3_000e18,
isLong
);
​
setAccountsAsLiquidatable(fuzzMarketConfig, isLong);
setAccountsAsLiquidatable(secondMarketConfig, isLong);
changePrank({ msgSender: liquidationKeeper });
​
uint128[] memory liquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(1, 2);
perpsEngine.liquidateAccounts(liquidatableAccountIds);
}

Run the test as:

forge test --match-test testLiquidateOneOff -vvv

The test shows that after the first position is liquidated, the account returns to a healthy state and should not be liquidated. Thi could have spared the second position.

Impact

Closing all an acccount positions unfairly seizes all their collateral and denies traders opportunity to rescue their accounts.

Tools Used

Manual review

Recommendations

Consider checking the accounts liquidation status before closing a position in order to avoid undue loss.

Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!