Submission Details
Severity:
Incorrect unrealizedPnlUsdX18 Calculation on Trade Size Reduction
Summary
When a trader is already making some loss, trade size reduction causes zaros to erroneously remove the total loss instead of the proportionate loss based on the reduced percentage of the trade. This problem can lead to financial discrepancies and a loss of trust in the protocol's fairness.
Vulnerability Details
When a trader reduces the size of their open position which is currently in loss, the protocol incorrectly removes the total loss associated with the entire position instead of adjusting the loss proportionately to the reduced trade size. The fillMarketOrder function calls the _fillOrderwhere the main login is executed. There is no check in the _fillOrderto execute logic specific to when a trader is trying to reduce the trading size. A new position is always created even if the trader is trying to reduce their position. The error is obvious on the zaros app where the app tells you the expected loss when you try to reduce a trade but the contract clears all the current unrealizedPnlUsdX18 and creates a new postion with a reduced trade size.
The image above shows a trader trying to reduce 10% of a trade and the expected loss that should be removed from the trade, but the smart contract does otherwise.
function _fillOrder(
uint128 tradingAccountId,
uint128 marketId,
uint128 settlementConfigurationId,
SD59x18 sizeDeltaX18,
UD60x18 fillPriceX18
)
internal
virtual
{
// working data
FillOrderContext memory ctx;
// fetch storage slot for global config
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
// fetch storage slot for perp market's settlement config
SettlementConfiguration.Data storage settlementConfiguration =
SettlementConfiguration.load(marketId, settlementConfigurationId);
// cache settlement token
ctx.usdToken = globalConfiguration.usdToken;
// determine whether position is being increased or not
ctx.isIncreasing = Position.isIncreasing(tradingAccountId, marketId, sizeDeltaX18.intoInt256().toInt128());
// both markets and settlement can be disabled, however when this happens we want to:
// 1) allow open positions not subject to liquidation to decrease their size or close
// 2) prevent new positions from being opened & existing positions being increased
//
// the idea is to prevent a state where traders have open positions but are unable
// to reduce size or close even though they can still be liquidated; such a state
// would severly disadvantage traders
if (ctx.isIncreasing) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
// load existing trading account; reverts for non-existent account
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
// fetch storage slot for perp market
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
// enforce minimum trade size
perpMarket.checkTradeSize(sizeDeltaX18);
// get funding rates for this perp market
ctx.fundingRateX18 = perpMarket.getCurrentFundingRate();
ctx.fundingFeePerUnitX18 = perpMarket.getNextFundingFeePerUnit(ctx.fundingRateX18, fillPriceX18);
// update funding rates
perpMarket.updateFunding(ctx.fundingRateX18, ctx.fundingFeePerUnitX18);
// calculate order & settlement fees
ctx.orderFeeUsdX18 = perpMarket.getOrderFeeUsd(sizeDeltaX18, fillPriceX18);
ctx.settlementFeeUsdX18 = ud60x18(uint256(settlementConfiguration.fee));
// fetch storage slot for account's potential existing position in this market
Position.Data storage oldPosition = Position.load(tradingAccountId, marketId);
// int256 -> SD59x18
ctx.oldPositionSizeX18 = sd59x18(oldPosition.size);
{
// calculate required initial & maintenance margin for this trade
// and account's unrealized PNL
(
UD60x18 requiredInitialMarginUsdX18,
UD60x18 requiredMaintenanceMarginUsdX18,
SD59x18 accountTotalUnrealizedPnlUsdX18
) = tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(marketId, sizeDeltaX18);
// check maintenance margin if:
// 1) position is not increasing AND
// 2) existing position is being decreased in size
//
// when a position is under the higher initial margin requirement but over the
// lower maintenance margin requirement, we want to allow the trader to decrease
// their losing position size before they become subject to liquidation
//
// but if the trader is opening a new position or increasing the size
// of their existing position we want to ensure they satisfy the higher
// initial margin requirement
ctx.shouldUseMaintenanceMargin = !ctx.isIncreasing && !ctx.oldPositionSizeX18.isZero();
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? requiredMaintenanceMarginUsdX18 : requiredInitialMarginUsdX18;
// reverts if the trader can't satisfy the appropriate margin requirement
tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18,
tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18),
ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);
}
// cache unrealized PNL from potential existing position in this market
// including accrued funding
ctx.pnlUsdX18 =
oldPosition.getUnrealizedPnl(fillPriceX18).add(oldPosition.getAccruedFunding(ctx.fundingFeePerUnitX18));
// create new position in working area
ctx.newPosition = Position.Data({
size: ctx.oldPositionSizeX18.add(sizeDeltaX18).intoInt256(),
lastInteractionPrice: fillPriceX18.intoUint128(),
lastInteractionFundingFeePerUnit: ctx.fundingFeePerUnitX18.intoInt256().toInt128()
});
// int256 -> SD59x18
ctx.newPositionSizeX18 = sd59x18(ctx.newPosition.size);
// enforce open interest and skew limits for target market and calculate
// new open interest and new skew
(ctx.newOpenInterestX18, ctx.newSkewX18) =
perpMarket.checkOpenInterestLimits(sizeDeltaX18, ctx.oldPositionSizeX18, ctx.newPositionSizeX18);
// update open interest and skew for this perp market
perpMarket.updateOpenInterest(ctx.newOpenInterestX18, ctx.newSkewX18);
// update active markets for this account; may also trigger update
// to global config set of active accounts
tradingAccount.updateActiveMarkets(marketId, ctx.oldPositionSizeX18, ctx.newPositionSizeX18);
// if the position is being closed, clear old position data
if (ctx.newPositionSizeX18.isZero()) {
oldPosition.clear();
}
// otherwise we are opening a new position or modifying an existing position
else {
// revert if new position size is under the minimum for this market
if (ctx.newPositionSizeX18.abs().lt(sd59x18(int256(uint256(perpMarket.configuration.minTradeSizeX18))))) {
revert Errors.NewPositionSizeTooSmall();
}
// update trader's existing position in this market with new position data
oldPosition.update(ctx.newPosition);
}
// if trader's old position had positive pnl then credit that to the trader
if (ctx.pnlUsdX18.gt(SD59x18_ZERO)) {
ctx.marginToAddX18 = ctx.pnlUsdX18.intoUD60x18();
tradingAccount.deposit(ctx.usdToken, ctx.marginToAddX18);
// mint settlement tokens credited to trader; tokens are minted to
// address(this) since they have been credited to trader's deposited collateral
//
// NOTE: testnet only - this call will be updated once the Market Making Engine is finalized
LimitedMintingERC20(ctx.usdToken).mint(address(this), ctx.marginToAddX18.intoUint256());
}
// pay order/settlement fees and deduct collateral
// if trader's old position had negative pnl
@> tradingAccount.deductAccountMargin({
@> feeRecipients: FeeRecipients.Data({
@> marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
@> orderFeeRecipient: globalConfiguration.orderFeeRecipient,
@> settlementFeeRecipient: globalConfiguration.settlementFeeRecipient
@> }),
@> pnlUsdX18: ctx.pnlUsdX18.lt(SD59x18_ZERO) ? ctx.pnlUsdX18.abs().intoUD60x18() : UD60x18_ZERO,
@> orderFeeUsdX18: ctx.orderFeeUsdX18,
@> settlementFeeUsdX18: ctx.settlementFeeUsdX18
@> });
emit LogFillOrder(
msg.sender,
tradingAccountId,
marketId,
sizeDeltaX18.intoInt256(),
fillPriceX18.intoUint256(),
ctx.orderFeeUsdX18.intoUint256(),
ctx.settlementFeeUsdX18.intoUint256(),
ctx.pnlUsdX18.intoInt256(),
ctx.fundingFeePerUnitX18.intoInt256()
);
}
Proof of Concept
Trader makes a LONG trade with Size 1 on BTC-USD Market
The price of BTC-USD went down which puts the trader in a loss
Trader tries to reduce trade size to reduce risk
A new position was created for the trader and all the initial expected loss was removed
Proof of Code
To run the test paste the test into test/integration/perpetuals/order-branch/createMarketOrder
Add this to the test file - import { Position } from "@zaros/perpetuals/leaves/Position.sol";
function testReducePositionSizeLeadToLoss() public {
changePrank({ msgSender: users.sasuke.account });
// $10,000
uint256 amount = 10000 * 10 ** 6;
deal({ token: address(usdc), to: users.sasuke.account, give: amount });
uint128 tradingAccountId = createAccountAndDeposit(amount, address(usdc));
// With this size, the trade is on 10x leverage
int128 size = 1 * 10 ** 18;
// Create a LONG market order on BTC-USD with size 1
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId : BTC_USD_MARKET_ID,
sizeDelta: size
})
);
// Pick up market order by marketorderkeepers
changePrank({ msgSender: marketOrderKeepers[1] });
bytes memory firstMockSignedReport =
getMockedSignedReport(0x00037da06d56d083fe599397a4769a042d63aa73dc4ef57709d31e9971a5b439, MOCK_BTC_USD_PRICE);
perpsEngine.fillMarketOrder(tradingAccountId, BTC_USD_MARKET_ID, firstMockSignedReport);
changePrank({ msgSender: users.sasuke.account });
// Get current trading account position
Position.State memory position = perpsEngine.getPositionState(tradingAccountId,
BTC_USD_MARKET_ID,
MOCK_BTC_USD_PRICE
);
//Entry Price
UD60x18 initialEntryPrice = position.entryPriceX18;
//Entry Position Size
SD59x18 positionSize = position.sizeX18;
// Simulate Change in Price
// Price moved from 100,000 to 95,000
uint256 updatedPrice = MOCK_BTC_USD_PRICE - MOCK_BTC_USD_PRICE / 20;
updateMockPriceFeed(BTC_USD_MARKET_ID, updatedPrice);
// Get updated trading account position
Position.State memory position2 = perpsEngine.getPositionState(tradingAccountId,
BTC_USD_MARKET_ID,
updatedPrice
);
//Updated Position Size
SD59x18 updatedPositionSize = position2.sizeX18;
// unrealizedPnlUsdX18
SD59x18 unrealizedPnlUsdX18 = position2.unrealizedPnlUsdX18;
// Reduce Trade Size
size = 0.5 * 10 ** 18;
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: BTC_USD_MARKET_ID,
sizeDelta: -size
})
);
changePrank({ msgSender: marketOrderKeepers[1] });
bytes memory firstMockSignedReport2 =
getMockedSignedReport(0x00037da06d56d083fe599397a4769a042d63aa73dc4ef57709d31e9971a5b439, updatedPrice);
perpsEngine.fillMarketOrder(tradingAccountId, BTC_USD_MARKET_ID, firstMockSignedReport2);
changePrank({ msgSender: users.sasuke.account });
Position.State memory position3 = perpsEngine.getPositionState(
tradingAccountId,
BTC_USD_MARKET_ID,
updatedPrice
);
//Updated Position Size
SD59x18 updatedPositionSize1 = position3.sizeX18;
// unrealizedPnlUsdX18
SD59x18 unrealizedPnlUsdX181 = position3.unrealizedPnlUsdX18;
// assert that the updatedPositionSize1 is half of the initial updatedPositionSize before size reduction
// the size reduction should remove half of the intial size
assert(updatedPositionSize1.intoUint256() == updatedPositionSize.intoUint256() / 2);
// assert that the unrealizedPnlUsdX181 is half of the initial unrealizedPnlUsdX18 before size reduction
// the size reduction should remove half of the loss
// the test will fail because the whole loss was removed after size reduction
assert(unrealizedPnlUsdX181.intoInt256() == unrealizedPnlUsdX18.intoInt256() / 2);
}
Impact
Trader makes an unexpected loss - The trader is trying to reduce the risk by reducing the trade size but because all the unrealizedPnlUsdX18 (loss) is removed, the trader has lost all the money that could have reversed if the market changed direction.
Tools Used
Manual Review
Recommendations
Update the loss calculation logic to ensure that only the proportionate loss corresponding to the reduced trade size is removed.
Updates
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821
USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.