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

Incorrect UnrealizedPnL Deduction on Position Size Reduction

pelz

Summary

The createMarketOrder function in the OrderBranch contract incorrectly handles the reduction of position sizes. This flaw forces traders to incur 100% of their losses when they attempt to reduce their position size, instead of the expected proportionate loss.

Vulnerability Details

The createMarketOrder function allows users to create and update orders. When a trader running at a loss decides to reduce their position size by a certain percentage (e.g., 50%), the function should close only the corresponding percentage of the position and its losses. However, the function currently closes the entire position, forcing the trader to realize 100% of the losses at that time.

Proof of Concept

function testMarketPositionSizes() public {
​
changePrank({ msgSender: users.sasuke.account });
​
uint256 amount = 100000 * 10 ** 6;
​
deal({ token: address(usdc), to: users.sasuke.account, give: amount });
​
uint128 tradingAccountId = createAccountAndDeposit(amount, address(usdc));
int128 size = 1 * 10 ** 18;
​
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: 1,
sizeDelta: size
})
);
perpsEngine.getActiveMarketOrder({ tradingAccountId: tradingAccountId });
​
bytes memory firstMockSignedReport =
getMockedSignedReport(0x00037da06d56d083fe599397a4769a042d63aa73dc4ef57709d31e9971a5b439, 100_000e18);
​
changePrank({ msgSender: marketOrderKeepers[1] });
​
perpsEngine.fillMarketOrder(tradingAccountId, 1, firstMockSignedReport);
​
​
Position.State memory position = perpsEngine.getPositionState(
tradingAccountId,
1,
100_000e18
);
//get entry price first time
UD60x18 initialEntryPrice = position.entryPriceX18;
​
// get position size
SD59x18 positionSize1 = position.sizeX18;
​
// reduce price to simulate losses..
uint256 updatedPrice = MOCK_BTC_USD_PRICE - 7000e18;
​
//uint256 updatedPrice = MOCK_BTC_USD_PRICE - 70000;
updateMockPriceFeed(BTC_USD_MARKET_ID, updatedPrice);
​
changePrank({ msgSender: users.sasuke.account });
​
// reduce trade size by 50% of trade
size = 0.5 * 10 ** 18;
​
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: 1,
sizeDelta: -size
})
);
​
bytes memory firstMockSignedReport2 =
getMockedSignedReport(0x00037da06d56d083fe599397a4769a042d63aa73dc4ef57709d31e9971a5b439, updatedPrice);
​
// get pnl which should be 50% of trade to be closed
Position.State memory position2 = perpsEngine.getPositionState(
tradingAccountId,
1,
updatedPrice
);
// this pnl is 100% of unrealizeed pnl
SD59x18 PNL = position2.unrealizedPnlUsdX18;
// fill order
changePrank({ msgSender: marketOrderKeepers[1] });
​
perpsEngine.fillMarketOrder(tradingAccountId, 1, firstMockSignedReport2);
​
​
changePrank({ msgSender: users.sasuke.account });
​
// get position state
Position.State memory position3 = perpsEngine.getPositionState(
tradingAccountId,
1,
updatedPrice
);
​
// get entry price second time
UD60x18 newEntryPrice = position3.entryPriceX18;
​
// get position size should reduce
SD59x18 positionSize2 = position3.sizeX18;
​
// get pnl which should be the remaining 50% of initialsize left and 100% of new size
SD59x18 PNLAfterReduction = position3.unrealizedPnlUsdX18;
​
// position size should decrease
assertLt( positionSize2.intoUint256(), positionSize1.intoUint256(), "new position size should be less than initial position size");
​
// pnl should be equal to 50% of initial pnl
// assertion fails because when trade got reduced it closed the trade with the Full losses incurred to the user in this scenario
assertEq( PNLAfterReduction.intoInt256(), PNL.intoInt256()/2, "new pnl should be equal to 50% of initial pnl");
}

Root Cause:

The function deductAccountMargin incorrectly deducts the full PnL for the entire position rather than proportionately based on the size reduction.

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

Impact

This vulnerability leads to incorrect financial outcomes for traders. When a trader attempts to reduce their position size, they are forced to incur the entire loss/profits of the position rather than just a proportionate amount. This can result in significant unexpected losses and incorrect trading behavior, impacting the trader's financial strategy.

Tools Used

  • Foundry

  • Zaros Website

Recommendations

  1. Fix the Position Reduction Logic: Modify the createMarketOrder & fillMarketOrder function to ensure that it correctly handles partial position reductions. The function should only close the proportionate part of the position and realize corresponding losses.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

In case of a position reduction + negative PNL, the whole negative PNL is deducted

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

In case of a position reduction + negative PNL, the whole negative PNL is deducted

Support

FAQs

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

Give us feedback!