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

SEV 5: The getAccountMarginRequirementUsdAndUnrealizedPnlUsd function returns incorrect margin requirement values when a position is being changed

s3v3ru5

Severity: High

Summary

The getAccountMarginRequirementUsdAndUnrealizedPnlUsd function incorrectly uses current order's fill price as the mark price for computing margin requirements the account needs to satisfy after the trade is processed.

Vulnerability Details

The TradingAccount::getAccountMarginRequrementUsdAndUnrealizedPnlUsd function is used to compute margin requirements of an account. The function performs additional computations to calculate the margin requirements of the account after the trade was executed.

The function calculates uses incorrect markPrice for computing after trade margin requirements. The incorrect price is used to calculate the intialMargin and maintenanceMargin. As a result, the protocol might process invalid orders which do not meet the actual margin requirements and might reject valid orders.

The vulnerable uses of this function is in SettlementBranch::_fillOrder and OrderBranch::createMarketOrder. Both the _fillOrder and createMarketOrder functions try to ensure that account meets the margin requirements after the requsted order is processed.

consider following definitions:

usedInitialMargin, usedMaintenanceMargin: These are values returned by the current incorrect implementation and are used by the _fillOrder, createMarketOrder functions. These are margin requirements the protocol considers the account needs to satisfy after the trade.

correctInitialMargin, correctMaintenanceMargin: These are correct margin requirement values after the trade. If the liquidation function is called immediately after processing the trade, the liquidation function uses these values to check if the account is liquidatable.

The incorrect code is in the if (targetMarketId != 0) branch of the getAccountMarginRequrementUsdAndUnrealizedPnlUsd function. The branch calculates markPrice for filling the current order. The function uses this markPrice as the price liquidation code would use if called immediately after filling the order. These two prices will be different leading to incorrect calculation of margin requirements.

Consider skew and positionSize as the market skew and trader's position size in the target market.

markPrice for filling the order as calculated by the current implementation:

markPriceCurrentOrder = perpMarket.getMarkPrice(sizeDelta, indexPrice)

The markPrice depends on the market skew and skewScale. The markPriceCurrentOrder is the filling price for the current order.

Consider that the current order is filled and the liquidation code is executed in the next transaction. The markPrice will be different because of change in skew and the position size.

afterTradeSkew = skew + sizeDelta
afterTradePositionSize = positionSize + sizeDelta

markPriceUsedByLiquidation = perpMarket.getMarkPrice(-afterTradePositionSize, indexPrice)

The markPriceCurrentOrder, markPriceUsedByLiquidation are different because of changes in skew and the change in position size. Because the markPriceCurrentOrder is used for computing after the trade margin requirements (i.e as markPriceUsedByLiquidation) and margin requirements change depending on the mark price, the function returns incorrect margin requirement values.

Let
correctMarkPrice = markPriceUsedByLiquidation
usedMarkPrice = markPriceCurrentOrder

positionSize = position.size before the order.

Then
correctMarkPrice - usedMarkPrice = -1/2 * indexPrice * positionSize / skewScale

As a result,

  • if positionSize > 0:

    • correctMarkPrice < usedMarkPrice =>

      • correctInitialMargin < usedInitialMargin

      • correctMaintenanceMargin < usedMaintenanceMargin

    • As a result, protocol uses a larger value than the actual for the margin requirement values.

    • if correctInitialMargin < traderMarginBalance < usedInitialMargin

      • protocol rejects a size increasing trade even though user has required initialMargin.

    • if correctMaintenanceMargin < traderMarginBalance < usedMaintenanceMargin

      • protocol considers the account to be liquidatable even though its not and rejects orders which reduce the position size.

      • The trader cannot save their account from becoming liquidatable.

  • if positionSize < 0:

    • correctMarkPrice > usedMarkPrice =>

      • correctInitialMargin > usedInitialMargin

      • correctMaintenanceMargin > usedMaintenanceMargin

    • As a result, protocol uses a smaller value than the actual for the margin requirement values.

    • if correctInitialMargin > traderMarginBalance > usedInitialMargin

      • protocol accepts a size increasing trade when the trader is under initialMarginRequirement.

    • if correctMaintenanceMargin > traderMarginBalance > usedMaintenanceMargin

      • Protocol allows a liquidatable account to perform trades and close their positions.

Code Snippets:

Use of getAccountMarginRequirementUsdAndUnrealizedPnlUsd to compute after the trade margin values: SettlementBranch.sol#L410-L414

