Some positions can become instantly liquidatable after a DAO proposal is finished

[c4-bot-7]

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-L135 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Parameters.sol#L95 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L122

Vulnerability details

Impact

Healthy positions near the liquidation threshold can become instantly liquidatable after a parameter configuration is changed

Note: This Impact fits into the Attack Ideas: "Any issue related to the USDS stablecoin, collateral and the liquidation process."

Proof of Concept

The protocol has a function called canUserBeLiquidated() to check if a user can be liquidated. It compares the borrowed amount ratio with the config value minimumCollateralRatioPercent:

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

CollateralAndLiquidity.sol#L307

This value is changed via changeMinimumCollateralRatioPercent() and can be decremented with an adjustment of 1% at a time.

The function to decrease that config value is called via the Parameters contract, after the corresponding "parameter" DAO proposal is finalized.

Proposals can be finalized at any moment once the ballot minimum end time is reached.

So, as soon as someone finalizes the proposal to change the minimumCollateralRatioPercent, all healthy positions near the liquidation threshold will become automatically liquidatable.

Recommended Mitigation Steps

One way of fixing this is to implement config changes that can affect liquidations via a timelock, to give time to users to repay their debt.

Assessed type

Governance

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

Members will be made aware of impactful proposals that look like they will soon be able to be executed.

[Picodes]

As there is a voting period, this falls within QA to me. Users have a minimum time to react and adapt to pending changes.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[0xcats]

As there is a voting period, this falls within QA to me. Users have a minimum time to react and adapt to pending changes.

Hi @Picodes , thanks for your judging efforts. Although I see your point, assuming users would always be aware of ALL actively voted on proposals doesn't seem too fair to me as there can be many different ones open at a single time. Wouldn't you agree that it is more fair to implement a grace period after the actual finalization as done by standard. That way if this happens the protocol could never be at fault as they've at least taken necessary measures.

[Picodes]

@0xcats the question is not to know if I think it would be better or "more fair" with a grace period or not - I do agree with you. The question is does this fulfill the definition for Medium severity: "assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements". I don't think it does and you haven't added any argument to show that it does.

[0xcats]

@0xcats the question is not to know if I think it would be better or "more fair" with a grace period or not - I do agree with you. The question is does this fulfill the definition for Medium severity: "assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements". I don't think it does and you haven't added any argument to show that it does.

The fact of the matter is that while a proposal is still being voted on the votes for/against as well as quorum change over the timeline of the proposal. As I said earlier, I don't believe users will always be tracking how many open proposals there are and every one of their votes since there can be many random proposals anyway. On the other hand, once the proposal is actually finalized and takes place, the positions can be liquidated instantly. That itself is the issue, and I believe it leads to the users' assets being at risk unfairly. Even the sponsors' suggested fix on #443 does not seem like a good idea - notifying users if a proposal "is on the verge" of passing does not mean it will actually pass. The situation can be reduced to a YES/NO one by taking action only such as grace period only after passing.

That was my final comment on the topic, thank you for taking your time, I'll respect your final decision.

[Picodes]

Thanks for your comment!

"As I said earlier, I don't believe users will always be tracking how many open proposals there are and every one of their votes since there can be many random proposals anyway." -> This is actually what's not convincing to me. First I still don't see what's broken here - to me it's a design choice. Secondly even with a grace period you still need users to track pending proposals so this should happen anyway.