High vulnerability: Broken internal health factor after `liquidate`

[nzlatev7]

Broken internal health factor after liquidate

Relevant GitHub Links

https://github.com/Cyfrin/foundry-defi-stablecoin-f23/blob/76ba1ceb93214ece058f6b8b31580745a8340f4b/src/DSCEngine.sol#L200-L227

Summary

Base on the proof of concept provided below, you can see that after the liquidator perform liquidation calling the liquidate function his internal health factor is broken (the health factor may be good, but is not the health factor that need to be). The key is that the liquidate function does not update the s_DSCMinted mapping for the liquidator. It updates the s_DSCMinted mapping for the user with the bad health factor and sends totalCollateralToRedeem to the liquidator but nothing is performed for the dept deposited by the liquidator. At the end, base on the system ( s_DSCMinted mapping) liquidator has amount of tokens that is difference from his actual dsc token balance (dsc.balanceOf(bob)). In this way the liquidator will not be able to get back his full amount of collateral because he can not provide the amount listed in the mapping.

Vulnerability Details

function testBreaksInternalHealthFactor() public {
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");

        ERC20Mock(weth).mint(alice, STARTING_ERC20_BALANCE);
        ERC20Mock(weth).mint(bob, STARTING_ERC20_BALANCE);

        MockV3Aggregator(ethUsdPriceFeed).updateAnswer(int256(2_000 * 10 ** 8)); // $2,000
        MockV3Aggregator(wbtcUsdPriceFeed).updateAnswer(int256(30_000 * 10 ** 8)); // $30,000

        // alice call depositCollateralAndMintDsc
        // collateral: 10 weth tokens -> 10 * $2000 -> $20,000
        // dsc tokens: 10000 dsc tokens (the max that alice can mint for this collateral)

        vm.startPrank(alice);
        ERC20Mock(weth).approve(address(dscEngine), 10e18);
        dscEngine.depositCollateralAndMintDsc(weth, 10e18, 10000e18);
        vm.stopPrank();

        (uint256 totalDscMinted, uint256 collateralValueInUsd) = dscEngine.getAccountInformation(alice);
        assert(totalDscMinted == 10000e18); // in $
        assert(collateralValueInUsd == 20000e18); // in $

        // bob call depositCollateralAndMintDsc
        // collateral: 20 tokens -> 20 * $2000 -> $40,000
        // dsc tokens: 10000 dsc tokens

        vm.startPrank(bob);
        ERC20Mock(weth).approve(address(dscEngine), 20e18);
        dscEngine.depositCollateralAndMintDsc(weth, 20e18, 10000e18);
        vm.stopPrank();

        (uint256 totalDscMinted2, uint256 collateralValueInUsd2) = dscEngine.getAccountInformation(bob);
        assert(totalDscMinted2 == 10000e18);
        assert(collateralValueInUsd2 == 40000e18);

        // the price drops
        MockV3Aggregator(ethUsdPriceFeed).updateAnswer(1800e8); // $1,800

        // alice have bad health factor now
        //(uint256 totalDscMinted, uint256 collateralValueInUsd) = dscEngine.getAccountInformation(alice);
        //assert(totalDscMinted == 10000);
        //assert(collateralValueInUsd == 18000);

        // bob tries to improve alice's health factor
        vm.startPrank(bob);
        dsc.approve(address(dscEngine), 1000e18); // 1000e18 - $1000 (1 dsc token = $1)
        dscEngine.liquidate(weth, alice, 1000e18); // 1000e18 - $1000 (1 dsc token = $1)
        vm.stopPrank();

        // alice heath factor is improved
        // uint256 healthFactor = dscEngine.getHealthFactor(alice);
        // console.log("alice healthFactor", healthFactor); // 1000000000000000000 -> 1e18

        // after the liquidation
        // collateral: 20 tokens -> 20 * $1800 -> $36,000
        // dsc tokens: 10000 dsc tokens tracked by the system, 9000 actual amount

        (uint256 totalDscMinted3, uint256 collateralValueInUsd3) = dscEngine.getAccountInformation(bob);

        assert(collateralValueInUsd3 == 36000e18);
        assert(totalDscMinted3 == 10000e18);
        assert(dsc.balanceOf(bob) == 9000e18);

        // if bob wants to redeem some collateral
        // base on the system (s_DSCMinted mapping) -> bob will be able to redeem only 8 amount of collateral
        // base on the actual amount of dsc -> bob will be able to redeem 10 amount of collateral

        // to see the difference you can change the amount of collateral to redeem

        vm.startPrank(bob);
        uint256 amountCollateralToRedeem = 10e18;
        vm.expectRevert();
        dscEngine.redeemCollateral(weth, amountCollateralToRedeem);
    }

Impact

Liquidators will no be able to redeem their full collateral potencial base on the health factor, because the s_DSCMinted mapping have stale info. Additionally, liquidators would not call liquidate. The protocol would suffer insolvency in adverse market conditions due to no liquidations taking place

Tools Used

Manual Review

Recommendations

To update the s_DSCMinted mapping in the liquidate function for the liquidator, but this will lead to other vulnerabilities.

[Blockchain-Oracle]

Yh a good one @nzlatev7 Actually found this bug while doing my unit test

Only to find out we ain't updating the liquidator Mapping of minted dsc

[nzlatev7]

Yh a good one @nzlatev7 Actually found this bug while doing my unit test

Only to find out we ain't updating the liquidator Mapping of minted dsc

I was wondering why the liquidator health factor stay the same. And are we deliberately not updating the mapping. But if we purposely do not update liquidator's health factor the impact is this.

[PatrickAlphaC]

This is actually the correct functionality. When we liquidate a user, we are essentially paying off the users debt in return for a better rate on the liquidated users collateral.

It wouldn't make sense to liquidate a user and then STILL have the user owe money to the protocol.