Use of markPriceCurrentOrder for calculating after the tradenotionalValue: TradingAccount.sol#L232-L240

Computation of markPrice by the liquidation code: TradingAccount.sol#L278-L285

Use of skew in computation of markPrice: PerpMarket.sol#L110-L121

POC:

The following POC can be used to verify the issue. The test case only considers one active market and the test case shows the difference in the correct and used values. The mock functions are a copy of the original implementation without any storage variables. The arthimetic operations are equivalent.

The pnl and fundingFee are not considered in the calculations as these values only influence the trader's marginBalance.

forge test --match-contract AfterTradeMarginTest -vv --via-ir

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;
​
import { UD60x18, ud60x18, convert as ud60x18Convert } from "@prb-math/UD60x18.sol";
import { SD59x18, sd59x18, ZERO as SD59x18_ZERO, unary } from "@prb-math/SD59x18.sol";
import { SafeCast } from "@openzeppelin/utils/math/SafeCast.sol";
​
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";
​
library MockConstants {
​
uint256 constant indexPrice = 68818;
uint256 constant skewScale = 1_000_000e18; // 1M
int256 constant skew = 0e18;
uint256 constant initialMarginRateX18 = 0.1e18;
uint256 constant maintenanceMarginRateX18 = 0.05e18;
​
}
​
library MockPerpMarket {
function getIndexPrice() internal pure returns (UD60x18) {
return ud60x18Convert(MockConstants.indexPrice);
}
​
function getMarkPrice(
SD59x18 skew,
SD59x18 skewDelta,
UD60x18 indexPriceX18
)
internal
pure
returns (UD60x18 markPrice)
{
// following two lines are changed to use MockConstants and arguments
SD59x18 skewScale = sd59x18(int256(MockConstants.skewScale));
​
SD59x18 priceImpactBeforeDelta = skew.div(skewScale);
SD59x18 newSkew = skew.add(skewDelta);
SD59x18 priceImpactAfterDelta = newSkew.div(skewScale);
​
SD59x18 cachedIndexPriceX18 = indexPriceX18.intoSD59x18();
​
UD60x18 priceBeforeDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactBeforeDelta)).intoUD60x18();
UD60x18 priceAfterDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactAfterDelta)).intoUD60x18();
​
markPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2));
}
}
​
library MockTradingAccount {
function getAccountMarginRequirementUsdAndUnrealizedPnlUsd(
Position.Data memory position,
uint128 targetMarketId,
SD59x18 sizeDeltaX18,
// passed as arg instead of storage
SD59x18 skew
)
internal
view
returns (
UD60x18 requiredInitialMarginUsdX18,
UD60x18 requiredMaintenanceMarginUsdX18,
UD60x18 markPrice
)
{
// if an existing position is being changed, perform some additional processing
if (targetMarketId != 0) {
// @s3v3ru5: Incorrect implementation used by protocol to calculate margin requirements after a trade
// calculate price impact of the change in position
markPrice = MockPerpMarket.getMarkPrice(skew, sizeDeltaX18, MockPerpMarket.getIndexPrice());
​
// when dealing with the market id being settled, we simulate the new position size to get the new
// margin requirements.
UD60x18 notionalValueX18 = sd59x18(position.size).add(sizeDeltaX18).abs().intoUD60x18().mul(markPrice);
​
// calculate margin requirements
(UD60x18 positionInitialMarginUsdX18, UD60x18 positionMaintenanceMarginUsdX18) = Position
.getMarginRequirement(
notionalValueX18,
ud60x18(MockConstants.initialMarginRateX18),
ud60x18(MockConstants.maintenanceMarginRateX18)
);
​
// update cumulative outputs
requiredInitialMarginUsdX18 = requiredInitialMarginUsdX18.add(positionInitialMarginUsdX18);
requiredMaintenanceMarginUsdX18 = requiredMaintenanceMarginUsdX18.add(positionMaintenanceMarginUsdX18);
} else {
// @s3v3ru5: This is the correct calculation of margin requirements after a trade
// calculate price impact as if trader were to close the entire position
markPrice = MockPerpMarket.getMarkPrice(skew, sd59x18(-position.size), MockPerpMarket.getIndexPrice());
​
// calculate notional value
UD60x18 notionalValueX18 = sd59x18(position.size).abs().intoUD60x18().mul(markPrice);
// position.getNotionalValue(markPrice);
​
// calculate margin requirements
(UD60x18 positionInitialMarginUsdX18, UD60x18 positionMaintenanceMarginUsdX18) = Position
.getMarginRequirement(
notionalValueX18,
ud60x18(MockConstants.initialMarginRateX18),
ud60x18(MockConstants.maintenanceMarginRateX18)
);
​
// update cumulative outputs
requiredInitialMarginUsdX18 = requiredInitialMarginUsdX18.add(positionInitialMarginUsdX18);
requiredMaintenanceMarginUsdX18 = requiredMaintenanceMarginUsdX18.add(positionMaintenanceMarginUsdX18);
}
}
}
​
library Position {
​
struct Data {
int256 size;
uint128 lastInteractionPrice;
int128 lastInteractionFundingFeePerUnit;
}
​
function getMarginRequirement(
UD60x18 notionalValueX18,
UD60x18 initialMarginRateX18,
UD60x18 maintenanceMarginRateX18
)
internal
pure
returns (UD60x18 initialMarginUsdX18, UD60x18 maintenanceMarginUsdX18)
{
initialMarginUsdX18 = notionalValueX18.mul(initialMarginRateX18);
maintenanceMarginUsdX18 = notionalValueX18.mul(maintenanceMarginRateX18);
}
​
}
​
​
contract AfterTradeMarginTest is Test {
function assertGetAccountMargin(int256 sizeDelta, int256 positionSize) internal {
uint128 marketId = 1;
uint128 marketIdLiqudator = 0;
SD59x18 sizeDelta = sd59x18(sizeDelta);
SD59x18 skew = sd59x18(MockConstants.skew);
​
Position.Data memory position = Position.Data({
size: positionSize,
lastInteractionPrice: 0,
lastInteractionFundingFeePerUnit: 0
});
​
(UD60x18 usedInitialMarginUsdX18, UD60x18 usedMaintenanceMarginUsdX18, UD60x18 usedMarkPrice) = MockTradingAccount
.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(
position,
marketId,
sizeDelta,
skew
);
// @s3v3ru5: Liquidation code would calculate the margin requirements as calculated below.
// These values should have been used by the SettlementBranch to check the margin balance.
position.size = sd59x18(position.size).add(sizeDelta).intoInt256();
skew = skew.add(sizeDelta);
​
(UD60x18 correctInitialMarginUsdX18, UD60x18 correctMaintenanceMarginUsdX18, UD60x18 correctMarkPrice) = MockTradingAccount
.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(
position,
marketIdLiqudator,
SD59x18_ZERO,
skew
);
​
emit log_named_decimal_uint("used mark price:", usedMarkPrice.intoUint256(), 18);
emit log_named_decimal_uint("correct mark price:", correctMarkPrice.intoUint256(), 18);
// correctMarkPrice - usedMarkPrice = -1/2 * indexPrice * positionSize / skewScale
int256 markPriceDiffEstimate = int256(MockConstants.indexPrice * 1e18) * (- positionSize) / (2 * int256(MockConstants.skewScale));
int256 actualDiff = int256(correctMarkPrice.intoUint256()) - int256(usedMarkPrice.intoUint256());
assert(actualDiff == markPriceDiffEstimate);
​
emit log_named_decimal_int("correctMarkPrice - usedMarkPrice:", actualDiff, 18);
​
int256 initialMarginDiff = int256(correctInitialMarginUsdX18.intoUint256()) - int256(usedInitialMarginUsdX18.intoUint256());
int256 maintenanceMarginDiff = int256(correctMaintenanceMarginUsdX18.intoUint256()) - int256(usedMaintenanceMarginUsdX18.intoUint256());
emit log_named_decimal_int("correctInitialMargin - usedInitialMargin:", initialMarginDiff, 18);
emit log_named_decimal_int("correctMaintenanceMargin - usedMaintenanceMargin:", maintenanceMarginDiff, 18);
​
// correctMarkPrice - usedMarkPrice = -1/2 * indexPrice * positionSize / skewScale
if (positionSize > 0) {
// - correctMarkPrice < usedMarkPrice =>
// - correctInitialMargin < usedInitialMargin
// - correctMaintenanceMargin < usedMaintenanceMargin
// As a result,
// protocol uses a larger value than the actual for the margin requirement values.
// - if user's margin balance
// if correctInitialMargin < marginBalance < usedInitialMargin
// - protocol rejects a size increasing trade even though user has required initialMargin.
// if correctMaintenanceMargin < marginBalance < usedMaintenanceMargin
// - protocol considers the account to be liquidatable even though its not and rejects
// - orders which reduce the position size.
// - The trader could have saved their account from becoming liquidatable.
assert(correctMarkPrice.lt(usedMarkPrice));
assert(correctInitialMarginUsdX18.lt(usedInitialMarginUsdX18));
assert(correctMaintenanceMarginUsdX18.lt(usedMaintenanceMarginUsdX18));
} else {
// - correctMarkPrice > usedMarkPrice =>
// - correctInitialMargin > usedInitialMargin
// - correctMaintenanceMargin > usedMaintenanceMargin
// As a result,
// protocol uses a smaller value than the actual for the margin requirement values.
// if correctInitialMargin > traderMarginBalance > usedInitialMargin
// - protocol accepts a size increasing trade when the trader is under initialMarginRequirement.
// if correctMaintenanceMargin > traderMarginBalance > usedMaintenanceMargin
// - Protocol allows a liquidatable account to perform trades and close their positions.
assert(correctMarkPrice.gt(usedMarkPrice));
assert(correctInitialMarginUsdX18.gt(usedInitialMarginUsdX18));
assert(correctMaintenanceMarginUsdX18.gt(usedMaintenanceMarginUsdX18));
}
}
​
function test_incorrectMarginCalculation() external {
int256 positionSizeNeg = -150e18;
int256 sizeDeltaNeg = -50e18;
int256 positionSizePos = 100e18;
int256 sizeDeltaPos = 100e18;
​
emit log("Test case 1 Position Size Negative: correct > used");
emit log_named_decimal_int("positionSize:", positionSizeNeg, 18);
emit log_named_decimal_int("sizeDelta:", sizeDeltaNeg, 18);
assertGetAccountMargin(sizeDeltaNeg, positionSizeNeg);
​
emit log("Test case 2 Position Size Positive: correct < used");
emit log_named_decimal_int("positionSize:", positionSizePos, 18);
emit log_named_decimal_int("sizeDelta:", sizeDeltaPos, 18);
assertGetAccountMargin(sizeDeltaPos, positionSizePos);
}
}
> forge test --match-contract AfterTradeMarginTest -vv --via-ir
​
Ran 1 test for test/unit/AfterTradeMargin.t.sol:AfterTradeMarginTest
[PASS] test_incorrectMarginCalculation() (gas: 41250)
Logs:
Test case 1 Position Size Negative: correct > used
positionSize:: -150.000000000000000000
sizeDelta:: -50.000000000000000000
used mark price:: 68816.279550000000000000
correct mark price:: 68821.440900000000000000
correctMarkPrice - usedMarkPrice:: 5.161350000000000000
correctInitialMargin - usedInitialMargin:: 103.227000000000000000
correctMaintenanceMargin - usedMaintenanceMargin:: 51.613500000000000000
Test case 2 Position Size Positive: correct < used
positionSize:: 100.000000000000000000
sizeDelta:: 100.000000000000000000
used mark price:: 68821.440900000000000000
correct mark price:: 68818.000000000000000000
correctMarkPrice - usedMarkPrice:: -3.440900000000000000
correctInitialMargin - usedInitialMargin:: -68.818000000000000000
correctMaintenanceMargin - usedMaintenanceMargin:: -34.409000000000000000

Impact

  • if positionSize > 0:

    • if correctInitialMargin < traderMarginBalance < usedInitialMargin

      • protocol rejects a size increasing trade even though user has required initialMargin.

    • if correctMaintenanceMargin < traderMarginBalance < usedMaintenanceMargin

      • protocol considers the account to be liquidatable even though its not and rejects orders which reduce the position size.

      • The trader cannot save their account from becoming liquidatable.

  • if positionSize < 0:

    • if correctInitialMargin > traderMarginBalance > usedInitialMargin

      • protocol accepts a size increasing trade when the trader is under initialMarginRequirement.

    • if correctMaintenanceMargin > traderMarginBalance > usedMaintenanceMargin

      • Protocol allows a liquidatable account to perform trades and close their positions.

Tools Used

Manual Review

Recommendations

Add a function to PerpMarket library which takes skew as an argument and computes the markPrice. In the getAccountMarginRequirementUsdAndUnrealizedPnlUsd function use the added function with after the trade skew and correct position size to compute the markPriceAfterTheTrade. Use this markPrice to compute the notional value and margin requirements.

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.