Cyfrin / foundry-defi-stablecoin-cu

241 stars 107 forks source link

A huge issue in liquidation functionality!!!!!! #55

Closed inishantjain closed 8 months ago

inishantjain commented 8 months ago

I was in the middle of the 3rd video and came across an issue in the liquidation function of the Engine. I want to confirm if my understanding is correct or if there's a misunderstanding on my part.

In the Engine, there's a liquidate function designed to liquidate the DSC of a user with a health factor below 1. The function _burnDsc is invoked for this purpose:

    function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
        s_DSCMinted[onBehalfOf] -= amountDscToBurn;

        bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
        // This conditional is hypothetically unreachable
        if (!success) {
            revert DSCEngine__TransferFailed();
        }
        i_dsc.burn(amountDscToBurn);
    }

called like this on L217 in liquidation function

_burnDsc(debtToCover, user, msg.sender);

Now,

  1. we are subtracting the DSC balance from the user's (who's going to get liquidated) account. However, the user's actual DSC balance never gets burned. If we write a test to check the actual balance vs the stored balance, it fails:
    function testUserActualBalanceVsStoredBalanceAfterLiquidation() public liquidated {
        uint256 actualDscBalance = dsc.balanceOf(user);
        (uint256 storedDscBalance,) = dsce.getAccountInformation(user);
        assert(actualDscBalance == storedDscBalance);
    }

image

  1. Moreover, in the _burnDsc function, we transfer all the DSC of the liquidator to the engine's address and then burn it. However, we've never subtracted that burnt balance from the liquidator's account. This means the liquidator still has the DSC according to DSC Engine, but if we try to transfer that to another account, it fails because the actual balance is low. Performing the same test for the liquidator also fails.

I've attempted to refactor the code, but I realized that if we want to transfer the balance from the user's account to burn for liquidation, it will require an allowance to do this.

PatrickAlphaC commented 8 months ago

This is actually correct.

The liquidator is paying off the liquidated's debt to the protocol. So the one getting liquidated's DSC isn't lowered, however, the liquidator's DSC is burned instead.

Let me know if that doesn't make sense.

inishantjain commented 8 months ago

ok that makes some sense!!! but i'm still confused If the liquidator's DSC is burned, we need to adjust the liquidator's s_DSCMinted by the amount of DSC that was burned.

PatrickAlphaC commented 8 months ago

Not quite!

Actually, you can think of the s_DSCMinted as the "debt" to the protocol. When user A mints DSC, their s_DSCMinted goes up. This way, the protocol keeps track of how much user A needs to pay back.

But when they get liquidated, user B pays user A's debt, so we subtract user A's debt (aka, user A's s_DSCMinted) since user B is paying off user A's debt.

inishantjain commented 8 months ago

ohhkhh it is more clear to me now, Thanks ^_^

Mohankotte123 commented 1 month ago

Not quite!

Actually, you can think of the s_DSCMinted as the "debt" to the protocol. When user A mints DSC, their s_DSCMinted goes up. This way, the protocol keeps track of how much user A needs to pay back.

But when they get liquidated, user B pays user A's debt, so we subtract user A's debt (aka, user A's s_DSCMinted) since user B is paying off user A's debt.

@PatrickAlphaC But during liquidation, When Liquidator Mints some Dsc, S_DSCMinted is goes up , which is also a debt to the protocol Right!!

PatrickAlphaC commented 1 month ago

Correct, so they'd have to pay their own debt off later themselves. This is a bit of a flaw in this project, and it's why this is just demo code and not meant for production.

Mohankotte123 commented 1 month ago

Correct, so they'd have to pay their own debt off later themselves. This is a bit of a flaw in this project, and it's why this is just demo code and not meant for production.

Can we fix this, through updating the liquidator balance after redeeming Collateral and burn Dsc, in liquidate function of DSCEngine.sol?

EngrPips commented 1 month ago

Is the Liquidator sending DSC to liquidate a user, or are they sending collateral to liquidate the user? I have misunderstood the protocol all the while. I thought the liquidation process works as explained below.

LIQUIDATION SCENARIO user A deposited 1 ETH worth $100 and minted DSC worth $20, but after a while, 1 ETH is now worth $35, meaning that their collateral is not worth double their debt anymore, so we need to liquidate them. A liquidator comes along and sends the protocol ETH amount worth $20 to clear off user A debt. We transfer DSC from user A, burn it, and release ETH amount worth $20 plus the liquidation bonus to the liquidator. Does the Liquidator need to mint DSC to liquidate user A?