Defi Stablecoin course question: do we really need _revertIfHealthFactorIsBroken(msg.sender); in the liquidate function?

[thurendous]

I am having the course of Stablecoin course now. And Patrick use the code below as the liquidation function implementation.
I am quite skeptic about the last line of the code.

Do you think revertIfHealthFactorIsBroken(msg.sender); is necessary or not?

I think this one is not necessary because:

• The msg.sender's health factor has nothing to do with the current user's liquidation process.
  • The important thing is that they have enough DSC to cover the debt, not their own position's health factor is underwater or not.
• It limits who can perform the liquidation process, and it is not necessary.
• A user with a broken health factor can still have enough DSC to liquidate others.

By deleting this line, the protocol can become more robust.

How do you think??
Did Patrick make some change about this one? Also there was an audit I think.

    /*
     * @param collateral: The ERC20 token address of the collateral you're using to make the protocol solvent again.
     * This is collateral that you're going to take from the user who is insolvent.
     * In return, you have to burn your DSC to pay off their debt, but you don't pay off your own.
     * @param user: The user who is insolvent. They have to have a _healthFactor below MIN_HEALTH_FACTOR
     * @param debtToCover: The amount of DSC you want to burn to cover the user's debt.
     *
     * @notice: You can partially liquidate a user.
     * @notice: You will get a 10% LIQUIDATION_BONUS for taking the users funds.
    * @notice: This function working assumes that the protocol will be roughly 150% overcollateralized in order for this
    to work.
    * @notice: A known bug would be if the protocol was only 100% collateralized, we wouldn't be able to liquidate
    anyone.
     * For example, if the price of the collateral plummeted before anyone could be liquidated.
     */
    function liquidate(
        address collateral,
        address user,
        uint256 debtToCover
    )
        external
        isAllowedToken(collateral)
        moreThanZero(debtToCover)
        nonReentrant
    {
        uint256 startingUserHealthFactor = _healthFactor(user);
        if (startingUserHealthFactor >= MIN_HEALTH_FACTOR) {
            revert DSCEngine__HealthFactorOk();
        }
        // If covering 100 DSC, we need to $100 of collateral
        uint256 tokenAmountFromDebtCovered = getTokenAmountFromUsd(collateral, debtToCover);
        // And give them a 10% bonus
        // So we are giving the liquidator $110 of WETH for 100 DSC
        // We should implement a feature to liquidate in the event the protocol is insolvent
        // And sweep extra amounts into a treasury
        uint256 bonusCollateral = (tokenAmountFromDebtCovered * LIQUIDATION_BONUS) / LIQUIDATION_PRECISION;
        // Burn DSC equal to debtToCover
        // Figure out how much collateral to recover based on how much burnt
        _redeemCollateral(collateral, tokenAmountFromDebtCovered + bonusCollateral, user, msg.sender);
        _burnDsc(debtToCover, user, msg.sender);

        uint256 endingUserHealthFactor = _healthFactor(user);
        // This conditional should never hit, but just in case
        if (endingUserHealthFactor <= startingUserHealthFactor) {
            revert DSCEngine__HealthFactorNotImproved();
        }
        revertIfHealthFactorIsBroken(msg.sender);
    }

https://github.com/Cyfrin/foundry-defi-stablecoin-cu/blob/1415a29efaa9ca092b940432261f6961ba292ac1/src/DSCEngine.sol#L202C1-L249C6

[EngrPips]

Yes, there was an audit for the protocol, and yeah, you can argue that line of code is unnecessary, but it is a safety check that will keep the protocol invariant.

I am unsure what you mean when you said, A user with a broken health factor can still have enough DSC to liquidate others. Isn't a user with a broken healthFactor be more concerned about getting healthy than liquidating other users?

[thurendous]

And in this design, when I have a position in the protocol and I am underwater, I cannot liquidate myself too...

[EngrPips]

Thanks for your comment! I also understand this point of the education purpose of the course. But I thought that if it is production level feasible project, it will make more sense for the students to learn from the course. That is the reason why I wanted to ask this question. 😘

Filming a production codebase for course content isn't as easy as you think. But yeah I get your point.

[EngrPips]

And in this design, when I have a position in the protocol and I am underwater, I cannot liquidate myself too...

Hmmm

[thurendous]

I learned a lot from Patrick and really enjoyed his content since long time ago.
Because of that, I introduced some team to the Cyfrin auditing contest.
Thank you for talking with me!

[EngrPips]

Awesome, I am glad you like it with Cyfrin.