Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Incorrect use of PnL in `deductAccountMargin()` in `TradingAccount.sol`

viquetoh

Summary

A vulnerability sterms in deductAccountMargin() where the function incorrectly handles proffit and loss by treating them as unsigned. This leads to deductions of profit from user's margin instead of being added, directly contradicting the behaviour of reward gains. Also, to really understand how PnL is being used, looking at the struct DeductAccountMarginParams pnlUsdX18 is of type UD60X18 which happens to be unsigned. Meaning that even when a PnL is positive, the code deducts it from the user's margin instead of increasing the user's margin.

Vulnerability Details

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/perpetuals/leaves/TradingAccount.sol#L506-L642
​
function deductAccountMargin(
Data storage self,
DeductAccountMarginParams memory params
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
// working data
DeductAccountMarginContext memory ctx;
​
// fetch storage slot for perps engine configuration
PerpsEngineConfiguration.Data storage perpsEngineConfiguration = PerpsEngineConfiguration.load();
​
// fetch storage slot for market making engine
IMarketMakingEngine marketMakingEngine = IMarketMakingEngine(perpsEngineConfiguration.marketMakingEngine);
​
// cache collateral liquidation priority length
uint256 cachedCollateralLiquidationPriorityLength =
perpsEngineConfiguration.collateralLiquidationPriority.length();
​
// loop through configured collateral types
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
// get ith collateral type
address collateralType = perpsEngineConfiguration.collateralLiquidationPriority.at(i);
​
// fetch storage slot for this collateral's config config
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// get trader's collateral balance for this collateral, scaled to 18 decimals
ctx.marginCollateralBalanceX18 = getMarginCollateralBalance(self, collateralType);
​
// skip to next loop iteration if trader hasn't deposited any of this collateral
if (ctx.marginCollateralBalanceX18.isZero()) continue;
​
// get this collateral's USD price
ctx.marginCollateralPriceUsdX18 = marginCollateralConfiguration.getPrice();
​
// if:
// settlement fee > 0 AND
// amount of settlement fee paid so far < settlement fee
if (
params.settlementFeeUsdX18.gt(UD60x18_ZERO)
&& ctx.settlementFeeDeductedUsdX18.lt(params.settlementFeeUsdX18)
) {
// attempt to deduct from this collateral difference between settlement fee
// and amount of settlement fee paid so far
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin,) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
params.settlementFeeUsdX18.sub(ctx.settlementFeeDeductedUsdX18),
params.feeRecipients.settlementFeeRecipient
);
​
// update amount of settlement fee paid so far by the amount
// that was actually withdraw from this collateral
ctx.settlementFeeDeductedUsdX18 = ctx.settlementFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// order fee logic same as settlement fee above
if (params.orderFeeUsdX18.gt(UD60x18_ZERO) && ctx.orderFeeDeductedUsdX18.lt(params.orderFeeUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin,) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
params.orderFeeUsdX18.sub(ctx.orderFeeDeductedUsdX18),
params.feeRecipients.orderFeeRecipient
);
ctx.orderFeeDeductedUsdX18 = ctx.orderFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// pnl logic same as settlement & order fee above
if (params.pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(params.pnlUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin, ctx.assetAmount) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
params.pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
address(this)
);
​
// verify if we have the asset to send to the market making engine
if (ctx.assetAmount > 0) {
// create variable to cache the sum of all position usd
UD60x18 sumOfAllPositionsUsdX18;
​
// cache market ids length
uint256 cachedMarketIdsLength = params.marketIds.length;
​
// we need to sum the notional value of all active positions only if we're handling more than one
// market id
if (cachedMarketIdsLength > 1) {
for (uint256 j; j < params.accountPositionsNotionalValueX18.length; j++) {
sumOfAllPositionsUsdX18 =
sumOfAllPositionsUsdX18.add(params.accountPositionsNotionalValueX18[j]);
}
}
​
// loop into market ids
for (uint256 j; j < cachedMarketIdsLength; j++) {
// collateral native precision -> collateral zaros precision
UD60x18 collateralAmountX18 =
marginCollateralConfiguration.convertTokenAmountToUd60x18(ctx.assetAmount);
​
// if we have more than one market id, we need to calculate the percentage that will be
// deposited for this market
if (cachedMarketIdsLength > 1) {
// calculate the percentage to deposit to this market
UD60x18 percentDeductForThisMarketX18 =
params.accountPositionsNotionalValueX18[j].div(sumOfAllPositionsUsdX18);
collateralAmountX18 = collateralAmountX18.mul(percentDeductForThisMarketX18);
}
​
// deposit the collateral in the market making engine
marketMakingEngine.depositCreditForMarket(
uint128(params.marketIds[j]),
collateralType,
marginCollateralConfiguration.convertUd60x18ToTokenAmount(collateralAmountX18)
);
}
}
​
ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// if there is no missing margin then exit the loop
// since all amounts have been deducted
if (!ctx.isMissingMargin) {
break;
}
//!@audit-Issue: So many things unclear about this function.
}
​

when handling PnL in the deductAccountMargin() function, it checks if pnLUsdX18.get(UD60X18_ZERO)` and proceeds to withdraw from the collateral. But if the PnL is positive then that should be a profit and not a deduction of funds. But here the code treats all PnL as a loss, which is incorrect.

Impact

All traders who make profits will have their margins reduced intead of being increased. As a result this will bring financial loss to the user. For instance if a user trades with a $100 dollar and makes a 100 from them stealing all their profits.

This could likely break users Trust for the system.

Tools Used

Manual Review

Recommendations

One easiest way is to correct the approach by using a signed type like SD59X18 to distinguish between profits (positive) and losses (negative). Also adjust the logic so that profits will be added to the user's margin and likewise losses will be deducted.

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

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