bug: M-08. when fillOrder() , small pnl can cause orderFee/settlementFee to not be fully collected

[0xjoaovictor]

Bug Report

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

M-08. when fillOrder() , small pnl can cause orderFee/settlementFee to not be fully collected

Submitted by giraffe0x, 0x1982. Selected submission by: 0x1982.

Summary

withdrawMarginUsd() uses round down to compute requiredMarginInCollateralX18

May result in fillOrder() part of fee not being collected

Vulnerability Details

in fillOrder()
We will deduct 3 fees settlementFee , orderFee , pnl

fillOrder() -> deductAccountMargin()

    function deductAccountMargin(
        Data storage self,
        FeeRecipients.Data memory feeRecipients,
        UD60x18 pnlUsdX18,
        UD60x18 settlementFeeUsdX18,
        UD60x18 orderFeeUsdX18
    )
...
        for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
            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);
            }

            // 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)) {
                (ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
                    self,
                    collateralType,
                    ctx.marginCollateralPriceUsdX18,
                    pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
                    feeRecipients.marginCollateralRecipient
                );
                ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
            }

            // if there is no missing margin then exit the loop
            // since all amounts have been deducted
@>          if (!ctx.isMissingMargin) {
                break;
            }
        }


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

The problem is this line: UD60x18 requiredMarginInCollateralX18 = amountUsdX18.div(marginCollateralPriceUsdX18);
This is using round down , which may cause dust pnl to change isMissingMargin to false, causing break loop.

Example.
The user has two collaterals
marginCollateralBalance = [Collateral_A = 1e18 usd , Collateral_B= 2000e18 usd ]
settlementFee =100e18
orderFee=100e18
pnl = 9
marginCollateralPrice= 10e18

1. loop use Collateral_A
   • 1.1 charge settlementFee: withdrawMarginUsd(settlementFee=100e18)
     • marginCollateralBalanceX18 = 1e18
     • requiredMarginInCollateralX18 = 100e18 * 1e18 / 10e18 = 10e18
     • isMissingMargin = true , Remaining = 99e18 --------------------> correct
   • 1.2 charge orderFee :withdrawMarginUsd(orderFee=100e18)
     • marginCollateralBalanceX18 = 0
     • requiredMarginInCollateralX18 = 100e18* 1e18 / 10e18 = 10e18
     • so isMissingMargin = true , Remaining = 100e18 --------------------> correct
   • 1.3 charge pnl: withdrawMarginUsd(pnl=9)
     • marginCollateralBalanceX18 = 0
     • requiredMarginInCollateralX18 = 9* 1e18 / 10e18 = 0 ----------> ***** round down *****
     • so marginCollateralBalanceX18.gte(requiredMarginInCollateralX18) == true
     • so isMissingMargin = false --------------------------------------> **** wrong *****
2. will loop Collateral_B , but step 1.3 , isMissingMargin == false , so loop break

When charging a pnl, due to the use of round down
requiredMarginInCollateralX18 = amountUsdX18.div(marginCollateralPriceUsdX18) = 9 * 1e18 / 10e18 ==0

This caused the loop to jump out of the loop early by incorrectly overriding isMissingMargin=false.

miss fees: settlementFee = 99e18 + orderFee = 100e18

Impact

dust pnl can cause orderFee/settlementFee to not be fully collected

Tools Used

Recommendations

use round up

    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);
+       UD60x18 requiredMarginInCollateralX18 = amountUsdX18.divUp(marginCollateralPriceUsdX18);