division before multiplication lead to precision loss

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L347

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L324C1-L332C6

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L361-L367

Summary

division before multiplication lead to precision loss

Vulnerability Details

Solidity uses fixed-point arithmetic so Division Before Multiplication can result in precision loss errors due to rounding down. in the functions that calculate the healthFactor

324: function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
        internal
        pure
        returns (uint256)
    {
        if (totalDscMinted == 0) return type(uint256).max;
331    -> uint256 collateralAdjustedForThreshold = (collateralValueInUsd * LIQUIDATION_THRESHOLD) / LIQUIDATION_PRECISION;
        return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;
    }

and the function that calculate the collateralValueInUsd is :

    function getUsdValue(address token, uint256 amount) public view returns (uint256) {
        AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
        (, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
        // 1 ETH = $1000
        // The returned value from CL will be 1000 * 1e8
 366 -> return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
    }

so the general (total) calculation of the actual healthFactor after expanding the function calls & variables in an equation to expose hidden division before multiplication is :

HealthFactor = (price * ADDITIONAL_FEED_PRECISION * amount / PRECISION) * LIQUIDATION_THRESHOLD / LIQUIDATION_PRECISION * PRECISION/totalDSCMinted ;

which contain division before multiplications twice , then this will cause a precision loss that will affect the process of liquidation which will result in loss of funds of the user

Impact

the precision loss will result in inaccurate health factor which will cause the liquidation of the users and loss their collateral , and this may result in users that exceed the liquidationThreshold without liquidation .

Tools Used

manual review

Recommendations

perform the multiplication before the division to avoid the precision loss . the recommended equation of the health factor :

uint healthFactor =  price * ADDITIONAL_FEED_PRECISION * amount * LIQUIDATION_THRESHOLD * PRECISION / PRECISION/ LIQUIDATION_PRECISION/totalDSCMinted ;

Cyfrin / 2023-07-foundry-defi-stablecoin