Summary
The deductAccountMargin function in the has a logical issue where it may fail to fully deduct the required margin from an account’s collateral. This issue arises due to the function not properly handling cases where there’s still a missing margin after looping through all collateral types.
Vulnerability Details
deductAccountMargin Function in TradingAccount.sol is implemented as follows:
TradingAccount.sol#L488-L578
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
DeductAccountMarginContext memory ctx;
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
uint256 cachedCollateralLiquidationPriorityLength = globalConfiguration.collateralLiquidationPriority.length();
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
address collateralType = globalConfiguration.collateralLiquidationPriority.at(i);
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
ctx.marginCollateralBalanceX18 = getMarginCollateralBalance(self, collateralType);
if (ctx.marginCollateralBalanceX18.isZero()) continue;
ctx.marginCollateralPriceUsdX18 = marginCollateralConfiguration.getPrice();
if (settlementFeeUsdX18.gt(UD60x18_ZERO) && ctx.settlementFeeDeductedUsdX18.lt(settlementFeeUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
settlementFeeUsdX18.sub(ctx.settlementFeeDeductedUsdX18),
feeRecipients.settlementFeeRecipient
);
ctx.settlementFeeDeductedUsdX18 = ctx.settlementFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
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);
}
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 (!ctx.isMissingMargin) {
break;
}
}
marginDeductedUsdX18 =
ctx.settlementFeeDeductedUsdX18.add(ctx.orderFeeDeductedUsdX18).add(ctx.pnlDeductedUsdX18);
}
The function loops through all types of collateral in the order specified by globalConfiguration.collateralLiquidationPriority.
For each type of collateral, it tries to deduct the required amounts (settlementFeeUsdX18, orderFeeUsdX18, pnlUsdX18) from the account’s margin.
If the full amount cannot be deducted, withdrawMarginUsd sets isMissingMargin to true.
If isMissingMargin is true, the function continues to the next type of collateral.
If isMissingMargin is false (i.e., the full amount has been deducted), the function breaks out of the loop.
The problem arises when the function has gone through all types of collateral and isMissingMargin is still true. In this case, the function simply returns the total amount that was able to be deducted (marginDeductedUsdX18), without any indication that it failed to deduct the full required amount.
As such, the deductAccountMargin function is called within the liquidateAccounts and _fillorder functions to deduct the required maintenance margin from the account’s collateral.
LiquidationBranch.sol#L152-L161
function liquidateAccounts(uint128[] calldata accountsIds) external {
....snip....
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
....snip....
}
SettlementBranch.sol#L495-L503)
function _fillOrder(
uint128 tradingAccountId,
uint128 marketId,
uint128 settlementConfigurationId,
SD59x18 sizeDeltaX18,
UD60x18 fillPriceX18
)
internal
virtual
{
...snip...
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
});
...snip...
}
Impact
When called by the liquidateAccounts function, a partial margin deduction may result in incomplete liquidations. Accounts that should be fully liquidated might only be partially liquidated, leaving the protocol exposed to undercollateralized positions.
In the context of _fillOrder, if the full required margin isn't deducted, orders may be executed without proper collateralization. This could lead to traders holding positions with insufficient margin, increasing the protocol's risk.
Tools Used
Manual Review
Recommendations
Implement a check at the end of the deductAccountMargin function to verify if the full required amount was deducted.
If the full amount wasn't deducted, either:
a. Revert the transaction with a clear error message, or
b. Return a boolean flag along with the deducted amount to indicate if the full deduction was successful.
Update the calling functions (e.g., liquidateAccounts, _fillOrder) to handle cases where the full margin couldn't be deducted.
