Summary

A user, once liquidated, can liquidate another undercollateralized user for the amount this user had before got liquidated.

Vulnerability Details

A user, once liquidated, still shows as having DSC under contract DecentralizedStableCoin.sol by invoking function balanceOf(). The debt to cover through liquidation is deducted from the mapping s_DSCMinted but not from the DSC ERC20 balance. So, in this scenario a user who gets liquidated can liquidate others for the total amount he had before getting liquidated.

When the function liquidate() is invoked, one of the steps calls _burnDsc(debtToCover, user, msg.sender).

function _burnDsc(uint256 amountDscToBurn, address onBehalfOf, address dscFrom) private {
    s_DSCMinted[onBehalfOf] -= amountDscToBurn;
    bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn); //@audit amount of DSC of dscFrom not updated
    // This conditional is hypothtically unreachable
    if (!success) {
        revert DSCEngine__TransferFailed();
    }
    i_dsc.burn(amountDscToBurn);
}

As can be seen, s_DSCMinted of the user being liquidated gets updated by deducting the debt to cover. Afterwards, function transferFrom() is invoked transferring the same amount of DSC from the liquidator. As explained above, a liquidated user still has balance of DSC after being completely liquidated so that transferFrom() will go through letting him liquidate this position.

Impact

Let's consider the following scenario:
We have three users at a quotation price of ETH/USD of $2,000

• user_1 deposits 10 WETH and mints 10,000 DSC

• user_2 deposits 20 WETH and mints 18,000 DSC

• user_3 deposits 50 WETH and mints 20,000 DSC

Let's assume now that the quotation price of ETH/USD falls to $1,800 so leaving user_1 undercollateralized and prone to be liquidated.
user_3 realizes user_1 is undercollateralized and completely liquidate him by covering the 10,000 DSC.
After having been liquidated, user_1 has zero DSC so the balances will stay as follows:

• user_1 has 3.888888 WETH and 0 DSC

• user_2 has 20 WETH and 18,000 DSC

• user_3 has 56.111111 WETH and 20,000 DSC

Assume now that the quotation price of ETH/USD falls to $1,700 so leaving user_2 undercollateralized. User_1 realizes of this and even though does not own any DSC, user_1 liquidates 10,000 DSC from user_2. After that both user_1 and user_2 redeem all DSC and collateral and the balances stay as follows:

• user_1 has 10.359477124183006536 WETH

• user_2 has 13.529411764705882354 WETH

user_2 could now, in turn, liquidate another user for the same 10,000 DSC.

Tools Used

Foundry

// SPDX-License-Identifier: MIT

pragma solidity 0.8.19;

import "forge-std/Test.sol";
import "../../src/DSCEngine.sol";
import "../../src/DecentralizedStableCoin.sol";
import {MockV3Aggregator} from "../mocks/MockV3Aggregator.sol";
import {AggregatorV3Interface} from "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol";
import {OracleLib} from "../../src/libraries/OracleLib.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";

