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

`getAccountMarginRequirementUsdAndUnrealizedPnlUsd()` might return an incorrect unrealized pnl when `targetMarketId > 0`.

kupiasec

Summary

getAccountMarginRequirementUsdAndUnrealizedPnlUsd() might return an incorrect unrealized pnl when targetMarketId > 0 and there is an existing position on the target market.

Vulnerability Details

While filling a market order, SettlementBranch::_fillOrder validates a margin requirement.

File: SettlementBranch.sol
408: // calculate required initial & maintenance margin for this trade
409: // and account's unrealized PNL
410: (
411: UD60x18 requiredInitialMarginUsdX18,
412: UD60x18 requiredMaintenanceMarginUsdX18,
413: SD59x18 accountTotalUnrealizedPnlUsdX18
414: ) = tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(marketId, sizeDeltaX18);
415:

It gets accountTotalUnrealizedPnlUsdX18 using the getAccountMarginRequirementUsdAndUnrealizedPnlUsd() function and this pnl shows a total pnl of active positions.

In getAccountMarginRequirementUsdAndUnrealizedPnlUsd(), it calculates markPrice differently for targetMarketId.

File: TradingAccount.sol
224: if (targetMarketId != 0) {
225: // load the market's perp information
226: PerpMarket.Data storage perpMarket = PerpMarket.load(targetMarketId);
227:
228: // load the account's position in that market
229: Position.Data storage position = Position.load(self.id, targetMarketId);
230:
231: // calculate price impact of the change in position
232: UD60x18 markPrice = perpMarket.getMarkPrice(sizeDeltaX18, perpMarket.getIndexPrice()); //@audit incorrect price
233:
234: // get funding fee per unit
235: SD59x18 fundingFeePerUnit =
236: perpMarket.getNextFundingFeePerUnit(perpMarket.getCurrentFundingRate(), markPrice);
237:
238: // when dealing with the market id being settled, we simulate the new position size to get the new
239: // margin requirements.
240: UD60x18 notionalValueX18 = sd59x18(position.size).add(sizeDeltaX18).abs().intoUD60x18().mul(markPrice);
241:
242: // calculate margin requirements
243: (UD60x18 positionInitialMarginUsdX18, UD60x18 positionMaintenanceMarginUsdX18) = Position
244: .getMarginRequirement(
245: notionalValueX18,
246: ud60x18(perpMarket.configuration.initialMarginRateX18),
247: ud60x18(perpMarket.configuration.maintenanceMarginRateX18)
248: );
249:
250: // get unrealized pnl + accrued funding fees
251: SD59x18 positionUnrealizedPnl =
252: position.getUnrealizedPnl(markPrice).add(position.getAccruedFunding(fundingFeePerUnit));

For normal markets, it gets a markPrice using perpMarket.getMarkPrice(-position.size, indexPrice) at L278 because this price would be a fill price when we close the position.

But for targetMarketId, it uses markPrice = perpMarket.getMarkPrice(sizeDeltaX18, perpMarket.getIndexPrice()) to get the old position's pnl. This markPrice is a fill price of a new position but shouldn't be used for the old position because this price is not a closing price of the old position.

Here is an example.

  • Alice has a position of PosSize on a market and the unrealized pnl of the position will be calculated using markPrice = getMarkPrice(-PosSize, indexPrice).

  • Alice opens an additional position of 1 wei(we ignore minTradeSize for simplicity) on the same market.

  • Logically, her unrealized pnl won't be changed a lot because she has opened a very small position.

  • But during the settlement, getAccountMarginRequirementUsdAndUnrealizedPnlUsd() will be called and it will use markPrice = getMarkPrice(sizeDelta = 1, indexPrice) for her entire position(PosSize + 1).

  • So in getAccountMarginRequirementUsdAndUnrealizedPnlUsd(), her unrealized pnl might be calculated wrongly and it will affect the margin requirement validation.

Here is a detailed POC.

  • To execute the POC, we should set minTradeSizeX18 = 1 here.

  • The test function should be copied to test\integration\perpetuals\trading-account-branch\getAccountEquityUsd\getAccountEquityUsd.t.sol.

// Import on the top
import {console} from "forge-std/console.sol";
​
function test_WrongMarkPriceForTragetIdIfTheSameMarketIdPositionExistsAlready(
uint256 initialMarginRate,
uint256 wstEthmarginValueUsd,
uint256 weEthmarginValueUsd,
bool isLong,
uint256 marketId
)
external
givenTheTradingAccountExists
{
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
​
initialMarginRate = 1e17;
​
wstEthmarginValueUsd = 1e23;
deal({ token: address(wstEth), to: users.naruto.account, give: wstEthmarginValueUsd * 2 });
​
weEthmarginValueUsd = 1e23;
deal({ token: address(weEth), to: users.naruto.account, give: weEthmarginValueUsd * 2});
​
uint128 tradingAccountId = createAccountAndDeposit(wstEthmarginValueUsd * 2, address(wstEth));
perpsEngine.depositMargin(tradingAccountId, address(weEth), weEthmarginValueUsd * 2);
​
// get marginCollateralValue
UD60x18 marginCollateralValue = getPrice(
MockPriceFeed(marginCollaterals[WSTETH_MARGIN_COLLATERAL_ID].priceFeed)
).mul(ud60x18(wstEthmarginValueUsd * 2)).add(
getPrice(MockPriceFeed(marginCollaterals[WEETH_MARGIN_COLLATERAL_ID].priceFeed)).mul(
ud60x18(weEthmarginValueUsd * 2)
)
);
// apply loanToValue(70%)
marginCollateralValue = marginCollateralValue.mul(ud60x18(7e17));
​
int128 sizeDelta = 5e22; // First order size
​
// create the first market order
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: fuzzMarketConfig.marketId,
sizeDelta: sizeDelta
})
);
​
// fill the first market order
bytes memory mockSignedReport =
getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
address marketOrderKeeper = marketOrderKeepers[fuzzMarketConfig.marketId];
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
​
SD59x18 accountTotalUnrealizedPnlFirst = perpsEngine.getAccountTotalUnrealizedPnl(tradingAccountId);
​
// simulate the second market order
sizeDelta = 1; // 1 wei
​
(SD59x18 marginBalanceUsdX18,,,,,) = perpsEngine.simulateTrade(tradingAccountId,
fuzzMarketConfig.marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta);
​
// create the second market order
changePrank({ msgSender: users.naruto.account });
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: fuzzMarketConfig.marketId,
sizeDelta: sizeDelta
})
);
​
// fill the second market order
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
​
SD59x18 accountTotalUnrealizedPnlSecond = perpsEngine.getAccountTotalUnrealizedPnl(tradingAccountId);
​
console.log("UnrealizedPnlAfterFirstOrder");
console.logInt(accountTotalUnrealizedPnlFirst.intoInt256());
​
console.log("UnrealizedPnlAfterSecondOrder");
console.logInt(accountTotalUnrealizedPnlSecond.intoInt256());
​
console.log("SimulatedUnrealizedPnlBeforeSecondOrder");
console.logInt((marginBalanceUsdX18.sub(marginCollateralValue.intoSD59x18())).intoInt256());
}

