Opus - 0xTheC0der's results

Lines of code

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

Vulnerability details

Impact

Once a yang (collateral asset) is suspended on Opus, the following holds true according to the documentation:

• No further deposits can be made. This is enforced by the Sentinel.
• Its threshold will decrease to zero linearly over the SUSPENSION_GRACE_PERIOD.

Therefore, a user will naturally deposit healthier collateral to their trove to maintain its threshold, while withdrawing the unhealthy/suspended collateral as it loses value on Opus, effectively replacing the collateral.

However, this is not possible because the underlying assets of a yang are immediately frozen once suspended. It's clear from the documentation that no more deposits of suspended collateral can be made, but this accidentally also affects the withdrawals.

The abbot::withdraw(...) method calls sentinel::convert_to_yang(...) which in turn calls sentinel::assert_can_enter(...):

fn assert_can_enter(self: @ContractState, yang: ContractAddress, gate: IGateDispatcher, enter_amt: u128) {
    ...
    
    let suspension_status: YangSuspensionStatus = self.shrine.read().get_yang_suspension_status(yang);
    assert(suspension_status == YangSuspensionStatus::None, 'SE: Yang suspended');
    
    ...
}

One can see that the assertion will be triggered once the suspension status is Temporary or Permanent.

As a consequence, a yang's underlying assets are frozen once suspended:

• If the suspension status is still Temporary, the admin can unsuspend the yang to make it withdrawable again.
  However, this also stops its threshold decrease and makes it depositable again, which eliminates the incentives to withdraw in the first place. This essentially breaks the suspension mechanism.
• If the suspension status reaches Permanent, the assets are permanently locked from withdrawal.
  Nevertheless, there is a workaround by closing the trove, which requires all the yin (debt) to be repaid and unnecessarily withdraws all other yangs (collateral assets) of the trove too. Therefore this is not a viable solution for the present issue.

Proof of Concept

The following PoC demonstrates that a yang's underlying assets cannot be withdrawn once suspended.

Add the test case below to src/tests/abbot/test_abbot.cairo and run it with snforge test test_withdraw_suspended_fail.

#[test]
#[should_panic(expected: ('SE: Yang suspended',))]
fn test_withdraw_suspended_fail() {
    let (shrine, sentinel, abbot, yangs, _, trove_owner, trove_id, _, _) = abbot_utils::deploy_abbot_and_open_trove(
        Option::None, Option::None, Option::None, Option::None, Option::None
    );
    let asset_addr: ContractAddress = *yangs.at(0);
    let amount: u128 = WAD_SCALE;

    start_prank(CheatTarget::One(sentinel.contract_address), sentinel_utils::admin());
    sentinel.suspend_yang(asset_addr);
    stop_prank(CheatTarget::One(sentinel.contract_address));

    start_prank(CheatTarget::One(abbot.contract_address), trove_owner);
    abbot.withdraw(trove_id, AssetBalance { address: asset_addr, amount });
}

Tools Used

Manual review

Recommended Mitigation Steps

Replace the accidental assert_can_enter(..) check with those that are really necessary at this point:

diff --git a/src/core/sentinel.cairo b/src/core/sentinel.cairo
index b18edde..9671ca2 100644
--- a/src/core/sentinel.cairo
+++ b/src/core/sentinel.cairo
@@ -156,7 +156,8 @@ mod sentinel {
         // This can be used to simulate the effects of `enter`.
         fn convert_to_yang(self: @ContractState, yang: ContractAddress, asset_amt: u128) -> Wad {
             let gate: IGateDispatcher = self.yang_to_gate.read(yang);
-            self.assert_can_enter(yang, gate, asset_amt);
+            assert(gate.contract_address.is_non_zero(), 'SE: Yang not added'); // alike to sentinel::convert_to_assets(...)
+            assert(self.yang_is_live.read(yang), 'SE: Gate is not live');      // to satisfy test_sentinel::test_kill_gate_and_preview_enter()
             gate.convert_to_yang(asset_amt)
         }
 

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Unhealthy troves with ltv > 90% and threshold < 90% cannot always be absorbed due to a wrong if-condition.
According to Priority of liquidation methods it should always be possible to absorb unhealthy troves with ltv > 90%:

Absorption can happen only after an unhealthy trove's LTV has exceeded the LTV at which the maximum possible penalty is reached, or if it has exceeded 90% LTV. The liquidation penalty in this case will similarly be capped to the maximum of 12.5% or the maximum possible penalty.

However, the purger::get_absorption_penalty_internal(...) method mistakenly checks the threshold instead of the ltv against the ABSORPTION_THRESHOLD (90%) in L467:

fn get_absorption_penalty_internal(
    self: @ContractState, threshold: Ray, ltv: Ray, ltv_after_compensation: Ray
) -> Option<Ray> {
    if ltv <= threshold {
        return Option::None;
    }

    ...

    let mut max_possible_penalty: Ray = min(
        (RAY_ONE.into() - ltv_after_compensation) / ltv_after_compensation, MAX_PENALTY.into()
    );

    if threshold > ABSORPTION_THRESHOLD.into() { // @audit ltv instead
        let s = self.penalty_scalar.read();
        let penalty = min(MIN_PENALTY.into() + s * ltv / threshold - RAY_ONE.into(), max_possible_penalty);

        return Option::Some(penalty);
    }

    let penalty = min(MIN_PENALTY.into() + ltv / threshold - RAY_ONE.into(), max_possible_penalty);

    if penalty == max_possible_penalty {
        Option::Some(penalty)
    } else {
        Option::None
    }
}

As a consequence, unhealthy troves can only be absorbed if they reach the maximum possible penalty although the condition ltv > 90% is already satisfied. This is against the protocol's intended liquidation/absorption incentives and therefore endangers the solvency of the protocol.

By observing the sponsor's graph for liquidation penalty it becomes evident that the MAX_PENALTY can only be achieved for ltv up to 89%. For even higher ltv up to 100%, the penalty approaches 0% due to max_possible_penalty (see code above), which lowers the incentives for liquidation and makes absorption a necessity.
In case of threshold > 83% there is a window where 90% < ltv < ltv@max_possible_penalty causing absorptions to be impossible due to the present bug. This linked graph visualizes the present issue.

Proof of Concept

In the following, a numerical example is provided to demonstrate the above claims.

Initial assumptions:

threshold              = 88%    (reasonable because shrine::MAX_THRESHOLD is 100%)
ltv                    = 91%    (should be eligible for absorption in any case)
ltv_after_compensation = 93%    (reasonable because ltv becomes worse due to compensation)

Let's do the math step-by-step:

fn get_absorption_penalty_internal(
    self: @ContractState, threshold: Ray, ltv: Ray, ltv_after_compensation: Ray
) -> Option<Ray> {
    // @PoC: 91% <= 88% --> false, skip body
    if ltv <= threshold {
        return Option::None;
    }

    ...

    let mut max_possible_penalty: Ray = min(
        (RAY_ONE.into() - ltv_after_compensation) / ltv_after_compensation, MAX_PENALTY.into()
    );
    // @PoC: max_possible_penalty = 7.53%
    
    // @PoC: 88% > 90% --> false, skip body due to wrong check
    if threshold > ABSORPTION_THRESHOLD.into() { // @audit ltv instead
        let s = self.penalty_scalar.read();
        let penalty = min(MIN_PENALTY.into() + s * ltv / threshold - RAY_ONE.into(), max_possible_penalty);

        return Option::Some(penalty);
    }

    let penalty = min(MIN_PENALTY.into() + ltv / threshold - RAY_ONE.into(), max_possible_penalty);
    // @PoC: penalty = 6.41%
    
    // @PoC: 6.41% == 7.53% --> false, go in else
    if penalty == max_possible_penalty {
        Option::Some(penalty)
    } else {
        Option::None    // @PoC: trove is not eligible for absorption
    }
}

We can see that the trove has not reached its maximum possible penalty yet, therefore it cannot be absorbed as expected, although ltv > 90%.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure the absorption threshold is checked against the ltv as intended:

diff --git a/src/core/purger.cairo b/src/core/purger.cairo
index 6a36bbc..820aff5 100644
--- a/src/core/purger.cairo
+++ b/src/core/purger.cairo
@@ -464,7 +464,7 @@ mod purger {
                 (RAY_ONE.into() - ltv_after_compensation) / ltv_after_compensation, MAX_PENALTY.into()
             );
 
-            if threshold > ABSORPTION_THRESHOLD.into() {
+            if ltv > ABSORPTION_THRESHOLD.into() {
                 let s = self.penalty_scalar.read();
                 let penalty = min(MIN_PENALTY.into() + s * ltv / threshold - RAY_ONE.into(), max_possible_penalty);
 

Assessed type

Math

Absorption can happen only after an unhealthy trove's LTV has exceeded the LTV at which the maximum possible penalty is reached, or if its threshold exceeds 90% LTV and its LTV has exceeded the threshold

Summary

• L-01: Abbot allows to open empty troves inflating the troves_count
• L-02: Inability to remove suspended Yangs and zero prices can cause the protocol to freeze
• L-03: Inactive failing reward assets can cause DoS for main Absorber methods
• L-04: Users currently need to approve tokens to the Gates instead of the Abbot
• L-05: Unresolved ToDo about failing oracles
• L-06: Allowing collateral thresholds up to 100% is unhealthy for the protocol
• L-07: Flash loan interface deviates from ERC-3156 specification
• L-08: Absorber removal request timelock leads to unnecessary disadvantage for users

---

• NC-01: Take flash loan fee
• NC-02: Scarb version possibly incompatible with Starknet
• NC-03: Absorption threshold LTV should be set to 89%
• NC-04: Explicitly handle zero value token tranfers
• NC-05: Mistake in documentation - threshold vs. LTV
• NC-06: Mistake in inline comment - minimum vs. maximum

---

L-01: Abbot allows to open empty troves inflating the troves_count

The abbot::open_trove(...) method allows to open empty troves (with 0 collateral).

Impacts:

• Unnecessary increase of troves_count: u64 (very unlikely: DoS due to u64_add Overflow)
• Unnecessary network stress since no funds (except fees) are required

PoC:

The test case below demonstrates that an empty trove can be opened.
Add the test case to src/tests/abbot/test_abbot.cairo and run it with snforge test test_open_empty_trove_pass.

#[test]
fn test_open_empty_trove_pass() {
    let (shrine, _, abbot, yangs, gates) = abbot_utils::abbot_deploy(
        Option::None, Option::None, Option::None, Option::None, Option::None
    );

    let mut spy = spy_events(SpyOn::One(abbot.contract_address));

    // Deploying the first trove
    let trove_owner: ContractAddress = common::trove1_owner_addr();
    let forge_amt: Wad = 0_u128.into();
    //common::fund_user(trove_owner, yangs, abbot_utils::initial_asset_amts());
    let asset_amts: Array<u128> = array![0_u128, 0_u128];
    let deposited_amts: Span<u128> = asset_amts.span();
    let trove_id: u64 = common::open_trove_helper(abbot, trove_owner, yangs, deposited_amts, gates, forge_amt);

    // Check trove ID
    let expected_trove_id: u64 = 1;
    assert(trove_id == expected_trove_id, 'wrong trove ID');
    assert(
        abbot.get_trove_owner(expected_trove_id).expect('should not be zero') == trove_owner, 'wrong trove owner'
    );
    assert(abbot.get_troves_count() == expected_trove_id, 'wrong troves count');

    let mut expected_user_trove_ids: Array<u64> = array![expected_trove_id];
    assert(abbot.get_user_trove_ids(trove_owner) == expected_user_trove_ids.span(), 'wrong user trove ids');
}

Recommendation:
Require a minumum deposit value when opening a trove.

L-02: Inability to remove suspended Yangs and zero prices can cause the protocol to freeze

The protocol only provides functionality to add yangs, but there is no corresponding method for removal.
Furthermore, the protocol allows to suspend yangs, but even if a yang is permanently suspened it cannot be removed.

However, all yangs ever added (not matter if suspended) are iterated by the seer in order to query the orcale(s) for the underlying assets' prices.

Consider the following scenario:
A collateral asset is hacked and/or there is a scandal about the project team (for example: Terra Luna). Therefore, the asset crashes. It gets delisted from major exchanges and suspended from Opus.

Impacts:

• At some point, the orcale(s) might be unable to provide a price feed for the asset and therefore revert/panic.
  This causes DoS for all price updates of the protocol, consequently freezing the whole protocol.
• There might still be a price feed, but the price is 0 (due to lack of data sources or the price is actually zero).
  However, the shrine reverts due to an assertion in case of 0 price provided by the seer.
  This causes DoS for all price updates of the protocol, consequently freezing the whole protocol.

Recommendation:
Allow the protocol's admin to remove yangs or at least exclude them from price updates.
Furthmore, the 0 price assertion is dangerous, even during normal operation. Zero prices should be handled in a graceful way, especially in case of suspended assets, in order to avoid freezing the whole protocol.

L-03: Inactive failing reward assets can cause DoS for main Absorber methods

According to the documentation:

The Absorber supports distribution of whitelisted rewards. The only requirement is that the vesting contracts adhere to the Blesser interface. Caution should be exercised (e.g. checking for non-standard behavior like blacklists) when whitelisting a reward token to ensure that a failure to vest reward tokens does not cause an absorption to revert.

For this purpose, the absorber::set_reward(asset, blesser, is_active) method has an is_active flag to deactivate a reward asset.
This flag is also correctly taken into account in absorber::bestow() which is called by absorber::update(...) on absorption in order to avoid permanent absorption DoS on failing reward tokens.

However, the is_active flag is not taken in account in absorber::get_provider_rewards(...) called by absorber::get_absorbed_and_rewarded_assets_for_provider() which is further called by absorber::reap_helper(...) on absorber::provide(...), absorber::remove(...) and absorber::reap(...).

Impacts:
The methods absorber::provide(...), absorber::remove(...) and absorber::reap(...) are subject to DoS in case a single reward asset becomes faulty (reverts on transfer). Deactivating the asset via is_active = false cannot avoid this issue in the current implementation.

Recommendation:
Skip inactive reward assets within absorber::get_provider_rewards(...):

diff --git a/src/core/absorber.cairo b/src/core/absorber.cairo
index 473275f..710075f 100644
--- a/src/core/absorber.cairo
+++ b/src/core/absorber.cairo
@@ -997,6 +997,11 @@ mod absorber {
                 }
 
                 let reward: Reward = self.rewards.read(current_rewards_id);
+                if !reward.is_active {
+                    current_rewards_id += 1;
+                    continue;
+                }
+
                 let mut reward_amt: u128 = 0;
                 let mut epoch: u32 = provision.epoch;
                 let mut epoch_shares: Wad = provision.shares;

L-04: Users currently need to approve tokens to the Gates instead of the Abbot

According to the documentation:

As the Gate is an internal-facing module, users will not be able to, and are not expected to, interact with the Gate directly.

Users are expected to handle their troves using the abbot module which indirectly interacts with the gate modules for each asset via the sentinel module:

However, a user still needs to know the address of a respective gate and give token approval to the gate instead of the abbot module, because the gate pulls the assets from the user on deposit:

fn enter(ref self: ContractState, user: ContractAddress, trove_id: u64, asset_amt: u128) -> Wad {
    ...

    let success: bool = self.asset.read().transfer_from(user, get_contract_address(), asset_amt.into());
    assert(success, 'GA: Asset transfer failed');

    ...
}

Impacts:
Complicated UX that partially counters the documentation.

Recommendation:
The user should only have to give token approvals to the abbot module which pulls the assets from the user and subsequently forwards them to the respective gate.

L-05: Unresolved ToDo about failing oracles

The following TODO is still unresolved:

// TODO: when possible in Cairo, fetch_price should be wrapped
//       in a try-catch block so that an exception does not
//       prevent all other price updates

Impacts:
It's entirely possible that an oracle pice data query fails/reverts/panics with an exception which causes the whole price update transaction to fail. This causes DoS for all price updates of the protocol, consequently freezing the whole protocol.

Recommendation:
Make sure an oracle's mehods are declared with the nopanic notation, otherwise handle potential errors gracefully with the match syntax instead of directly expecting return data.

L-06: Allowing collateral thresholds up to 100% is unhealthy for the protocol

The protocol currently allows thresholds up to 100% (see MAX_THRESHOLD) for collateral assets. This is unhealthy because liquidations/absorptions can only happen if ltv > threshold. Furthermore, the liquidation penalty at ltv >= 100% is zero, i.e. there is no liquidation incentive and consequently troves with such high thresholds can effectively only be absorbed.

Recommendation: Set the MAX_THRESHOLD to 90% in order to facilitate profitable searcher liquidations in any case.

L-07: Flash loan interface deviates from ERC-3156 specification

The current implementation of the flash_mint module only has methods in snake_case. However, the ERC-3156 specification requires the lenders's and receiver's methods in camelCase which could lead to severe interoperability problems.

Recommendation: Additonally add the camelCase implementation which forwards to the original methods. For reference, see the Shrine's Yin ERC-20 implementation where camelCase support was already implemented.

L-08: Absorber removal request timelock leads to unnecessary disadvantage for users

If a user requests to remove yin from the Absorber whithin the REQUEST_COOLDOWN period, the timelock for removal multiplies.
This makes sense in case the user immediately requests again after a yin removal. However, the timelock is also multiplied in case the user accidentally requests again before a removal, which is simply unfair.

Recommendation:
Only increase the timelock on request within REQUEST_COOLDOWN after a removal:

diff --git a/src/core/absorber.cairo b/src/core/absorber.cairo
index 473275f..ab4ba07 100644
--- a/src/core/absorber.cairo
+++ b/src/core/absorber.cairo
@@ -447,7 +447,12 @@ mod absorber {
 
             let mut timelock: u64 = REQUEST_BASE_TIMELOCK;
             if request.timestamp + REQUEST_COOLDOWN > current_timestamp {
-                timelock = request.timelock * REQUEST_TIMELOCK_MULTIPLIER;
+                if request.has_removed {
+                    timelock = request.timelock * REQUEST_TIMELOCK_MULTIPLIER;
+                }
+                else {
+                    timelock = request.timelock;
+                }
             }
 
             let capped_timelock: u64 = min(timelock, REQUEST_MAX_TIMELOCK);

---

NC-01: Take flash loan fee

Although the fee for yin flash loans is currently zero (FLASH_FEE = 0), it should still be accounted for when withdrawing the loan amount from the borrower, just to make sure this step is not forgotten once the flash fee becomes non-zero in the future.

diff --git a/src/core/flash_mint.cairo b/src/core/flash_mint.cairo
index d2114d8..97c21d9 100644
--- a/src/core/flash_mint.cairo
+++ b/src/core/flash_mint.cairo
@@ -138,7 +138,7 @@ mod flash_mint {
             assert(borrower_resp == ON_FLASH_MINT_SUCCESS, 'FM: on_flash_loan failed');
 
             // This function in Shrine takes care of balance validation
-            shrine.eject(receiver, amount_wad);
+            shrine.eject(receiver, amount_wad + FLASH_FEE.try_into().unwrap());
 
             if adjust_ceiling {
                 shrine.set_debt_ceiling(ceiling);

NC-02: Scarb version possibly incompatible with Starknet

When compiling the protocol's modules via Scarb v2.4.0 as suggested by the README, the following warning arises:

This version is not yet supported on Starknet! If you want to develop contracts deployable to current Starknet, please stick with Scarb v2.3.1

This info might already be obsolete but definitely deserves a second look to be sure.

NC-03: Absorption threshold LTV should be set to 89%

According to the protocol's graph for liquidation penalty, it becomes evident that the MAX_PENALTY can only be achieved for ltv up to 89%. For higher ltv the penalty sinks again towards zero at ltv = 100%. Therefore, the searcher liquidation incentives are getting lower for ltv >= 89%.
As a consequence, it would be more ideal to set the ABSORPTION_THRESHOLD to 89% instead of currently 90%.

NC-04: Explicitly handle zero value token tranfers

According to Starkscan, the desired collateral tokens WBTC, ETH and wstETH (mainnet addresses from Starknet documentation) do not panic on 0 value transfers which is a good thing.
However, there might be other collateral tokens added in the future which show such behaviour. Therefore, explicitly check for and skip 0 value token transfers throughout the protocol.

NC-05: Mistake in documentation - threshold vs. LTV

According to the liquidation penalty documentation:

s is a scalar that is introduced to control how quickly the absorption penalty reaches the maximum possible penalty for thresholds LTV at or greater than 90%. This lets the protocol control how much to incentivize users to call absorb depending on how quick and/or desirable absorptions are for the protocol.

For reference, see key functions documentation:

Absorption can happen only after an unhealthy trove's LTV has exceeded the LTV at which the maximum possible penalty is reached, or if it has exceeded 90% LTV. The liquidation penalty in this case will similarly be capped to the maximum of 12.5% or the maximum possible penalty.

NC-06: Mistake in inline comment - minimum vs. maximum

The following inline comment is wrong because it contradicts the context

// If the threshold is below the given minimum, we automatically
// return the minimum maximum penalty to avoid division by zero/overflow, or the largest possible penalty,
// whichever is smaller.

and is not in line with the subsequent code:

if ltv >= MIN_THRESHOLD_FOR_PENALTY_CALCS.into() {
    return Option::Some(min(MAX_PENALTY.into(), (RAY_ONE.into() - ltv) / ltv));
}