c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/Dependencies/EbtcBase.sol#L129-L130

Vulnerability details

Impact

There are two possible cases where a CDP can be liquidated:

In normal mode, when the ICR falls below the MCR.
In recovery mode, when the ICR falls below the TCR.

However, in recovery mode, there is a risk of accidentally liquidating a CDP with an ICR higher than the TCR.

Proof of Concept

The liquidation threshold check function is as follows:

function _checkICRAgainstLiqThreshold(uint256 _icr, uint _tcr) internal view returns (bool) {
    // Either undercollateralized
    // OR, it's RM AND they meet the requirement
    // Swapped Requirement && RM to save gas
    return
        _checkICRAgainstMCR(_icr) ||
        (_checkICRAgainstTCR(_icr, _tcr) && _checkRecoveryModeForTCR(_tcr));
}

As we can see, the CDP can always be liquidated when ICR < MCR. However, there is a scenario in which the TCR is below the MCR, indicating recovery mode (TCR < MCR < CCR). When a user attempts to liquidate a CDP with an ICR between TCR and MCR (i.e. TCR < ICR < MCR), the CDP is possible to liquidate due to ICR < MCR. This, however, violates the protocol.

Tools Used

Recommended Mitigation Steps

Please modify above check as below:

    return
        - _checkICRAgainstMCR(_icr) ||
        + (_checkICRAgainstMCR(_icr) && !_checkRecoveryModeForTCR(_tcr)) || 
        (_checkICRAgainstTCR(_icr, _tcr) && _checkRecoveryModeForTCR(_tcr));