Cyfrin / foundry-defi-stablecoin-cu

241 stars 107 forks source link

Liquidators S_totalMintedDsc not updating after liquidation #95

Closed Mohankotte123 closed 2 weeks ago

Mohankotte123 commented 1 month ago

In the liquidation process, I noticed that while the liquidator's totalMintedDsc was updated correctly during minting, it was not being decremented appropriately during liquidation. This led to discrepancies between the actual and expected balances of the liquidator. I’m currently seeking feedback from the community to confirm whether this is a valid bug or if there’s another underlying issue.

EngrPips commented 1 month ago

Hello @Mohankotte123, You should write some tests that prove this and give more details about the issue, such as what function in the codebase has an issue, what line in the function has an issue, and what your suggested recommendation is to fix the issue. Any other information that can help us understand properly the issue you are pointing out will be appreciated

cromewar commented 1 month ago

Yes, @Mohankotte123. Please provide more details about the issue so we can adequately address it.

Mohankotte123 commented 1 month ago

Hello @Mohankotte123, You should write some tests that prove this and give more details about the issue, such as what function in the codebase has an issue, what line in the function has an issue, and what your suggested recommendation is to fix the issue. Any other information that can help us understand properly the issue you are pointing out will be appreciated

@EngrPips , @cromewar Here is the test Below:-

    function testLiquidatorstotalDSCMintedNotUpdatingCorrecly() public {

    //Liquidation 
    //AMOUNT_COLLATERAL = 10 ether;
    //COLLATERAL_TO_COVER = 20ether;
    //AMOUNT_TO_MINT = 100 ether;
    //AMOUNT_TO_BURN = 100 ether

    vm.startPrank(USER);
    ERC20Mock(weth).approve(address(dsce), AMOUNT_COLLATERAL);
    dsce.depositCollateralAndMintDsc(weth, AMOUNT_COLLATERAL, AMOUNT_TO_MINT);
    vm.stopPrank();
    int256 ethUsdUpdatedPrice = 18e8;
    MockV3Aggregator(ethUsdPriceFeed).updateAnswer(ethUsdUpdatedPrice);
    vm.startPrank(LIQUIDATOR);
    ERC20Mock(weth).approve(address(dsce), COLLATERAL_TO_COVER);
    dsce.depositCollateralAndMintDsc(weth, COLLATERAL_TO_COVER, AMOUNT_TO_MINT);
    dsc.approve(address(dsce), AMOUNT_TO_BURN);
    dsce.liquidate(weth, USER, AMOUNT_TO_MINT);
    vm.stopPrank();

    // 1. Liquidtor deposit 20 ether and mints 100 ether
    // 2. when Liquidator, liquidates USER, the Liquidators minter 100 ether has to be payed as debt to protocol
    // 3. And then S_totalDscMinted[LIQUIDATOR] should be equal to 0;
    // But here its not updating correctly

    assertEq(dsce.getTotalDscMinted(LIQUIDATOR) ,  0 );
} 

The above test has to be passed actually, but since after liqudation the mintedDsc of Liquidator is not reducing from the S_totalMintedDsc mapping.

Output for test:-

Ran 1 test suite in 50.73ms (5.58ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/Unit/DSCEngineTest.t.sol:DSCEngoneTest
[FAIL. Reason: assertion failed: 100000000000000000000 != 0] testLiquidatorstotalDSCMintedNotUpdatingCorrecly() (gas: 543607) 

As of My knowledge, the above issue can be resolved by updating the Liquidator balance, in between redeemingCollateral and BurnDsc in liquidate function(DSCEngine.sol) as below:-

_redeemCollateral(user, msg.sender, collateral, totalCollateralToRedeem);
 _updateLiquidatorDsc(msg.sender, debtToCover); // Modification
_burnDSC(debtToCover, user, msg.sender);

 function _updateLiquidatorDsc(address liquidator, uint256 debtToCover) private {
        s_DSCMinted[liquidator] -= debtToCover;
    }
EngrPips commented 1 month ago

Thanks for the details, @Mohankotte123, This will be attended to appropriately as soon as possible.

PatrickAlphaC commented 2 weeks ago

Sorry on the delay here. The result you're seeing is actually correct. The liquidator will not have 0 DSC minted after liquidating.

vm.startPrank(LIQUIDATOR);
ERC20Mock(weth).approve(address(dsce), COLLATERAL_TO_COVER);
dsce.depositCollateralAndMintDsc(weth, COLLATERAL_TO_COVER, AMOUNT_TO_MINT);
dsc.approve(address(dsce), AMOUNT_TO_BURN);
dsce.liquidate(weth, USER, AMOUNT_TO_MINT);
vm.stopPrank();

In your code, the liquidator mints DSC themselves, and pays off the users debt, not their own debt.

It looks like this:

Step 1 USER: 10 DSC minted (bad health factor!) USER BALANCE: 10 DSC

LIQUIDATOR: 20 DSC MINTED LIQUIDATOR BALANCE: 20 DSC

Step 2 USER: 0 DSC minted (paid off by LIQUIDATOR) USER BALANCE: 10 DSC

LIQUIDATOR: 20 DSC MINTED (still! This does not go down!) LIQUIDATOR BALANCE: 10 DSC (20 - 10 = 10)