bug: L-19. Fees are not sent to their respective recipients when dealing with low decimals tokens

[0xjoaovictor]

Bug Report

• You can use the develop branch as a reference
• Please create an integration and unit test for this scenario.

L-19. Fees are not sent to their respective recipients when dealing with low decimals tokens

Submitted by S3v3ru5, nfmelendez, Greed, Infect3d, jsmi, Oblivionis. Selected submission by: Greed.

Summary

In order to handle low decimals tokens everywhere in the protocol, Zaros starts by scaling such token amount to 18 decimals so rounding errors are avoided.

When users adjust their positions, a part of their trades are collected as fees and sent to the fee recipients. There are 3 types of fees :

• settlement fees
• order fees
• PnL fees

The fees are collected when an account is being liquidated or an order is fulfilled, in the SettlementBranch::_fillOrder() and LiquidationBranch::liquidateAccounts() functions respectively.

These 2 functions use another interal function called TradingAccount::deductAccountMargin() where all the logic is implemented.

This function will loop through all the allowed collateral and give the due scaled USD value to withdrawMarginUsd()

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L528-L566

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

In withdrawMarginUsd(), before transferring the tokens to the recipients, the scaled token amount will be down scaled using the MarginCollateralConfiguration::convertUd60x18ToTokenAmount() function.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L434-L469

withdraw(self, collateralType, requiredMarginInCollateralX18);
amountToTransfer =
    marginCollateralConfiguration.convertUd60x18ToTokenAmount(requiredMarginInCollateralX18);

IERC20(collateralType).safeTransfer(recipient, amountToTransfer);

withdrawnMarginUsdX18 = amountUsdX18;
isMissingMargin = false;

Vulnerability Details

The issue is that the given requiredMarginInCollateralX18 is so small, when the downscaling occurs, the returned token amount will equal 0.

When dealing with WBTC for example, the requiredMarginInCollateralX18 will be divided by 1e10 which will round to 0.

So the contract will transfer 0 WBTC to the fee recipients, leaving this fee in the contract but still reducing the trader's account margins as if the transfer actually happened.

Currently, the supported tokens allow 0 amount transfers but the protocol should keep in mind that some tokens revert when attempting to transfer zero tokens.

This coded PoC can be pasted in test\integration\perpetuals\perp-market-branch\getFundingRate\getFundingRate.t.sol and demonstrates a user performing various operations that should be collecting fees. However, the recipients never receive them.

function testBypassFeesAndDustStuck() external {
    uint256 btcBalance = 10e8;

    deal({ token: address(wBtc), to: users.naruto.account, give: btcBalance });

    vm.startPrank(users.naruto.account);
    uint128 tradingAccountId = createAccountAndDeposit(btcBalance, address(wBtc));
    int128 narutoPosSizeDelta = int128(uint128(btcBalance));

    // recipients start with no WBTC
    assertEq(wBtc.balanceOf(users.orderFeeRecipient.account), 0);
    assertEq(wBtc.balanceOf(users.settlementFeeRecipient.account), 0);
    assertEq(wBtc.balanceOf(users.liquidationFeeRecipient.account), 0);

    // various operations are performed over time which should generate fees
    openManualPosition(ETH_USD_MARKET_ID, ETH_USD_STREAM_ID, MOCK_ETH_USD_PRICE, tradingAccountId, int128(ETH_USD_MIN_TRADE_SIZE) * 1_000);

    skip(5 days);
    openManualPosition(ETH_USD_MARKET_ID, ETH_USD_STREAM_ID, MOCK_ETH_USD_PRICE, tradingAccountId, int128(ETH_USD_MIN_TRADE_SIZE) * 1_000);
    
    updateMockPriceFeed(ETH_USD_MARKET_ID, MOCK_ETH_USD_PRICE - (MOCK_ETH_USD_PRICE / 10)); // price drops -10%
    skip(5 days);
    
    // user closes his position
    openManualPosition(ETH_USD_MARKET_ID, ETH_USD_STREAM_ID, MOCK_ETH_USD_PRICE, tradingAccountId, -int128(ETH_USD_MIN_TRADE_SIZE * 2_000));

    assertEq(wBtc.balanceOf(address(perpsEngine)), btcBalance);

    // user withdraws the maximum collateral he can
    perpsEngine.withdrawMargin(tradingAccountId, address(wBtc), 999999999);

    // user retrieves almost his whole collateral, leaving 1 wei of dust in the contract
    assertEq(wBtc.balanceOf(address(perpsEngine)), 1);
    assertEq(wBtc.balanceOf(users.naruto.account), 999999999);

    // recipients earned no fees
    assertEq(wBtc.balanceOf(users.orderFeeRecipient.account), 0);
    assertEq(wBtc.balanceOf(users.settlementFeeRecipient.account), 0);
    assertEq(wBtc.balanceOf(users.liquidationFeeRecipient.account), 0);
}

Impact

Fee recipients do not receive their due and these fees end up stuck in the contract.

Tools Used

Manual review

Recommendations

The issue can hardly be patched because the fee percentages are so small, the downscaling will most likely round to 0.

An idea when this happens would be to collect a flat amount of tokens.