Here is a test result.

├─ [0] console::log("UnrealizedPnlAfterFirstOrder") [staticcall]
│ └─ ← ()
├─ [0] console::logInt(0) [staticcall]
│ └─ ← ()
├─ [0] console::log("UnrealizedPnlAfterSecondOrder") [staticcall]
│ └─ ← ()
├─ [0] console::logInt(-12469659279800000 [-1.246e16]) [staticcall]
│ └─ ← ()
├─ [0] console::log("SimulatedUnrealizedPnlBeforeSecondOrder") [staticcall]
│ └─ ← ()
├─ [0] console::logInt(-561387670000904133200 [-5.613e20]) [staticcall]

As we can observe, UnrealizedPnlAfterFirstOrder and UnrealizedPnlAfterSecondOrder are very similar but SimulatedUnrealizedPnlBeforeSecondOrder shows a huge difference. (The output might be different due to the fuzz testing).

Impact

During an order settlement, it might validate a margin requirement wrongly as getAccountMarginRequirementUsdAndUnrealizedPnlUsd() might return an incorrect unrealized pnl.

Tools Used

Manual Review

Recommendations

While checking Synthetix as a reference, it uses mark price of original positions.

Synthetix calculates like this.

  • It gets currentAvailableMargin from isEligibleForLiquidation first. It means the pnl of an old position without considering the new position.

  • After that, it gets a fill price for the new position here. So this price is exactly same as the current markPrice for targetMarketId.

  • And then, it calculates order/settlement fees using this fill price.(Zaros handles inside _fillOrder() correctly)

  • After that, it gets a pnl of the final position(old size + new size) in calculateStartingPnl. And adds to currentAvailableMargin only if it's negative. It just uses (marketPrice - fillPrice) * totalPositionSize.

So with my example,

  • It should calculate the pnl of PosSize position like normal markets. So use getMarkPrice(-PosSize, indexPrice).

  • After that, it gets a mark price for the new position. getMarkPrice(1, offchainPrice).

  • And the starting pnl will be (indexPrice - fillPrice) * (PosSize + 1) and it should be added to the pnl if negative.

Updates

Lead Judging Commences

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

getAccountMarginRequirementUsdAndUnrealizedPnlUsd function returns incorrect margin requirement values when a position is being changed

Support

FAQs

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