codehawks-bot commented 1 year ago

High - The liquidate() function underflows due to LIQUIDATION_BONUS

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L229-L262

https://github.com/Cyfrin/2023-07-foundry-defi-stablecoin/blob/d1c5501aa79320ca0aeaa73f47f0dbc88c7b77e2/src/DSCEngine.sol#L285

Summary

It is a known issue that the protocol will break if it is insolvent. However, above 100% collateral rate (solvent) it is supposed to work, which is not the case.

The 10% liquidation_bonus breaks the liquidate() function when the collateral rate is above 100% but below 110%

Vulnerability Details

Observe the example in the comments of the liquidate function()

        // We want to burn their DSC "debt"
        // And take their collateral
        // Bad User: $140 ETH, $100 DSC
        // debtToCover = $100
        // $100 of DSC == ??? ETH?
        // 0.05 ETH
        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

This works fine as long as the bad debt is above $110, but what happens when it is below?

Example:

bad debt = $105 ETH, 100 DSC
liquidator pays 100 DSC and receives $100*1.10 = 110 dollar in ETH

The liquidate() function will call _redeemCollateral() which will then try to deduct 110 dollar in ETH from a balance of 105 dollar in ETH and cause an underflow which will then revert the function.

 s_collateralDeposited[from][tokenCollateralAddress] -= amountCollateral;

POC in Github Gist

Impact

Collateral positions that should be callable by liquidate() are not working and this breaks a critical function of the system.

Tools Used

Manual review, Foundry

Recommendations

Implement a check in the liquidate() function that makes sure the amount to be received by the liquidator can never be greater than the amount held by the initial collateral holder.

uint256 remainingValueCollateral = getUsdValue(collateral,s_collateralDeposited[from][tokenCollateralAddress];
if( remainingValueCollateral<=totalCollateralToRedeem ){ totalCollateralToRedeem = remainingValueCollateral;};

14si2o commented 1 year ago

Hello,

I would like to argue that the findings https://www.codehawks.com/finding/cllv56ds601bbw9blqbq2ffni and https://www.codehawks.com/finding/cllv56fps01bhw9bl2lxxqfc6 have the same root and effect and therefore should be merged together.

They both describe the same finding, that the 10% fixed bonus will cause the _redeemCollateral function to revert, which means that full liquidation is not possible and which might cause a liquidation crisis.

The first has a better PoC and the second a more detailed impact description, but the cause & effect are identical so these should not be separate issues.

PatrickAlphaC commented 1 year ago

Checking with Hans

PatrickAlphaC commented 1 year ago

Agreed. We will combine them.

Cyfrin / 2023-07-foundry-defi-stablecoin