Summary
When liquidating an account or filling an order, fees are deducted by iterating through the deposited collateral until all required fees are paid. Ref:
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L488-L578
However, there is no minimum deposit enforced in depositMargin, allowing users to deposit very small amounts like 1 USDC instead of 1e6, or to reach fractional values over time after paying fees.
This becomes problematic with tokens that revert on zero-value transfers, as detailed here: https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers. When the protocol scales down to token decimals, fractional values can become zero.
This can cause a denial of service (DoS) during settlement and liquidation due to zero-value transfers.
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L462
The function withdrawMarinUsdwill attempt to transfer 0 when marginCollateralBalanceX18cannot cover the entire requiredMarginInCollateralX18.
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)) {
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;
}
}
Note that withdrawMarginUsd is used to deduct all fees from deductAccountMarginwhile it iterates through all the collaterals:
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;
}
}
}
PoC
Scenario for testing:
Create a long position for BTC using WETH as a collateral
deposit 1 in USDC and make the long position liquidatable by dropping the BTC price.
try to liquidate the long position.
Transaction reverts due to zero transfer amount.
Add the following snippet code on MockERC20.solto simulate a revert on zero transfer token:
function transfer(address to, uint256 value) public override virtual returns (bool) {
require(value != 0, "zero-value-transfer");
return super.transfer(to, value);
}
Then add the following test on checkLiquidatableAccounts.t.soland run: forge test --match-test testDoS_onDeductionFees_dueToSmallAmountForCollateral
function testDoS_onDeductionFees_dueToSmallAmountForCollateral() public {
uint256 USER_STARTING_BALANCE = 100e18;
uint256 USER_STARTING_BALANCE_USDC = 1;
int128 USER_POS_SIZE_DELTA = 10e18;
address collateralType = address(wEth);
deal({ token: collateralType, to: users.naruto.account, give: USER_STARTING_BALANCE });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(USER_STARTING_BALANCE, collateralType);
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, USER_POS_SIZE_DELTA);
skip(1 minutes);
deal({ token: address(usdc), to: users.naruto.account, give: USER_STARTING_BALANCE_USDC });
perpsEngine.depositMargin(tradingAccountId, address(usdc), USER_STARTING_BALANCE_USDC);
updateMockPriceFeed(BTC_USD_MARKET_ID, 45_000e18);
uint128[] memory liquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(0, 1);
assertEq(liquidatableAccountIds[0], 1);
changePrank({ msgSender: liquidationKeeper });
vm.expectRevert("zero-value-transfer");
perpsEngine.liquidateAccounts(liquidatableAccountIds);
}
Output: transaction reverts with zero-value-transfer.
[PASS] testDoS_onDeductionFees_dueToSmallAmountForCollateral() (gas: 1654621)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.78ms (3.00ms CPU time)
Impact
Denial of Service (DoS): Settlement and liquidation processes can fail if a zero-value transfer occurs.
Unsettled Orders: Fees cannot be deducted, leaving orders unfulfilled.
Account Liquidation Failure: Liquidation of undercollateralized accounts may be blocked, risking the protocol's financial stability.
User Fund Lock: Users may be unable to withdraw or transfer their funds due to failed transactions.
Tools Used
Manual Review & Foundry
Recommendations
Only transfer the amount when the value is > 0.
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);
+ if (amountToTransfer > 0)
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
withdrawnMarginUsdX18 = marginCollateralPriceUsdX18.mul(marginCollateralBalanceX18);
isMissingMargin = true;
}
(Additional) Consider adding a minimum value for the deposit.
