`changeMinimumCollateralRatioPercent()` can immediately liquidate some users

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L307 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/StableConfig.sol#L118-L124

Vulnerability details

Impact

User who borrows an asset, borrows it on the previously defined conditions. Those conditions define when user can be liquidated: their collateral value / borrowedUSDS ratio is less than the minimum required. This ratio can be, however, changed after the loan was taken. This basically means, that changing stableConfig.minimumCollateralRatioPercent after the borrow may immediately liquidate the user.

Proof of Concept

File: CollateralAndLiquidity.sol

return (( userCollateralValue * 100 ) / usdsBorrowedAmount) < stableConfig.minimumCollateralRatioPercent();

User can be liquidated if the user's position is under-collateralized. This means that his ratio ( userCollateralValue * 100 ) / usdsBorrowedAmount has to be smaller than the one defined in stableConfig.minimumCollateralRatioPercent().

However, it's possible that DAO can change this minimumCollateralRatioPercent() for the currently taken loans:

File: StableConfig.sol

function changeMinimumCollateralRatioPercent(bool increase) external onlyOwner
        {
        if (increase)
            {
            if (minimumCollateralRatioPercent < 120)
                minimumCollateralRatioPercent += 1;
            }
        else

Since stableConfig.minimumCollateralRatioPercent() is nowhere linked to the previously taken borrow, it means that the check if user is under-collateralized is being made on the current value of stableConfig.minimumCollateralRatioPercent(). This constitutes a serious issue:

1. User borrows USDS. He knows that he will be liquidated when his ratio is smaller than 110
2. He keeps his position so that it's always bigger than 110 but smaller than 115 - thus he cannot be liquidated
3. DAO changes the minimumCollateralRatioPercent to 120
4. Now user can be immediately liquidated

The vulnerability here occurs in a way of checking the minimumCollateralRatioPercent. It should be linked to the current borrow. E.g. when user borrows USDS when minimumCollateralRatioPercent was 110 - it should remain the same (110) for that borrow. Calling changeMinimumCollateralRatioPercent() and changing minimumCollateralRatioPercent by the DAO should affect only future borrows and should not affect the current, existing borrows. Otherwise, the DAO will be able to increase minimumCollateralRatioPercent and immediately liquidate most of the users.

Tools Used

Manual code review

Recommended Mitigation Steps

This issues requires the whole redesign of the protocol. User who borrows funds should have linked the current stableConfig.minimumCollateralRatioPercent() value to that borrow. Changing the stableConfig.minimumCollateralRatioPercent() should affect only the future borrows and should not affect previously taken borrows.

Assessed type

Rug-Pull

[c4-judge]

Picodes marked the issue as duplicate of #626

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)