The function _revertIfHealthFactorIsBroken
calculates the _healthFactor
of an user and then compares it with the MIN_HEALTH_FACTOR
, reverting if it less than it, which will be 100% of the time when the assets used have lower decimals places.
Let's take the example of WBTC which is one of the assets used in the protocol. WBTC has 8 decimals so if an user deposits WBTC as the collateral, getAccountCollateralValue
which calls getUsdValue
to calculate USD value of the collateral, will return a 1e8 number, as can be seen per the calculation https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L366
This value will then be passed into _calculateHealthFactor
which will calculate the health factor, but in the case of WBTC the calculation would return a 1e8 number, since DSC inherits from ERC20Burnable and has 18 decimals https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L331
The health factor would then be passed into the if statement, which would always be true since you will compare a 1e8 number with MIN_HEALTH_FACTOR
which is 1e18 and revert. Consider also that this can be used also in the liquidation process to liquidate someone even though they are not undercollateralized.
The impact is a people can be easily liquidated in those cases, even if they are well collateralized
Manual review
Try to do the calculations bringing every asset to the same decimal place, so accounting errors like this would not occur.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.