Summary

During most operations on the DSCEngine.sol there is a check for the current healthFactor of the position of a user. The function _revertIfHealthFactorIsBroken makes sure to check that the expected 200% overcollateralization is satisfied when minting DSC, redeeming collateral etc. The healthFactor is also checked during liquidation to make sure that the position the liquidator is trying to liquidate is undercollateralized i.e. the healthFactor of the position is below the MIN_HEALTH_FACTOR which is hardcoded to 1e18.

The healthFactor check has the following flow:

1/ It gets the account information for a given user which is composed by totalDscMinted and collateralValueInUsd
1.1/ totalDscMinted is stored as a value in a mapping for each user which is updated during mint/burn operations on the token. DSC token is an ERC20 token with 18 decimals
1.2/ collateralValueInUsd is calculated by looping through each collateral the user has deposited, taking the amount which is stored in a mapping for each user and multiplying that amount by the price obtained from Chainlink Aggregators.
2/ Once both collateralValueInUsd and totalDscMinted are obtained the final healthFactor is calculated as follows: collateralValueInUsd * 1e18 / totalDscMinted

The expected result when the position healthFactor is at the exact minimum i.e 200% overcollateralization is that the healthFactor would equal 1e18. Anything below that means the position is below the 200% expected overcollateralization and can be considered undercollateralized.

Vulnerability Details

The above explained calculations work as expected, however it is assumed that any collateral token supported has 18 decimals, which even at a core level with the expected initial collateral tokens i.e. WETH and WBTC is a wrong assumption, as WBTC has 8 decimals. This wrongful assumption leads to any position which is composed solely of WBTC or is composed by at least some WBTC would produce invalid healthFactor due to lack of proper scaling. This means that potentially any position could be liquidated even tho it actually satisfies the >=MIN_HEALTH_FACTOR criteria, not to mention that users will need to provide way more collateral in order to mint any DSC tokens as their mint operations would fail otherwise.

A simple check via Remix shows the unexpected behavior:

In this simple test we are assuming the following:
a/ WBTC price is 30000 USD or 30000e8 as would be returned by Chainlink
b/ _amount passed to the function is the _amount of WBTC the user has as deposit - in our example we assume it to be 0.0667 WBTC or 6670000 (8 decimals token). This is equal to 2001 USD of collateral.
c/ _dscMinted is passed as 1000 DSC tokens or 1000000000000000000000, which is expected to be equal to 1000$

In this scenario the overcollateralization is a bit above 200% as we have 1000$ of DSC token and 2001$ of WBTC collateral. The expected healthFactor is 1.0005 which is greater than 1 ether MIN_HEALTH_FACTOR. However the produced value is 100050000 which is way lower than 1e18 as MIN_HEALTH_FACTOR.

Impact

Any position that is overcollateralized with only WBTC or a combination of collaterals where one of the collateral is not an 18 decimal token would produce a wrong healthFactor for that position which has the following implications:

a/ Minting of DSC would be blocked since there is a check if the userHealthFactor is above the MIN_HEALTH_FACTOR during the mint operation
b/ Positions could be liquidatable even tho they satisfy the healthFactor criteria

Tools Used

Manual Review

Recommendations

Make sure to properly scale the collateral amount in order to ensure that the protocol is working with values that are all scaled to 18 decimals.