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

Funding is incorrectly updated while executing `LiquidationBranch.liquidateAccounts()` with multiple trading accounts and/or positions.

baz1ka

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L188

Summary

LiquidationBranch.liquidateAccounts() can liquidate multiple accounts in one call. If account has more than 1 position, all these positions would be liquidated too. When liquidating an account asset price changes because it closes position. In that case when liquidating multiple trading accounts and/or positions mark price should be changed multiple times but it doesn't. It changes only for current position in loop, but doesn't count all other liquidated position sizes.

Impact

Incorrect value of markPrice affects on calculation of funding rate. The more accounts are liquidated, the bigger difference between correct and actual markPrice and consequently funding rate would be. This would be also a problem when the first closed position has a large size and the last has a small size, in that case, funding rate would be calculated for the last position with a small size.

Proof of Concept

If we take a look at LiquidationBranch.liquidateAccounts() function we will see that it iterates through all liquidatable account ids and if an account is liquidatable it start closing all account positions.

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);
}

As we see from in this line calculation of price impact is getting from closing current position (ctx.liquidationSizeX18 which is -ctx.oldPositionSizeX18). But this does not include price impact of previous positions as index price is the same for all closed positions.

Let's take a simple example. Liquidatable accounts have 2 buy positions, first with size 20e18, other with 10e18. indexPrice is MOCK_BTC_USD_PRICE = 100_000e18. When liquidating this account position position with size 20e18 is liquidated first. After closing first position price would be 999_999e17, but after closing second position price would be bigger - 999_9995e16 (price after closing second position is bigger than after closing first).

Here is a simple test example with provided data that "simulates" mark price change while liquidating accounts:

function test_GetMarkPriceOfTwoPositions() external {
uint256 marketId = BTC_USD_MARKET_ID;
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
// position delta = -position size
int256 firstPositionDelta = -20e18;
int256 secondPositionDelta = -10e18;
​
UD60x18 markPriceAfterClosingFirstPosition = perpsEngine.getMarkPrice(fuzzMarketConfig.marketId, fuzzMarketConfig.mockUsdPrice, firstPositionDelta);
​
console.log(markPriceAfterClosingFirstPosition.intoUint256()); // 99999900000000000000000
​
UD60x18 markPriceAfterClosingSecondPosition = perpsEngine.getMarkPrice(fuzzMarketConfig.marketId, fuzzMarketConfig.mockUsdPrice, secondPositionDelta);
​
console.log(markPriceAfterClosingSecondPosition.intoUint256()); // 99999950000000000000000
}

Recommended Mitigation Steps

It is necessary to take into account the sizeDelta of closed positions:

diff --git a/src/perpetuals/branches/LiquidationBranch.sol b/src/perpetuals/branches/LiquidationBranch.sol
index e2a4969..a3b3152 100644
--- a/src/perpetuals/branches/LiquidationBranch.sol
+++ b/src/perpetuals/branches/LiquidationBranch.sol
@@ -182,7 +182,7 @@ contract LiquidationBranch {
ctx.oldPositionSizeX18 = sd59x18(position.size);
// save inverted sign of open position size to prepare for closing the position
- ctx.liquidationSizeX18 = -ctx.oldPositionSizeX18;
+ ctx.liquidationSizeX18 = ctx.liquidationSizeX18.sub(ctx.oldPositionSizeX18);
// calculate price impact of open position being closed
ctx.markPriceX18 = perpMarket.getMarkPrice(ctx.liquidationSizeX18, perpMarket.getIndexPrice());
Updates

Lead Judging Commences

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

When calculating funding fees the indexPrice should be used

Appeal created

h2134 Auditor
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

When calculating funding fees the indexPrice should be used

Support

FAQs

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