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

For collaterals with less than 18 decimals, the formula used in `tradingAccount.withdrawMarginUsd` is wrong

Oblivionis

Summary

For collaterals with less than 18 decimals, the formula used in tradingAccount.withdrawMarginUsd() is wrong,

Vulnerability Details

function withdrawMarginUsd()is used for: 1. liquidation 2. charge orderFee, settlementFee and negative unsettled PnL.

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

For margins with less than 18 decimals(for example, WBTC has 8 decimals), There is a problem with the following logic:

amountToTransfer =
marginCollateralConfiguration.convertUd60x18ToTokenAmount(requiredMarginInCollateralX18);
​
IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
​
withdrawnMarginUsdX18 = amountUsdX18;

convertUd60x18ToTokenAmount(requiredMarginInCollateralX18)will truncate the last 10 digits of 18 decimals, so that the actual value received by the receiver is always less than withdrawnMarginUsdX18. The difference between the two value is locked into the contract forever.

It is worth noting that withdrawMarginUsd() is executed multiple times in a single order. For users with multiple margins, this rounding issue can happen dozens of times.

If we assume withdrawMarginUsd() is executed ten times in an order and WBTC's price = 70000usd, that means 0.007usd worth of WBTC would be locked in the contract. As orders accumulate, users suffer more and more losses.

Impact

Likelihood: high - Every transaction for every user with a margin that is not 18 decimals will experience this.

+

Impact: low - Losses from a single order are small, but can accumulate.

=

Severity: medium

Tools Used

Manual review

Recommendations

Recalculate withdrawnMarginUsdX18 based on amountToTransfer

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

convertUd60x18ToTokenAmount heavily truncates

Support

FAQs

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