DeFiFoundry
60,000 USDC
View results
Submission Details
Severity: medium
Valid

Users can withdraw all his margins and back his position with only positive PNL to avoid paying fees during liquidation

Summary

Users can withdraw all his margins and back his position with only positive PNL to avoid paying fees during liquidation

Vulnerability Details

When a user deposits some margin, opens a position and that position get positive unrealised PNL, he can withdraw all his deposited margin. At this point the position will be backed entirely by the unrealised PNL. The problem here is that if the position gets to a liquidable state, no fees will be deducted to the user because he does not have any margin, so no fees will be sent to receivers.
Let's take a look at how it works.

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

When a user wants to withdraw margin that he deposited this code is executed, in it we can extract the following checks:

  1. Verify that the caller is the owner of the position

  2. Enforce that the amount to withdraw is greater than 0

  3. Enforce that the amount to withdraw is less than or equal to the margin deposited

  4. With the margin removed from the account, ensure that the margin + PnL can back the position

  5. Transfer the requested margin to the user

As we can see, if a user has enough PnL can fully back his position and withdraw all his margin. With this state, if the position was liquidated it would execute the following:

function liquidateAccounts(uint128[] calldata accountsIds) external {
...
for (uint256 i; i < accountsIds.length; i++) {
...
// 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
});
...
}
}
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;
...
}
// output total margin deducted
marginDeductedUsdX18 =
ctx.settlementFeeDeductedUsdX18.add(ctx.orderFeeDeductedUsdX18).add(ctx.pnlDeductedUsdX18);
}

As we can see, the liquidation function will try to deduct the liquidation fee and the required maintenance margin but when the deductAccountMargin will be called, since the position will not have any margin, the loop will not execute any transfer and the fee receiver will not receive anything.

Impact

This situation can be dangerous for the protocol because keepers should get fees to pay for the service and also the protocol should get at least the maintenance margin in order to have solvency.

Tools Used

Manual review

Recommendations

There is not a trivial solution, but I would recommend to only allow to withdraw the margin only if it keeps a minimum to cover some fees.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

A user with a big enough positive PnL can withdraw his margin and avoid liquidation fee in case they get liquidated

Support

FAQs

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