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

In case collateral token is removed from the system it won't be accounted for margin deduction

SBSecurity

Summary

When a position is being liquidated the margin should be deducted from all the tokens that he deposited, but currently, TradingAccount::deductAccountMargin uses collateralLiquidationPriority:

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();//@audit this
...MORE CODE
}

Which is part of the global configuration and is not guaranteed to contain all the collateral types that the user has, especially when valid collaterals types are being removed from GlobalConfiguration:

function removeCollateralFromLiquidationPriority(Data storage self, address collateralType) internal {
// does the collateral being removed exist?
bool isInCollateralLiquidationPriority = self.collateralLiquidationPriority.contains(collateralType);
​
// if not, revert
if (!isInCollateralLiquidationPriority) revert Errors.MarginCollateralTypeNotInPriority(collateralType);
​
// the following code is required since EnumerableSet::remove
// uses the swap-and-pop technique which corrupts the order
​
// copy the existing collateral order
address[] memory copyCollateralLiquidationPriority = self.collateralLiquidationPriority.values();
​
// cache length
uint256 copyCollateralLiquidationPriorityLength = copyCollateralLiquidationPriority.length;
​
// create a new array to store the new order
address[] memory newCollateralLiquidationPriority = new address[](copyCollateralLiquidationPriorityLength - 1);
​
uint256 indexCollateral;
​
// iterate over the in-memory copies
for (uint256 i; i < copyCollateralLiquidationPriorityLength; i++) {
// fetch current collateral
address collateral = copyCollateralLiquidationPriority[i];
​
// remove current collateral from storage set
self.collateralLiquidationPriority.remove(collateral);
​
// if the current collateral is the one we want to remove, skip
// to the next loop iteration
if (collateral == collateralType) {
continue;
}
​
// otherwise add current collateral to the new in-memory
// order we are building
newCollateralLiquidationPriority[indexCollateral] = collateral;
indexCollateral++;
}
​
// the collateral priority in storage has been emptied and the new
// order has been built in memory, so iterate through the new order
// and add it to storage
for (uint256 i; i < copyCollateralLiquidationPriorityLength - 1; i++) {
self.collateralLiquidationPriority.add(newCollateralLiquidationPriority[i]);
}
}

On the other hand, when the margin balance of trader is being calculated TradingAccount::marginCollateralBalanceX18 is used and this is the right set that contains all the collateral of the account:

function getMarginBalanceUsd(
Data storage self,
SD59x18 activePositionsUnrealizedPnlUsdX18
)
internal
view
returns (SD59x18 marginBalanceUsdX18)
{
// cache colllateral length
uint256 cachedMarginCollateralBalanceLength = self.marginCollateralBalanceX18.length();
}

Vulnerability Details

Knowing that information, we can see that in case one of the collateral types is being removed from the collateralLiquidationPriority but it will remain in the trading account’s marginCollateralBalanceX18, since admins don’t have control over it. Then when position is being checked for liquidation the margin collateral will contain the removed collateral types as well, furthermore if position becomes liquidatable, margin falls below the maintenance margin, protocol will be at permanent loss, because deductAccountMargin will not withdraw from the removed collateral types.

function liquidateAccounts(uint128[] calldata accountsIds) external {
...MORE CODE
ctx.marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
​
// if account is not liquidatable, skip to next account
// account is liquidatable if requiredMaintenanceMarginUsdX18 > ctx.marginBalanceUsdX18
if (!TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18)) {
continue;
}
​
// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
}

So not the entire requiredMaintenanceMarginUsdX18 will be withdrawn but it will depend on the amount of removed collateral that the trader has deposited initially.

Important note: this attack can be performed both maliciously or not depending on the intentions of the trader.

Although frontrunning in Arbitrum is not possible addition/removal of collateral types will be under Zaros governance and there will be a proposal that the trader can monitor. The moment that this proposal passes he can simply swap all of his collateral for the token that will be removed and wait until his position becomes liquidatable.

After his position is liquidated and LPs have taken the loss (not part of this audit, but it’s standard in the PnL based systems) trader can freely withdraw this collateral type, as there is no check in the TradingAccount::withdrawMargin whether this type persists in the collateralLiquidationPriority.

function withdrawMargin(uint128 tradingAccountId, address collateralType, uint256 amount) external {
// fetch storage slot for this collateral's config config
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// load existing trading account; reverts for non-existent account
// enforces `msg.sender == owner` so only account owner can withdraw
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExistingAccountAndVerifySender(tradingAccountId);
​
// convert uint256 -> UD60x18; scales input amount to 18 decimals
UD60x18 amountX18 = marginCollateralConfiguration.convertTokenAmountToUd60x18(amount);
​
// enforce converted amount > 0
_requireAmountNotZero(amountX18);
​
// enforces that user has deposited enough collateral of this type to withdraw
_requireEnoughMarginCollateral(tradingAccount, collateralType, amountX18);
​
// deduct amount from trader's collateral balance
tradingAccount.withdraw(collateralType, amountX18);
​
// load account required initial margin requirement & unrealized USD profit/loss
// ignores "required maintenance margin" output parameter
(UD60x18 requiredInitialMarginUsdX18,, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
// get trader's margin balance
SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
​
// check against initial margin requirement as initial margin > maintenance margin
// hence prevent the user from withdrawing all the way to the maintenance margin
// so that they couldn't be liquidated very soon afterwards if their position
// goes against them even a little bit
tradingAccount.validateMarginRequirement(requiredInitialMarginUsdX18, marginBalanceUsdX18, UD60x18_ZERO);
​
// finally send the tokens
IERC20(collateralType).safeTransfer(msg.sender, amount);
​
emit LogWithdrawMargin(msg.sender, tradingAccountId, collateralType, amount);
}

Impact

When collateral type is removed from collateralLiquidationPriority, liquidations will issue less collateral and will allow the trader to take all of his initially deposited collateral back.

Tools Used

Manual Review

Recommendations

Update the code to validate the margin requirements in TradingAccount::getMarginBalanceUsd from collateralLiquidationPriority instead of the marginCollateralBalanceX18. That way when type is removed it is automatically being removed from the collateral margin of the user as well, but this can lead to situation where this removal makes the whole account subject of liquidation if the trader have significant balance.

Updates

Lead Judging Commences

inallhonesty Lead Judge 10 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.