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

Position loss will not be deducted on update/liquidation of position if collateral type is removed from LiquidationPriorityList

Reentrants

Summary

The admin can remove collateral from the LiquidationPriority List at any time to rearrange the priority list. However, if a user deposits this collateral, creates a position, and then attempts to increase a position already in loss (while the admin has removed the collateral, perhaps temporarily), the fill order succeeds without deducting from the account margin. Likewise liquidation attempts as well will fail to deduct loss from margin due to the same reason.

Vulnerability Details

The LiquidationPriority List determines which collateral types are used first when deducting margin. This priority list ensures collateral with the lowest LTV is used first, maintaining the highest quality collateral for the user after deductions. The priority-based deduction logic is within the deductAccountMargin function:

https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/perpetuals/leaves/TradingAccount.sol#L481C1-L578C6

A problem arises if the admin removes a collateral type from the priority list via the removeCollateralFromLiquidationPriority function in the GlobalConfigurationBranch.sol. This removal can be for various reasons, including removing elements and readding them to rearrange priorities for instance.

The act of removing a collateral type from the priority list introduces this edge case:

  1. Bob deposits collateral A and executes a market order to buy 1 BTC. The order gets filled.

  2. The price of BTC falls slightly, but Bob is not yet liquidatable. However, he has a loss on his current position.

  3. The Admin executes a call to remove collateral A from the collateralLiquidationPriority.

  4. Bob requests to increase his position size.

  5. Since collateral A is not in the collateralLiquidationPriority list, the current loss is not deducted from Bob via deductAccountMargin, and the order to increase the position is filled successfully.

Similarly it also introduces this edge case in regards to liquidations:

  1. Bob deposits collateral A and executes a market order to buy 1 BTC. The order gets filled.

  2. The price of BTC falls dramatically, and Bob's position is liquidatable.

  3. The Admin executes a call to remove collateral A from the collateralLiquidationPriority.

  4. The liquidation keeper attempts to liquidate Bob's position.

  5. Since collateral A is not in the collateralLiquidationPriority list, the request to liquidate Bob's position executes succesfully, but the current loss is not deducted from Bob via deductAccountMargin.

Proof Of Concept

The first POC demonstrates the former edge case, while the second demonstrates the latter.

function testShouldNotAcceptOrderIfCollateralNotInPriorityList() public {
// pre conditions -
// 1. create an account and deposit margin.
// 2. create a market order and let it on pending state.
uint256 marginValueUsdc = 100_00000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
​
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID,MOCK_BTC_USD_PRICE, tradingAccountId, int128(userPositionSizeDelta));
​
// price movement - Asset price drops
updateMockPriceFeed(BTC_USD_MARKET_ID, MOCK_BTC_USD_PRICE/2);
​
uint256 receivedMarginCollateralBalanceUSDCBefore =
perpsEngine.exposed_getMarginCollateralBalance(tradingAccountId, address(usdc)).intoUint256();
​
console.log("usdc balance before price increase is: ", receivedMarginCollateralBalanceUSDCBefore);
​
// colaterral is removed from liquidationPriority before order is submitted/filled
perpsEngine.exposed_removeCollateralFromLiquidationPriority(address(usdc));
​
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID,MOCK_BTC_USD_PRICE/2, tradingAccountId, int128(userPositionSizeDelta));
​
uint256 receivedMarginCollateralBalanceUSDCAfter =
perpsEngine.exposed_getMarginCollateralBalance(tradingAccountId, address(usdc)).intoUint256();
​
console.log("usdc balance after price decrease is: ", receivedMarginCollateralBalanceUSDCAfter);
​
// @audit no reduction to collateral from previous position loss occured because collateral type not in liquidationPriority
assertEq(receivedMarginCollateralBalanceUSDCBefore, receivedMarginCollateralBalanceUSDCAfter);
​
}

Output is:

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 31.37ms (5.57ms CPU time)
function testShouldNotAcceptLiquidationIfCollateralNotInPriorityList() public {
// pre conditions -
// 1. create an account and deposit margin.
// 2. create a market order and let it on pending state.
uint256 marginValueUsdc = 100_000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
​
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID,MOCK_BTC_USD_PRICE, tradingAccountId, int128(userPositionSizeDelta));
​
// price movement - Asset price drops and user position is liquidatable
updateMockPriceFeed(BTC_USD_MARKET_ID, MOCK_BTC_USD_PRICE/20);
​
uint256 receivedMarginCollateralBalanceUSDCBefore =
perpsEngine.exposed_getMarginCollateralBalance(tradingAccountId, address(usdc)).intoUint256();
​
console.log("usdc balance before price increase is: ", receivedMarginCollateralBalanceUSDCBefore);
​
// colaterral is removed from liquidationPriority before order is submitted/filled
perpsEngine.exposed_removeCollateralFromLiquidationPriority(address(usdc));
​
_liquidateOneAccount(tradingAccountId);
​
uint256 receivedMarginCollateralBalanceUSDCAfter =
perpsEngine.exposed_getMarginCollateralBalance(tradingAccountId, address(usdc)).intoUint256();
​
console.log("usdc balance after price decrease is: ", receivedMarginCollateralBalanceUSDCAfter);
​
// @audit no reduction to collateral from liquidation occured because collateral type not in liquidationPriority
assertEq(receivedMarginCollateralBalanceUSDCBefore, receivedMarginCollateralBalanceUSDCAfter);
​
}
​
function _liquidateOneAccount(uint128 tradingAccountId) internal {
changePrank({ msgSender: liquidationKeeper });
skip(1 hours);
​
uint128[] memory liquidableAccountIds = new uint128[](1);
liquidableAccountIds[0] = tradingAccountId;
perpsEngine.liquidateAccounts(liquidableAccountIds);
}

Output is:

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 31.37ms (5.57ms CPU time)

Impact

All trades to increase a position will fail to fully deduct losses from current position if the collateral type was removed from the priority list before submitting the order. Similarly, liquidation calls will succeeed without deducting losses from user's margin.

Tools Used

Manual Review

Recommendations

Before creating/filling an order that 'increases' position size, make sure that all the user's deposited margins are included with the Liquidation priority list. A solution will also need to be introduced to ensure liquidation calls will only occur if all user deposited margins are included in liquidation priority list.

Updates

Lead Judging Commences

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

removal of collateral types from the liquidation priority list will affect affect the liquidation process of the existing positions

Support

FAQs

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

Give us feedback!