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

If attacker's unsettled loss is greater than available margin, the remaining loss will be wiped out

Oblivionis

Summary

If attacker's unsettled loss is greater than available margin, the remaining loss will be wiped out. Attacker can craft attack to direct theft margin from protocol.

Vulnerability Details

For users who wish to change their existing positions, the following is the logic executed by _fillOrder: (We omit some security checks for now)

  1. fetch global config, perp market's config, account config, etc from storage

  2. get funding rates for this perp market

  3. update funding rates

  4. calculate order & settlement fees

  5. calculate required initial & maintenance margin for this trade and account's unrealized PNL

  6. Ensure that the user has sufficient margin (based on calculations in step 5)

  7. cache unrealized PNL from potential existing position in this market including accrued funding

  8. update all config storage

  9. If unrealized PnL > 0, mint new usdTokens for trader's margin account

  10. If unrealized PnL < 0, deduct collateral and order/settlement fees from trader's margin account

Let's dive into step 10:

// pay order/settlement fees and deduct collateral
// if trader's old position had negative pnl
//@Audit return value not checked!
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
});

And the implementation of tradingAccount.deductAccountMargin():

/// @notice Deducts the account's margin to pay for the settlement fee, order fee, and realize the pnl.
/// @param self The trading account storage pointer.
/// @param feeRecipients The fee recipients.
/// @param pnlUsdX18 The total unrealized PnL of the account.
/// @param settlementFeeUsdX18 The total settlement fee to be deducted from the account.
/// @param orderFeeUsdX18 The total order fee to be deducted from the account.
/// @return marginDeductedUsdX18 The total margin deducted from the account.
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
// working data
DeductAccountMarginContext memory ctx;
​
// fetch storage slot for global config
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
​
// cache collateral liquidation priority length
uint256 cachedCollateralLiquidationPriorityLength = globalConfiguration.collateralLiquidationPriority.length();
​
// loop through configured collateral types
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
// get ith collateral type
address collateralType = globalConfiguration.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 (settlementFeeUsdX18.gt(UD60x18_ZERO) && ctx.settlementFeeDeductedUsdX18.lt(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,
settlementFeeUsdX18.sub(ctx.settlementFeeDeductedUsdX18),
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 (orderFeeUsdX18.gt(UD60x18_ZERO) && ctx.orderFeeDeductedUsdX18.lt(orderFeeUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
orderFeeUsdX18.sub(ctx.orderFeeDeductedUsdX18),
feeRecipients.orderFeeRecipient
);
ctx.orderFeeDeductedUsdX18 = ctx.orderFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// pnl logic same as settlement & order fee above
if (pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(pnlUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
feeRecipients.marginCollateralRecipient
);
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;
}
}
​
// output total margin deducted
marginDeductedUsdX18 =
ctx.settlementFeeDeductedUsdX18.add(ctx.orderFeeDeductedUsdX18).add(ctx.pnlDeductedUsdX18);
}

As shown above, deductAccountMargin() iterates through all of the user's margin types and attempts to deduct orderFee, settlementFee and pnl from the user's account.

It is worth noting that deductAccountMargin() allows the difference between user's margin and (orderFee + settlementFee + pnl) to be skipped - if the user's available margin is insufficient to cover all three charges, then the function will exit normally and return the value of total margin deducted.(This is to allow negative margin positions to be liquidated properly as well) However, in SettlementBranch, the return value is not examined, which means when filling a trade, protocol assumes that the user's available margin is sufficient to cover losses in a given market.

In fact, this assumption is incorrect. An attacker can abuse this mechanism to wipe out his losses for free.

Consider such scenario: (For simplicity's sake, we'll ignore all fees for now, as they are a fairly small percentage of the position and are not important to this issue)

  1. Alice has 10,000usd margin and has two positions now: size = +10,000usd at market A; size = -20,000usd at market B.

  2. As token prices change, the prices of both A and B rise dramatically, makes the pnl reach:

    market A: unrealized PnL = 30,000usd

    market B: unrealized PnL = -15,000usd

    Alice now has a full position with a profit of $15,000, and her account is very healthy.

  3. When Alice tries to place an tiny order for Market B, deductAccountMargin()will try to subtract 15,000usd from Alice‘s margin, but there's only 10,000 usd available. Due to the mechanism we described above, Alice's $5,000 loss was just ignored by the contract.

  4. Alice then closes position at market B, now she has 0 margin balance and +30000usd PnL in market A.

    Alice now has a full position with a profit of $20,000. The extra $5,000 in bad debt is covered by protocol.

Impact

Likelihood: medium - Attacks depend on a large profitable position and a large losing position

+

Impact: Critical - direct theft of margin

=

Severity: High

Tools Used

Manual review

Recommendations

If the user's negative unsettled PnL is greater than the current margin, a mechanism similar to forced position reduction should be introduced to ensure that the user pays a sufficiently large amount.

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

Oblivionis Submitter
9 months ago
Oblivionis Submitter
9 months ago
inallhonesty Lead Judge
9 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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