Cyfrin / foundry-full-course-cu

GNU General Public License v3.0
3.7k stars 916 forks source link

Defi protocol - Lesson 14: Setup liquidations #1657

Closed whxint closed 6 months ago

whxint commented 7 months ago

Lesson

Lesson 14

Could you please leave a link to the timestamp in the video where this error occurs? (You can right click a video and "copy video URL at current time")

https://youtu.be/8dRAd-Bzc_E?t=10229

Operating System

macOS (Apple Silicon)

Describe the bug

Liquidator pays someone's debt and burns its own DSC, but its state in the engine contract does not change.

When the burn() function runs, the liquidated user's DSC amount decreases, but the liquidater's tokens are transferred to the contract and burned. Considering that the liquidater has a DSC of $100 dollars, it does not have any DSCs since they are all burned, but it continues to write $100 dollars in the s_DSCMinted variable of the contract. When liquidater comes later and mints $100, he will write $200 in the s_DSCMinted variable, but he will have $100 dollars. It always has extra DSC.

function liquidate(
        address collateral,
        address user,
        uint256 debtToCover
    )
        external
        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);
    }

If the liquidator liquidates a user, the DSC that will be used is not what he pressed into the engine, but a DSC sent to him by someone else or perhaps purchased from the exchange. Therefore, when liquidating a user, their DSC balance will not decrease. But when the Liquidator performs DepositColleteral and liquidates someone, he always has an extra amount of DSC. If he wants to withdraw his collectral after liquidating someone, the application will give an error because he does not have such a DSC amount, but the DSC engine shows that he has it. Why is the liquidator's DSC balance not taken into account in DSCEngine after a successful liquidation call?

Chinwuba22 commented 7 months ago

Good day @whxint,

The line _burnDsc(debtToCover, user, msg.sender); from what you showed above decreases the balance of the person to be liquidated in favour of the person doing the liquidation. You can go through the _burnDsc function for clarity.

whxint commented 7 months ago
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);
    }

When the burn() function runs, the liquidated user's DSC amount decreases, but the liquidater's tokens are transferred to the contract and burned. Considering that the liquidater has a DSC of $100 dollars, it does not have any DSCs since they are all burned, but it continues to write $100 dollars in the s_DSCMinted variable of the contract.

Someone is making depositcollateral, the system will run into a bug. It might have been overlooked because it wasn't a real project, or it might have worked that way. $100 dollars in the s_DSCMinted variable of the contract. When liquidater comes later and mints $100, he will write $200 in the s_DSCMinted variable, but he will have $100 dollars. It always has extra DSC.

Chinwuba22 commented 7 months ago

Sorry @whxint, Yes it isnt a real project amd was intented to help more with how to write code.

I think i get your question however and it is not a bug; the s_DSCMinted is also updated in the _burnDsc function here: s_DSCMinted[onBehalfOf] -= amountDscToBurn; which would mean that that the person to be liquidated balance is updated correctly.

cromewar commented 6 months ago

Thanks for the answer @Chinwuba22

whxint commented 6 months ago

No. liquidatior balance never decreases. The state in the DSC engine never changes.

Chinwuba22 commented 6 months ago

https://github.com/Cyfrin/foundry-defi-stablecoin-f23/blob/main/test/unit/DSCEngineTest.t.sol#L453C5-L460C6

whxint commented 6 months ago

Bro, I am responding based on your answer. You go and test the increase in the amount of weth collateral. Is this a joke ?

s_DSCMinted[onBehalfOf] -= amountDscToBurn;

This does not reduce the DSC balance of the liquidatior and then DSC is transferred to the contract and burned, but the contract state does not change. Understand the code well pls

Chinwuba22 commented 6 months ago

Send your own test that is failing here.