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

`removeCollateralFromLiquidationPriority` do not check if `marginCollateralConfiguration.totalDeposited == 0` before, making it possible to have an un-liquidatable margin balance

infect3d

Summary

It is possible for a user to have an unliquidatable collateral balance, as even though TradingAccountBranch::depositMargin check that collateral has configured liquidation priority, if admin decide to call GlobalConfiguration::removeCollateralFromLiquidationPriority for this particular collateral, it is possible that in a same block a user tx to deposit is executed before the admin call.
This balance will then count as margin balance, but will not be accessible to liquidators.

Vulnerability Details

This can happen by race-condition, which means one tx can simply be placed before the other by bad timing. Especially because on Arbitrum there is no public mempool, and transactions are prioritized on a first-come first-served basis.
This means operator/admin have no options to make sure no one sends collateral right before it get removed from the liquidation priority list.

While the TradingAccountBranch::depositMargin function ensure that the collateral is on the liquidation priority list L342:

File: src/perpetuals/branches/TradingAccountBranch.sol
317: function depositMargin(uint128 tradingAccountId, address collateralType, uint256 amount) public virtual {
...:
...: //* ---- some code ---- *//
...:
340:
341: // enforce collateral has configured liquidation priority
342:✅ _requireCollateralLiquidationPriorityDefined(collateralType);
343:
344: // get the tokens first
345: IERC20(collateralType).safeTransferFrom(msg.sender, address(this), amount);
346:
347: // then perform the actual deposit
348: tradingAccount.deposit(collateralType, amountX18);
349:
350: emit LogDepositMargin(msg.sender, tradingAccountId, collateralType, amount);
351: }

While (1) GlobalConfiguration::removeCollateralFromLiquidationPriority do not check if marginCollateralConfiguration.totalDeposited is zero which is understandable to avoid DoS, (2) TradingAccount::getMarginBalanceUsd will count this collateral as user's margin balance.

But we see that TradingAccount::getMarginBalanceUsd is used extensively inside the protocol to calculate user's margin balance, to decide if he can create a market order, if the order can be filled, and if the order must be liquidated

Finally, the last piece that make it risk-free for the user is that TradingAccount::deductAccountMargin which is used to seize user's collateral during liquidation or when an order is updated to seize losses is only able to seize collaterals that are in the collateralLiquidationPriority list:

File: src/perpetuals/leaves/TradingAccount.sol
488: function deductAccountMargin(
489: Data storage self,
490: FeeRecipients.Data memory feeRecipients,
491: UD60x18 pnlUsdX18,
492: UD60x18 settlementFeeUsdX18,
493: UD60x18 orderFeeUsdX18
494: )
495: internal
496: returns (UD60x18 marginDeductedUsdX18)
497: {
...:
...: //* ---- some code ---- *//
...:
503:
504: // cache collateral liquidation priority length
505: uint256 cachedCollateralLiquidationPriorityLength = globalConfiguration.collateralLiquidationPriority.length();
506:
507: // loop through configured collateral types
508: for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
509: // get ith collateral type
510: address collateralType = globalConfiguration.collateralLiquidationPriority.at(i);
511:
512: // fetch storage slot for this collateral's config config
513: MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
514: MarginCollateralConfiguration.load(collateralType);
515:
516: // get trader's collateral balance for this collateral, scaled to 18 decimals
517: ctx.marginCollateralBalanceX18 = getMarginCollateralBalance(self, collateralType);
518:
519: // skip to next loop iteration if trader hasn't deposited any of this collateral
520: if (ctx.marginCollateralBalanceX18.isZero()) continue;
521:
522: // get this collateral's USD price
523: ctx.marginCollateralPriceUsdX18 = marginCollateralConfiguration.getPrice();
...:
...: //* ---- some code ---- *//
...:
555:
556: // pnl logic same as settlement & order fee above
557: if (pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(pnlUsdX18)) {
558: (ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
559: self,
560: collateralType,
561: ctx.marginCollateralPriceUsdX18,
562: pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
563: feeRecipients.marginCollateralRecipient
564: );
...:
...: //* ---- some code ---- *//
...:
578: }

What make it even worse is that if user's position PnL is positive, he will get its profit credited.

Impact

A user can open risk-less positions with an unliquidatable collateral, and as such steal funds.
When his position will be liquidated, nothing will be seized as user has no seizable collateral.
If user is in profit, he will be able to get rewarded.

While this require some 'luck' to have the deposit hapenning right before the removal of call, the odds to succeed can be increased as the attacker will most probably know when a collateral will be removed from the list, and make a huge deposit in the same block. The attacker could also simply regularly deposit/withdraw collateral if he has no idea when a collateral will be removed from the list.

Tools Used

Manual review

Recommendations

The fix is pretty simple, only collaterals that are in the collateralLiquidationPriority should be accounted as user's margin balance.

diff --git a/src/perpetuals/leaves/TradingAccount.sol b/src/perpetuals/leaves/TradingAccount.sol
index b573fae..f9dcd27 100644
--- a/src/perpetuals/leaves/TradingAccount.sol
+++ b/src/perpetuals/leaves/TradingAccount.sol
@@ -181,6 +181,12 @@ library TradingAccount {
for (uint256 i; i < cachedMarginCollateralBalanceLength; i++) {
// read key/value from storage for current iteration
(address collateralType, uint256 balance) = self.marginCollateralBalanceX18.at(i);
+ GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
+ bool isInCollateralLiquidationPriority =
+ globalConfiguration.collateralLiquidationPriority.contains(collateralType);
+ if(!isInCollateralLiquidationPriority)
+ continue;
​
// load collateral margin config for this collateral type
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

removal of collateral types from the liquidation priority list will affect affect the liquidation process of the existing positions

Support

FAQs

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