Collateral cannot be withdrawn from trove once yang is suspended

[c4-bot-3]

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

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-sponsor]

tserg (sponsor) confirmed

[alex-ppg]

The Warden has demonstrated how a Yang's suspension (either temporary or permanent) will cause the overall system to deviate from its specification and disallow withdrawals of the Yang (collateral) instead of permitting them, thereby never letting the system gradually recover.

I consider a medium-risk severity to be appropriate for this exhibit as it relates to misbehavior that will arise during an emergency scenario.

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory