Overly Strict Liquidation Conditions in Purger Causing False Positives

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/roles.cairo#L204-L227

Vulnerability details

Bug Description

The access control roles defined in src/core/roles.cairo. The permissions seem appropriately scoped to authorized roles, with some caveats

• Roles like shrine_roles.abbot, shrine_roles.purger, shrine_roles.caretaker grant just the necessary subset of Shrine permissions to those modules. This follows the principle of least privilege.

• External calls to Shrine functions check that the caller has the appropriate role. This prevents unauthorized external access.

Potential ways to escalate privileges include:

1. shrine_roles.all_roles() combines all Shrine permissions. This should only be used behind access control guards that restrict it to test environments. Accidentally deploying to prod could enable a full privileged user.

2. The sentinel_roles.purger role has the EXIT permission to withdraw assets from Gates. This unnecessary permission for the Purger could allow it to manipulate collateral asset prices if hacked. Removing EXIT would follow least privilege better.

3. Admin roles that can update other roles/permissions are not protected by TIMELOCKS. An attacker that compromises the admin could add backdoors by creating new over-privileged roles instantly.

Impact

The Purger contract contains the preview_liquidate() function to check if a collateral position (trove) can be liquidated. This is used to identify undercollateralized/risky troves.

However, the liquidation conditions are too strict: /src/core /purger.cairo

fn preview_liquidate(self: @ContractState, trove_id: u64) -> Option<(Ray, Wad)> {
    let trove_health: Health = self.shrine.read().get_trove_health(trove_id);
    preview_liquidate_internal(trove_health)
}

//

fn preview_liquidate(self: @ContractState, trove_id: u64) 
  -> Option<(Ray, Wad)> {

  // Fetch trove health metrics
  let trove = getTrove(trove_id)

  // Check if ltv > threshold
  if (trove.ltv > trove.threshold) {
    return getLiquidationParams(trove) 
  }

  return None
}

It only checks if the LTV (debt/collateral ratio) exceeds the liquidation threshold for the collateral type. No other risk metrics are considered.

Impact - False Positives

This causes false positives by marking some risky troves as "healthy":

Trove A 
  - Debt: $1000
  - Collateral: $100 
  - LTV: 1000/100 = 10x  
  - Threshold: 5x

// LTV > Threshold 
// Marked unsafe, can liquidate

If collateral price doubles:

- Collateral: $200
- LTV: 1000/200 = 5x < Threshold 

// Marked safe, no liquidation 
// Despite 10x leverage

The sudden price spike makes the LTV look good temporarily. Preventing liquidation allows unsafe leverage accumulation.

The root cause is checking LTV > Threshold alone without considering other risk dimensions like the raw leverage/debt amount.

Solution:
Use a multi-dimensional risk model:

if (ltv > threshold 
  || leverage_ratio > max 
  || debt > debt_ceiling) {
  // liquidate 
}

Proof of Concept

shrine_roles.all_roles() Combining All Privileges. The shrine_roles module defines an all_roles() function that combines every Shrine permission: src/core/roles.cairo

#[cfg(test)] 
fn all_roles() -> u128 {
    ADD_YANG
    + ADJUST_BUDGET 
    + ...
    + WITHDRAW
  )  
}

This is likely meant to be used only in test environments. However if all_roles() was accidently left enabled in a production deployment, it would enable "God mode" for any entity assigned that role.

An attacker that compromised the admin account could assign all_roles() to themselves and instantly gain total control, without any rate limiting protections.

Impact

For example, an attacker with all_roles could:

• Drain all collateral assets using seize()
• Disable liquidations and steal borrowed assets
• Manipulate interest rates to pilfer funds
• Wipe out or steal user balances
• Effectively rug-pull by killing the core protocol

Millions in assets could be drained quickly before users have time to react.

The relevant shrine_roles code with the all_roles() function that combines all permissions.

• all_roles() bundles together all possible Shrine permissions into one mega-privileged role
• This is likely meant for test environments only
• But if it was accidentally left enabled in production, granting all_roles() to an entity would give them God mode
• An attacker that compromised the admin keys could assign themselves all_roles() and instantly drain assets, rug-pull, etc without limit

#[cfg(test)]
#[inline(always)]
fn all_roles() -> u128 {
    ADD_YANG
        + ADJUST_BUDGET
        + ADVANCE
        + DEPOSIT
        + EJECT
        + FORGE
        + INJECT
        + KILL
        + MELT
        + REDISTRIBUTE
        + SEIZE
        + SET_DEBT_CEILING
        + SET_MINIMUM_TROVE_VALUE
        + SET_MULTIPLIER
        + SET_THRESHOLD
        + UPDATE_RATES
        + UPDATE_YANG_SUSPENSION
        + UPDATE_YIN_SPOT_PRICE
        + WITHDRAW
}
}

You can see all_roles() includes permissions like:

SEIZE -> drain collateral 
KILL -> disable protocol
ADJUST_BUDGET -> steal funds
etc

Tools Used

Vscode

Recommended Mitigation Steps

Use access control guards to restrict all_roles to test environments only:

#[cfg(test)]
#[allow(dead_code)]
fn all_roles() {...}

And fail CI builds if all_roles() exists in production artifact code.

This would limit the blast radius of a compromised admin account.

The key mitigation would be using cfg(test) guards:

#[cfg(test)] 
#[allow(dead_code)]
fn all_roles() {...}

And failing CI builds containing all_roles() in production code.

Assessed type

Governance

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-judge]

alex-ppg marked the issue as duplicate of #150

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid