Submission Details
Severity:
Liquidations can fall into DoS for collaterals that does not support 0 value transfer
## Summary
Accounts can become unliquidable if the account has a collateral that reverts when transferring 0 value.
## Vulnerability Details
[When deducting margin from an account being liquidated](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L488-L578), the logic is to iterate over the collateralLiquidationPriority and attempt to deduct from the TradingAccount the most of each collateral where the account has balance until the total amount to be deducted is paid off.
- If a TradingAccount has any balance of the current collateral on the collateralLiquidationPriority, then, the execution will try to deduct either the liquidationFee or the requiredMargin to pay. If the account runs out of collateral paying the first fee, either way, the logic will try to deduct the second fee from the account using the same collateral.
- Here is where the problem can occur if the account runs out of the current collateral after paying the first fee when attempting to deduct the second fee, the collateral already has no more of that collateral, so, the [`TradingAccount.withdrawMarginUsd() function`](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L434-L469) will attempt to do a transfer of all the remaining of that collateral, which is 0, so, the execution will attempt to [transfer 0 value of that collateral](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L464), which, if the collateralToken doesn't allow 0 value transfer, the whole tx could be reverted, effectively DoSing the liquidation process.
```
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
...
// loop through configured collateral types
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
/...
// skip to next loop iteration if trader hasn't deposited any of this collateral
if (ctx.marginCollateralBalanceX18.isZero()) continue;
// get this collateral's USD price
ctx.marginCollateralPriceUsdX18 = marginCollateralConfiguration.getPrice();
// if:
// settlement fee > 0 AND
// amount of settlement fee paid so far < settlement fee
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
//@audit => Let's suppose all the collateral of collateralType the liquidated account has is exactly `settlementFeeUsdX18`.
//@audit => After covering this fee, the account will have 0 balance of this collateral type
(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);
}
// pnl logic same as settlement & order fee above
if (pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(pnlUsdX18)) {
//@audit => Now, even though the account has already run out of balance for the current collateralType, the execution will attempt to deduct `pnlUsdX18.sub(ctx.pnlDeductedUsdX18)`
//@audit-issue => Here is where the problem can occur, let's see the withdrawMaringUsd() down below!
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
feeRecipients.marginCollateralRecipient
);
ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
...
}
...
}
function withdrawMarginUsd(
Data storage self,
address collateralType,
UD60x18 marginCollateralPriceUsdX18,
UD60x18 amountUsdX18,
address recipient
)
internal
returns (UD60x18 withdrawnMarginUsdX18, bool isMissingMargin)
{
...
//@audit => If the account has enough collateral to cover the specified `amountUsd`, no problems!
if (marginCollateralBalanceX18.gte(requiredMarginInCollateralX18)) {
...
} else {
//@audit => If the account DOES NOT have enough collateral, the execution attempts to transfer whatever remaining collateral balance is left for the account
//@audit-issue => When an account has 0 balance of the collateralType, the below transfer will attempt to transfer 0 value
//@audit-issue => If the collateralToken doesn't allow transfer of 0 value, the execution will be reverted, effectively reverting the liquidation process.
withdraw(self, collateralType, marginCollateralBalanceX18);
amountToTransfer = marginCollateralConfiguration.convertUd60x18ToTokenAmount(marginCollateralBalanceX18);
//@audit-issue => If collateralType doesn't allow 0 value transfer, execution can be reverted.
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
withdrawnMarginUsdX18 = marginCollateralPriceUsdX18.mul(marginCollateralBalanceX18);
isMissingMargin = true;
}
}
```
## Impact
DoSing Liquidations
## Tools Used
Manual Audit & [Solodit Report](https://solodit.xyz/issues/m-13-transaction-revert-if-the-basetoken-does-not-support-0-value-transfer-when-charging-changefee-code4rena-caviar-caviar-private-pools-git)
## Recommendations
The recommendation would be to add a check if the `amountToTransfer > 0`, if not, then don't attempt the transfer.
function withdrawMarginUsd(
Data storage self,
address collateralType,
UD60x18 marginCollateralPriceUsdX18,
UD60x18 amountUsdX18,
address recipient
)
internal
returns (UD60x18 withdrawnMarginUsdX18, bool isMissingMargin)
{
...
if (marginCollateralBalanceX18.gte(requiredMarginInCollateralX18)) {
...
} else {
...
amountToTransfer = marginCollateralConfiguration.convertUd60x18ToTokenAmount(marginCollateralBalanceX18);
+ if(amountToTransfer > 0 ) IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
withdrawnMarginUsdX18 = marginCollateralPriceUsdX18.mul(marginCollateralBalanceX18);
isMissingMargin = true;
}
}
```
Updates
Lead Judging Commences
inallhonesty 11 months ago
Submission Judgement Published Reason: Non-acceptable severity
Assigned finding tags:
Liquidations can fall into DoS for collaterals that does not support 0 value transfer
Appeal created
inallhonesty 10 months ago
Submission Judgement Published Reason: Non-acceptable severity
Assigned finding tags:
Liquidations can fall into DoS for collaterals that does not support 0 value transfer
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.