contract TestBalanceOfDSC is Test {
using OracleLib for AggregatorV3Interface;

uint8 public constant DECIMALS = 8;
int256 public constant ETH_USD_INITIAL_PRICE = 2000e8;
int256 public constant ETH_USD_EVOLVED_PRICE = 1800e8;
int256 public constant BTC_USD_INITIAL_PRICE = 1000e8;
int256 public constant BTC_USD_EVOLVED_PRICE = 999e8;

uint256 public constant STARTING_USER_BALANCE = 10 ether;
uint256 public constant MIN_HEALTH_FACTOR = 1e18;
uint256 public constant LIQUIDATION_THRESHOLD = 50;

DSCEngine public dsce;
DecentralizedStableCoin public dsc;

address public weth;
address public wbtc;

address public owner = makeAddr("owner");
address public user_1 = makeAddr("user_1");
address public user_2 = makeAddr("user_2");
address public user_3 = makeAddr("user_3");

MockV3Aggregator public ethUsdPriceFeed;
MockV3Aggregator public btcUsdPriceFeed;
ERC20Mock public wethMock;
ERC20Mock public wbtcMock;

address[] public tokenAddresses;
address[] public priceFeedAddresses;

function setUp() public {
    vm.startPrank(owner);
    dsc = new DecentralizedStableCoin();
    ethUsdPriceFeed = new MockV3Aggregator(DECIMALS, ETH_USD_INITIAL_PRICE);
    wethMock = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
    btcUsdPriceFeed = new MockV3Aggregator(DECIMALS, BTC_USD_INITIAL_PRICE);
    wbtcMock = new ERC20Mock("WBTC", "WBTC", msg.sender, 1000e8);

    weth = address(wethMock);
    wbtc = address(wbtcMock);

    tokenAddresses = [weth, wbtc];
    priceFeedAddresses = [address(ethUsdPriceFeed), address(btcUsdPriceFeed)];

    dsce = new DSCEngine(tokenAddresses, priceFeedAddresses, address(dsc));
    dsc.transferOwnership(address(dsce));

    wethMock.mint(user_1, STARTING_USER_BALANCE);
    wethMock.mint(user_2, STARTING_USER_BALANCE * 2);
    wethMock.mint(user_3, STARTING_USER_BALANCE * 5);
    vm.stopPrank();

    assertEq(wethMock.balanceOf(user_1), 10 ether);
    assertEq(wethMock.balanceOf(user_2), 20 ether);
    assertEq(wethMock.balanceOf(user_3), 50 ether);
}

function testCanLiquidateWithNoDsc() public {
    // User's balances of WETH before depositing
    uint256 user_1BalanceBeforeDeposit = wethMock.balanceOf(user_1);
    uint256 user_2BalanceBeforeDeposit = wethMock.balanceOf(user_2);

    // user_1 deposits 10 weth and mints the maximum amount of DSC => 10,000 DSC
    vm.startPrank(user_1);
    wethMock.approve(address(dsce), 10 ether);
    dsce.depositCollateralAndMintDsc(address(weth), 10 ether, 10_000 ether);
    vm.stopPrank();

    // user_2 deposits 20 weth and mints 18,000 DSC
    vm.startPrank(user_2);
    wethMock.approve(address(dsce), 20 ether);
    dsce.depositCollateralAndMintDsc(address(weth), 20 ether, 18_000 ether);
    vm.stopPrank();

    // user_3 deposits 50 weth and mints 20,000 DSC
    vm.startPrank(user_3);
    wethMock.approve(address(dsce), 50 ether);
    dsce.depositCollateralAndMintDsc(address(weth), 50 ether, 20_000 ether);
    vm.stopPrank();

    // Health Factor is calculated before the price of weth is updated
    // TotalDscMinted is calculated before the position is liquidated
    uint256 user_1HealthFactorBeforePriceUpdate = dsce.getHealthFactor(user_1);
    (uint256 user_1BeforeLiquidationTotalDscMinted,) = dsce.getAccountInformation(user_1);
    uint256 user_2HealthFactorBeforePriceUpdate = dsce.getHealthFactor(user_2);
    (uint256 user_2BeforeLiquidationTotalDscMinted,) = dsce.getAccountInformation(user_2);
    uint256 user_3HealthFactorBeforePriceUpdate = dsce.getHealthFactor(user_3);
    (uint256 user_3BeforeLiquidationTotalDscMinted,) = dsce.getAccountInformation(user_3);

    assertGe(user_1HealthFactorBeforePriceUpdate, 1 ether);
    assertGe(user_2HealthFactorBeforePriceUpdate, 1 ether);
    assertGe(user_3HealthFactorBeforePriceUpdate, 1 ether);
    assertEq(user_1BeforeLiquidationTotalDscMinted, 10_000 ether);
    assertEq(user_2BeforeLiquidationTotalDscMinted, 18_000 ether);
    assertEq(user_3BeforeLiquidationTotalDscMinted, 20_000 ether);
    assertEq(dsc.balanceOf(user_1), user_1BeforeLiquidationTotalDscMinted);
    assertEq(dsc.balanceOf(user_2), user_2BeforeLiquidationTotalDscMinted);
    assertEq(dsc.balanceOf(user_3), user_3BeforeLiquidationTotalDscMinted);

    // the price of weth is updated to $1,800 making user_1 go undercollateralized
    ethUsdPriceFeed.updateRoundData(2, ETH_USD_EVOLVED_PRICE, block.timestamp, 2);
    uint256 wethPriceUpdate1 = dsce.getUsdValue(weth, 1 ether);
    assertEq(wethPriceUpdate1, 1_800e18);

    // Health Factor is assessed after the price of weth is updated to make sure user_1 is undercollateralized
    uint256 user_1HealthFactorAfterPriceUpdate = dsce.getHealthFactor(user_1);
    assertLt(user_1HealthFactorAfterPriceUpdate, 1 ether);

    // user_3 gets user_1 fully liquidated
    vm.startPrank(user_3);
    dsc.approve(address(dsce), 10_000 ether);
    dsce.liquidate(weth, user_1, 10_000 ether);
    vm.stopPrank();

    // it can be seen that the amount of DSC owned by user_1 is zero according to getAccountInformation (i.e. s_DSCMinted[user])
    (uint256 totalDscMinted,) = dsce.getAccountInformation(user_1);
    assertEq(totalDscMinted, 0);

    // by using the function balanceOf, it can be seen that user_1 still has the 10,000 DSC
    uint256 userBalanceOfDsc = dsc.balanceOf(user_1);
    assertEq(userBalanceOfDsc, 10_000 ether);

    // the price of weth is updated to $1,700 making user_2 go undercollateralized
    ethUsdPriceFeed.updateRoundData(3, 1_700e8, block.timestamp, 3);
    uint256 wethPriceUpdate2 = dsce.getUsdValue(weth, 1 ether);
    assertEq(wethPriceUpdate2, 1_700e18);

    // user_1 could liquidate user_2 even though uer_1 does not have any DSC
    vm.startPrank(user_1);
    dsc.approve(address(dsce), 10_000 ether);
    dsce.liquidate(weth, user_2, 10_000 ether);
    vm.stopPrank();

    // user_1 and user_2 redeem all DSC and collateral
    vm.startPrank(user_1);
    dsce.redeemCollateral(weth, dsce.getCollateralBalanceOfUser(user_1, weth));
    vm.stopPrank();
    vm.startPrank(user_2);
    (uint256 dscAfterLiquidation,) = dsce.getAccountInformation(user_2);
    dsc.approve(address(dsce), dscAfterLiquidation);
    dsce.redeemCollateralForDsc(weth, dsce.getCollateralBalanceOfUser(user_2, weth), dscAfterLiquidation);

    // User's balances of WETH after redeeming
    uint256 user_1BalanceAfterRedeeming = wethMock.balanceOf(user_1);
    uint256 user_2BalanceAfterRedeeming = wethMock.balanceOf(user_2);

    console.log("***************************************** RESULTS ***************************************");
    console.log("user_1 balance before deposit  =>  ", user_1BalanceBeforeDeposit);
    console.log("user_2 balance before deposit  =>  ", user_2BalanceBeforeDeposit);
    console.log("user_1 balance after redeeming  =>  ", user_1BalanceAfterRedeeming);
    console.log("user_2 balance after redeeming  =>  ", user_2BalanceAfterRedeeming);
    console.log("*****************************************************************************************");
}

Recommendations

The current behavior of this contract does not get mapping s_DSCMinted and DSC ERC20 balance aligned. If the observed behavior is unintended, it is recommended to have both mappings in line to avoid mismatches.