Attacker can lock every trove withdrawals

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/abbot.cairo#L210 https://github.com/code-423n4/2024-01-opus/blob/main/src/core/sentinel.cairo#L159 https://github.com/code-423n4/2024-01-opus/blob/main/src/core/sentinel.cairo#L288

Vulnerability details

Description

During withdraw, the convert_to_yang function of sentinel is called. This function converts an asset amount to a Yang amount.

convert_to_yang then calls assert_can_enter to ensure that current_total + enter_amt <= max_amt. This check is incorrect in the case of a withdrawal, a substraction should be made instead of an addition.

An attacker can deposit an asset amount such that current_total == max_amt. In such condition, withdrawing asset will be impossible because of the incorrect check.

Impact

In every Gate for which a maximum amount of asset is set in Sentinel, an attacker can lock asset withdrawals for every users.

Proof of Concept

The following test can be added to src/tests/abbot/test_abbot.cairo to show a Proof-of-Concept of the issue:

rust📋
    #[test]
    #[should_panic(expected: ('SE: Exceeds max amount allowed',))]
    fn test_zigtur_exploit_withdraw_exceeds_asset_cap_fail() {
        let (_, sentinel, abbot, yangs, gates, 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 gate_addr: ContractAddress = *gates.at(0).contract_address;
        let gate_bal = IERC20Dispatcher { contract_address: asset_addr }.balance_of(gate_addr);

        // Set the maximum to current value, which means `current_total == max_amt`
        start_prank(CheatTarget::One(sentinel.contract_address), sentinel_utils::admin());
        let new_asset_max: u128 = gate_bal.try_into().unwrap();
        sentinel.set_yang_asset_max(asset_addr, new_asset_max);
        stop_prank(CheatTarget::One(sentinel.contract_address));

        // User withdraws, so the max should not be reached.
        // But `assert_can_enter()` in sentinel uses addition even if withdrawing, so it reverts.
        let amount: u128 = 1;
        start_prank(CheatTarget::One(abbot.contract_address), trove_owner);
        abbot.withdraw(trove_id, AssetBalance { address: asset_addr, amount });
    }

Note: The PoC doesn't show the attacker adding Yang asset to make withdrawals impossible. It only shows that the withdrawals are impossible, but the attack vector is clearly identifiable.

Tools Used

Manual review and unit testing.

Recommended Mitigation Steps

During withdrawals, a withdraw_to_yang() function could be used instead of convert_to_yang(). This new function would not call assert_can_enter.

The following patch fixes this issue by implementing a withdraw_to_yang function which calculates the amount of Yang without reverting.

diff📋
diff --git a/src/core/abbot.cairo b/src/core/abbot.cairo
index 1f0a589..1ededa4 100644
--- a/src/core/abbot.cairo
+++ b/src/core/abbot.cairo
@@ -207,7 +207,7 @@ mod abbot {
             let user = get_caller_address();
             self.assert_trove_owner(user, trove_id);
 
-            let yang_amt: Wad = self.sentinel.read().convert_to_yang(yang_asset.address, yang_asset.amount);
+            let yang_amt: Wad = self.sentinel.read().withdraw_to_yang(yang_asset.address, yang_asset.amount);
             self.withdraw_helper(trove_id, user, yang_asset.address, yang_amt);
         }
 
diff --git a/src/core/sentinel.cairo b/src/core/sentinel.cairo
index b18edde..644bb85 100644
--- a/src/core/sentinel.cairo
+++ b/src/core/sentinel.cairo
@@ -160,6 +160,15 @@ mod sentinel {
             gate.convert_to_yang(asset_amt)
         }
 
+        fn withdraw_to_yang(self: @ContractState, yang: ContractAddress, asset_amt: u128) -> Wad {
+            let gate: IGateDispatcher = self.yang_to_gate.read(yang);
+            assert(gate.contract_address.is_non_zero(), 'SE: Yang not added');
+            assert(self.yang_is_live.read(yang), 'SE: Gate is not live');
+            let suspension_status: YangSuspensionStatus = self.shrine.read().get_yang_suspension_status(yang);
+            assert(suspension_status == YangSuspensionStatus::None, 'SE: Yang suspended');
+            gate.convert_to_yang(asset_amt)
+        }
+
         // This can be used to simulate the effects of `exit`.
         fn convert_to_assets(self: @ContractState, yang: ContractAddress, yang_amt: Wad) -> u128 {
             let gate: IGateDispatcher = self.yang_to_gate.read(yang);
diff --git a/src/interfaces/ISentinel.cairo b/src/interfaces/ISentinel.cairo
index 4149a38..04d0d04 100644
--- a/src/interfaces/ISentinel.cairo
+++ b/src/interfaces/ISentinel.cairo
@@ -33,5 +33,6 @@ trait ISentinel<TContractState> {
     fn unsuspend_yang(ref self: TContractState, yang: ContractAddress);
     // view
     fn convert_to_yang(self: @TContractState, yang: ContractAddress, asset_amt: u128) -> Wad;
+    fn withdraw_to_yang(self: @TContractState, yang: ContractAddress, asset_amt: u128) -> Wad;
     fn convert_to_assets(self: @TContractState, yang: ContractAddress, yang_amt: Wad) -> u128;
 }

Note: Unit tests are all passing with the fix. To apply the patch, import the content in a fix.patch file, then execute git apply fix.patch.

Assessed type

DoS