Precision Loss in the Health Factor Calculation.

[codehawks-bot]

Precision Loss in the Health Factor Calculation.

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/main/src/DSCEngine.sol#L330-L331

Summary - Vulnerability Details

Due to performing a multiplication on the result of a division, the _calculateHealthFactor loss precision while checking the collateralAdjustedForThreshold against the totalDscMinted.

(collateralAdjustedForThreshold * 1e18) / totalDscMinted;

Example:

Let's say the user collateral value in USD is $1001(in wei 1001000000000000000000) and the user tries to mint 500.5(in wei 500500000000000000000) DSC.

uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;

After inserting the values in this formula 1001000000000000000000 * 50 / 100 the result will be 500000000000000000000.

We can see it lost 500000000000000000, the actual result should be 500500000000000000000 (500.5 DSC).

Due to that, if the user inserts the totalDscMinted value 500500000000000000000(which should be valid), he will see the DSCEngine__BreaksHealthFactor error.

(collateralAdjustedForThreshold * 1e18) / totalDscMinted;

Because this will return 999000999000999000 which is < than 1e18.

Impact

Users face the DSCEngine__BreaksHealthFactor error when such a calculation occurs.

Tools Used

Slither, Math, Solodit

Recommendations

Make sure to do all the multiplications before division.

[alymurtazamemon]

@hans-cyfrin May you please mention the reason for not considering it so I can discuss this?

[alymurtazamemon]

@hans-cyfrin @PatrickAlphaC Found the similar issue as valid Precision loss when calculating the health factor

[alymurtazamemon]

@PatrickAlphaC @hans-cyfrin I consider it a Medium issue because;

Impact: will be (Disruption of protocol functionality or availability)
Likelihood: will be Highly likely to happen. (Many calculations will result in decimals where this issue can happen)

And the impact vs. likelihood matrix shows it as H/M so at least it should be a medium issue.

[PatrickAlphaC]

This is 100% a low issue:

IMPACT: Low - the collateral requirements a a little higher when using weird values due to a small rounding error
Likelihood: Medium - users will have to hit very specific low amounts for them to run into this

This is a low