search

Cyfrin / foundry-defi-stablecoin-cu

250 stars 117 forks source link

Update DSCEngine.sol #8

Closed aaankeet closed 1 year ago

aaankeet commented 1 year ago

fixed a bug in _calculateHealthFactor in DSCEngine.sol

PatrickAlphaC commented 1 year ago

Thanks for the PR!

Remember to always run the test suite after making a change on a codebase. At the moment, this change breaks 17 tests! So we couldn't merge it even if it was correct.

Additionally, in solidity, we always want to multiple before we divide.

And finally, why would we add this here?

    function _calculateHealthFactor(uint256 totalDscMinted, uint256 collateralValueInUsd)
        internal
        pure
        returns (uint256)
    {
        if (totalDscMinted == 0) return type(uint256).max;
        uint256 collateralAdjustedForThreshold = ((collateralValueInUsd / 1e18) * LIQUIDATION_THRESHOLD) / 100;
        return (collateralAdjustedForThreshold * 1e18) / totalDscMinted;
    }

If collateralValueInUsd was 1e18, we'd get:

uint256 collateralAdjustedForThreshold = ((collateralValueInUsd / 1e18) * LIQUIDATION_THRESHOLD) / 100;
uint256 collateralAdjustedForThreshold = (1e18/1e18) * 50 / 100;
uint256 collateralAdjustedForThreshold = 0

Which isn't correct!

The correct value is:

uint256 collateralAdjustedForThreshold = ((collateralValueInUsd ) * LIQUIDATION_THRESHOLD) / 100;
uint256 collateralAdjustedForThreshold = (1e18) * 50 / 100;
uint256 collateralAdjustedForThreshold = 5e17