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

incorrect implementation of deductAccountMargin functionality leads to wrong accounting

inzinko

Summary

The protocol uses a liquidations priority list to loop over the collateral types and use that for operations about settlement and liquidations in deducting margin from the user balance, an incorrect assumption will enable the loop to go on without reverting and allow the code to make a wrong implementation to occur and disrupt the operation of the protocol

Vulnerability Details

SettlementBranch and LiquidationBranch both call TradingAccount::deductAccountMargin whenever they want to perform an operation like removing collateral or paying for fees, the main vulnerability exists in the deductAccountMargin function so we will explore it

TradingAccount::deductMargin L510 - L516

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

Inside the function we see that the liquidationPriority is loop over to get the collateral type to deduct from

TradingAccount::deductMargin L529

if (ctx.marginCollateralBalanceX18.isZero()) continue;

This prevents the loop from going on if the collateral balance is finished, but it is a critically wrong assumption because here the code assumes that the collateral balance will only be zero after the end of the loop, this is critically wrong as would be shown with further explanation below

TradingAccount::deductAccountMargin L534 - L560

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);
}
...more code

In this Snippet there is a peculiar situation to take a look at these two block of code tries to remove tokens using withdrawMarginUsd, this function withdraws the given amount of margin and transfers them to the recipient, but it also levrages another function withdraw when doing it, this function deducts the margin to be removed from the collateral balance, two snippets would be shown to explain what exactly is going on, for both functions
withdrawMarginUsd

if (marginCollateralBalanceX18.gte(requiredMarginInCollateralX18)) {
withdraw(self, collateralType, requiredMarginInCollateralX18);
amountToTransfer =
marginCollateralConfiguration.convertUd60x18ToTokenAmount(requiredMarginInCollateralX18);
​
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
​
withdrawnMarginUsdX18 = amountUsdX18;
isMissingMargin = false;
} else {
withdraw(self, collateralType, marginCollateralBalanceX18);
amountToTransfer = marginCollateralConfiguration.convertUd60x18ToTokenAmount(marginCollateralBalanceX18);
​
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
​
withdrawnMarginUsdX18 = marginCollateralPriceUsdX18.mul(marginCollateralBalanceX18);
isMissingMargin = true;
}

withdraw

function withdraw(Data storage self, address collateralType, UD60x18 amountX18) internal {
// load address : collateral map for this account
EnumerableMap.AddressToUintMap storage marginCollateralBalanceX18 = self.marginCollateralBalanceX18;
​
// get currently deposited scaled-to-18-decimals this account has
// for this collateral type, then subtracts newly withdrawn amount also scaled to 18 decimals
UD60x18 newMarginCollateralBalance = getMarginCollateralBalance(self, collateralType).sub(amountX18);
​
if (newMarginCollateralBalance.isZero()) {
// if no more collateral, remove this collateral type
// from this account's deposited collateral
marginCollateralBalanceX18.remove(collateralType);
} else {
// else convert updated scaled-to-18-decimals UD60x18 to uint256 and stores
// it in address : collateral map for this account
marginCollateralBalanceX18.set(collateralType, newMarginCollateralBalance.intoUint256());
}
​
// load collateral configuration for this collateral type
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// update total deposited for this collateral type
marginCollateralConfiguration.totalDeposited =
ud60x18(marginCollateralConfiguration.totalDeposited).sub(amountX18).intoUint256();
}

Now when viewing both together with the blocks of code in the deductAccountMargin we can now get to the root of the issue,

  • When deductAccountMargin is called it loops over collateral type to remove collateral, and remove fees and pnl as shown in the blocks of code, there are three deductions happening settlementfees, orderfees and pnl

  • Before this three block execute a check for the zero balance of the collateralType is made

  • in the first block which is settlement fees, when it makes a call to the withdrawMarginUsd it enters one of the if/else block and a call to withdraw is made

  • when that call is made inside the withdraw function, this is where the issue occurs, inside the function is subtracts the collateral to be removed from the balance and makes a check to see if the balance is zero, it removes it from the user list of collateral if it is, in our example this is what happens the amount to be removed will be equal to the balance of the collateral type which will lead to its removal

  • now inside the current loop of the deductAccountMargin the second block, order fee will be executed with the same collateralType that has already being removed from the user enumerable set !

  • This becomes an issue inside the withdrawMarginUsd where it checks the balance of the removed collateralType, which would be zero and tries to withdraw again

  • Inside withdraw the fact that the collaterType has been removed does not affect the execution has Open zepplin enumerable set does not revert, if a key does not exist, it will return false instead which means this if statement will execute

if (newMarginCollateralBalance.isZero()) {
// if no more collateral, remove this collateral type
// from this account's deposited collateral
marginCollateralBalanceX18.remove(collateralType);
}

  • Then it will Incorrectly subtract the amount from the totalDeposited for the collateralType as shown

marginCollateralConfiguration.totalDeposited =
ud60x18(marginCollateralConfiguration.totalDeposited).sub(amountX18).intoUint256();

  • inside the else block the execution will still not revert even though the collateral balance is zero which will return back to the deductAccountMargin loop where it will do this again for the pnl and incorrectly subtract the amount from the totalCollateral type deposited amount, before it moves on to the next loop and starts the process to get all fees deducted again, and if the other collateral has the same issue as the first one described in this report, it will also subtract from collateralType total deposited wrongly, it will do this until it finds the collateral that will pay for all the amount to deduct, but the damage to the marginCollateralConfiguration.totalDeposited would have been extensive.

Impact

SInce the marginCollateralConfiguration.totalDeposited is a crritcal parameter used to limit the total amount of a collateralType deposited to the protocol as shown below

UD60x18 totalCollateralDepositedX18 = ud60x18(marginCollateralConfiguration.totalDeposited);
​
// enforce converted amount > 0
_requireAmountNotZero(amountX18);
​
// enforce new deposit + already deposited <= deposit cap
_requireEnoughDepositCap(collateralType, amountX18, depositCapX18, totalCollateralDepositedX18);

This will allow over the cap deposit and will also Distort the whole accounting of the protocol, impacting it functionality

Tools Used

Manual Review

Recommendations

There were many times during the tracing of the call stack that presented an opportunity for a malicious user to prevent liquidations but a revert could just not happen when the collateral balance is zero, and because no reverts, that attack was mitigated, but that also lead to this issue of updating the total deposited collateral balance, a check should be implemented in the withdraw function to prevent an update when the balance is zero.

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Liquidations can fall into DoS for collaterals that does not support 0 value transfer

Support

FAQs

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