Summary

A user that has been liquidated can still use his DSC tokens to interact with the contract through the liquidate function as it doesn't check the health factor of the liquidator.

Vulnerability Details

After a user has been liquidated his tokens are not burned and instead the liquidator's funds are burned which means that the user still has the same amount of tokens even after the s_DSCMinted mapping has been changed.

Lets say Tom deposits as collateral 10 eth and the price of eth is 2000 worth of DSCs(1 DSC = 1 per eth which means that Tom has now 180$ worth of collateral and 100$ of DSCs - he is undercollateralized. Alice comes as a liquidator. She deposits 200$ worth of eth and mints 100$ of DSCs. She then liquidates Tom by calling the liquidate function Alice sends her 100 DSCs to be burned and the storage updates that the amount of DSCs of Tom = 0 which pays his debt, but in reality Tom has 100 DSCs but he cannot redeem them since the storage arrays says he doesn't have any DSCs. But what he can do is liquidate another user by not having the collateral to back his DSCs. Now lets say he liquidates another user that was in the same situation as him(price of eth falls from 2000$ to 18$) he has the tokens which means he doesn't have to deposit collateral. Tom now pays the debt of the other user although he shouldn't have the resources and should have been liquidated. So what happens is Tom gets free money from liquidating another user. This is not the natural flow of the liquidation process because the user can continue to operate with his DSCs even after being liquidated. This issue can become more serious if the stablecoin gets included in a DEX or in other protocols where you can do stuff with it.

Proof Of Concept

Add the following code in the DSCEngineTest.t.sol test file to see the issue:

function testFundsStuck() public {
    vm.startPrank(user);
    ERC20Mock(weth).approve(address(dsce), amountCollateral);
    dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
    vm.stopPrank();

    address user2 = address(2);

    ERC20Mock(weth).mint(user2, STARTING_USER_BALANCE);

    vm.startPrank(user2);
    ERC20Mock(weth).approve(address(dsce), amountCollateral);
    dsce.depositCollateralAndMintDsc(weth, amountCollateral, amountToMint);
    vm.stopPrank();

    //Act
    int256 ethUsdUpdatedPrice = 18e8; // 1 ETH = $18
    MockV3Aggregator(ethUsdPriceFeed).updateAnswer(ethUsdUpdatedPrice);

    ERC20Mock(weth).mint(liquidator, collateralToCover);

    vm.startPrank(liquidator);
    ERC20Mock(weth).approve(address(dsce), collateralToCover);
    dsce.depositCollateralAndMintDsc(weth, collateralToCover, amountToMint);
    dsc.approve(address(dsce), amountToMint);
    dsce.liquidate(weth, user, amountToMint);
    vm.stopPrank();

    vm.startPrank(user);
    dsc.approve(address(dsce), amountToMint);
    dsce.liquidate(weth, user2, amountToMint);
    dsce.redeemCollateral(weth, dsce.getCollateralBalanceOfUser(user, weth));
    vm.stopPrank();

    console.log("User funds: ", dsc.balanceOf(user));
    console.log("Liquidator funds: ", ERC20Mock(weth).balanceOf(liquidator));
    console.log("User collateral value: ", dsce.getCollateralBalanceOfUser(user, weth));
    console.log("User funds: ", ERC20Mock(weth).balanceOf(user));
}

This test shows how a user can take advantage of the liquidate function and get his funds back even after being liquidated. So there is no punishment for a user that is undercollateralized and that breaks the logic of the contract.

Impact

Users are left unpunished for being undercollateralized and can continue to operate with their DSCs even after being liquidated.

Tools Used

Visual Studio Code, Foundry

Recommendations

The easiest way to fix this is by adding a check in the _burnDsc function to see if the user actually has the balance he wants to burn like this:

s_DSCMinted[onBehalfOf] -= amountDscToBurn;

if(s_DSCMinted[dscFrom] < amountDscToBurn) {
    revert DSCEngine__NotEnoughDSCBalance(); //This is a custom error added for this check
}
bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);