Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

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

0x1982us

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);
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

0x1982us Submitter
12 months ago
inallhonesty Lead Judge
11 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

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

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.