Description
TradingAccount::deductAccountMargin function allows an account to pay fees and losses from its collateral. However, the function prematurely ends the loop when the first non-zero balance token is completely drained.
The premature termination occurs when withdrawMarginUsd returns isMissingMarging as true. This happens whenever the collateral type is insufficient to cover the fee or loss.
function withdrawMarginUsd(
Data storage self,
address collateralType,
UD60x18 marginCollateralPriceUsdX18,
UD60x18 amountUsdX18,
address recipient
)
internal
returns (UD60x18 withdrawnMarginUsdX18, bool isMissingMargin)
{
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
UD60x18 marginCollateralBalanceX18 = getMarginCollateralBalance(self, collateralType);
UD60x18 requiredMarginInCollateralX18 = amountUsdX18.div(marginCollateralPriceUsdX18);
uint256 amountToTransfer;
if (marginCollateralBalanceX18.gte(requiredMarginInCollateralX18)) {
...
} else {
withdraw(self, collateralType, marginCollateralBalanceX18);
amountToTransfer = marginCollateralConfiguration.convertUd60x18ToTokenAmount(marginCollateralBalanceX18);
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
withdrawnMarginUsdX18 = marginCollateralPriceUsdX18.mul(marginCollateralBalanceX18);
@> isMissingMargin = true;
}
}
This implies that if users have a small amount of collateral in the first token of collateralLiquidationPriority, they will avoid the fees and loss deduction during settlement or liquidation.
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
...
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
...
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);
}
Risk
Likelyhood: High
This will always occur during liquidation if users have multiple collaterals and use them all.
This will occur for regular users during settlement if the first collateral is too small to cover the fees.
Impact: High
Proof of Concept
Foundry PoC to add in test/integration/perpetuals/trading-account-branch/deductAccountMargin/deductAccountMargin.t.sol
function testFuzz_BypassFeeAndLiquidation(
)
external
givenTheAccountHasAMarginBalanceOf0
whenThereIsCollateralLiquidationPriority
{
uint marginValueUsd = USDC_MIN_DEPOSIT_MARGIN;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsd });
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
uint feeAmount = USDC_MIN_DEPOSIT_MARGIN *2;
uint randomFeeAmount1 = 0;
uint randomFeeAmount2 = 0;
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdc));
perpsEngine.depositMargin(tradingAccountId, address(usdz), marginValueUsd);
FillOrderContext memory ctx;
ctx.settlementFeeUsdX18 = ud60x18(randomFeeAmount1);
UD60x18 pnlUsdX18 = ud60x18(feeAmount);
UD60x18 orderFeeUsdX18 = ud60x18(randomFeeAmount2);
assertEq(0, usdc.balanceOf(MSIG_ADDRESS));
assertEq(0, usdz.balanceOf(MSIG_ADDRESS));
TradingAccountHarness(address(perpsEngine)).exposed_deductAccountMargin({
tradingAccountId: tradingAccountId,
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: MSIG_ADDRESS,
orderFeeRecipient: MSIG_ADDRESS,
settlementFeeRecipient: MSIG_ADDRESS
}),
pnlUsdX18: pnlUsdX18,
orderFeeUsdX18: orderFeeUsdX18,
settlementFeeUsdX18: ctx.settlementFeeUsdX18
});
assertEq(marginValueUsd, usdz.balanceOf(MSIG_ADDRESS));
assertEq(marginValueUsd, usdc.balanceOf(MSIG_ADDRESS));
}
Recommended Mitigation
Instead of prematurely ending the loop, continue to check all the different collaterals to ensure the loss is covered until the user can no longer pay.
