fix: disable multisig after remove signer with etd expires

In case the client side did not bundle remove signer with etd together with disable multisig with etd we need to disable multisig when the remove signer happens as it does not make sense to keep multisig in this scenario.

The user can still disable multisig with etd in this scenario but he will have to wait for ETD (4 days) - we would like to avoid that in case of client side bug.

src/account/Account.cairo

@@ -390,6 +390,8 @@ func __validate__{
 
     // Account state House Keeping
     Account._migrate_storage_if_needed();
+    Multisig.apply_elapsed_etd_requests(block_timestamp);
+    Signers.apply_elapsed_etd_requests(block_timestamp);
 
     let (account_valid) = Account.account_validate(
         call_array_len, call_array,

src/multisig/library.cairo

@@ -1,7 +1,7 @@
 %lang starknet
 
 from starkware.cairo.common.alloc import alloc
-from starkware.cairo.common.bool import TRUE
+from starkware.cairo.common.bool import TRUE, FALSE
 from starkware.cairo.common.cairo_builtins import HashBuiltin, SignatureBuiltin
 from starkware.cairo.common.hash_state import (
     hash_init,
@@ -27,7 +27,10 @@ from src.account.library import (
     AccountCallArray,
     Call,
 )
-from src.signers.library import Signers
+from src.signers.library import (
+    Account_signers_num_hw_signers,
+    Signers
+)
 from src.utils.constants import (
     ACCOUNT_DEFAULT_EXECUTION_TIME_DELAY_SEC,
     DISABLE_MULTISIG_SELECTOR,
@@ -462,10 +465,8 @@ namespace Multisig {
         syscall_ptr: felt*,
         pedersen_ptr: HashBuiltin*,
         range_check_ptr
-    }(
-        disable_multisig_req: DeferredMultisigDisableRequest,
-        block_timestamp: felt
-    ) -> () {
+    }(block_timestamp: felt) -> () {
+        let (disable_multisig_req) = Multisig_deferred_disable_request.read();
         let have_disable_multisig_etd = is_not_zero(disable_multisig_req.expire_at);
         let disable_multisig_etd_expired = is_le_felt(
             disable_multisig_req.expire_at, block_timestamp);
@@ -489,18 +490,31 @@ namespace Multisig {
         tx_info: TxInfo*, block_timestamp: felt, block_num: felt,
     ) -> (valid: felt, is_multisig_mode: felt) {
         alloc_locals;
+
+        let (num_multisig_signers) = Multisig_num_signers.read();
+        let is_multisig_mode = is_not_zero(num_multisig_signers);
+        if (is_multisig_mode == 0) {
+            return (valid=TRUE, is_multisig_mode=FALSE);
+        }
+
+        let (num_additional_signers) = Account_signers_num_hw_signers.read();
+        let have_additional_signers = is_not_zero(num_additional_signers);
+        if (have_additional_signers == FALSE) {
+            // This will happen when remove signer with etd was not bundled
+            // with a disable multisig with etd, so we handle it here.
+            disable_multisig();
+            return (valid=TRUE, is_multisig_mode=FALSE);
+        }
+
+
         let (pending_multisig_txn) = Multisig_pending_transaction.read();
         let (pending_multisig_txn) = discard_expired_multisig_pending_transaction(
             pending_multisig_txn,
             block_num, block_timestamp
         );
         let (local current_signer) = Signers.resolve_signer_from_sig(
             tx_info.signature_len, tx_info.signature);
-        let (disable_multisig_req) = Multisig_deferred_disable_request.read();
-        apply_elapsed_etd_requests(disable_multisig_req, block_timestamp);
 
-        let (num_multisig_signers) = Multisig_num_signers.read();
-        let is_multisig_mode = is_not_zero(num_multisig_signers);
         tempvar is_est_fee = 1 - is_not_zero(tx_info.version - TX_VERSION_1_EST_FEE);
         tempvar is_sign_pending_selector = 1 - is_not_zero(
             [call_array].selector - SIGN_PENDING_MULTISIG_TXN_SELECTOR
@@ -514,7 +528,7 @@ namespace Multisig {
             is_sign_pending_selector *
             is_stark_signer) == 1) {
             dummy_secp256r1_ecdsa_for_gas_fee();
-            return (valid=TRUE, is_multisig_mode=is_multisig_mode);
+            return (valid=TRUE, is_multisig_mode=TRUE);
         }
 
 

src/signers/library.cairo

@@ -272,7 +272,7 @@ namespace Signers {
             tempvar pedersen_ptr = pedersen_ptr;
             tempvar range_check_ptr = range_check_ptr;
         } else {
-            // FIXME: ASSERT (any maybe remove revokes handling)
+            // FIXME: ASSERT (and maybe remove revokes handling)
             tempvar syscall_ptr = syscall_ptr;
             tempvar pedersen_ptr = pedersen_ptr;
             tempvar range_check_ptr = range_check_ptr;
@@ -380,10 +380,8 @@ namespace Signers {
         syscall_ptr: felt*,
         pedersen_ptr: HashBuiltin*,
         range_check_ptr
-    }(
-        remove_signer_req: DeferredRemoveSignerRequest,
-        block_timestamp: felt
-    ) -> () {
+    }(block_timestamp: felt) -> () {
+        let (remove_signer_req) = Account_deferred_remove_signer.read();
         let have_remove_signer_etd = is_not_zero(remove_signer_req.expire_at);
         let remove_signer_etd_expired = is_le_felt(remove_signer_req.expire_at, block_timestamp);
 
@@ -407,15 +405,12 @@ namespace Signers {
         in_multisig_mode,
     ) -> (valid: felt) {
         alloc_locals;
-        let (local remove_signer_req) = Account_deferred_remove_signer.read();
-        apply_elapsed_etd_requests(remove_signer_req, block_timestamp);
 
         // Authorize Signer
         _authorize_signer(
             tx_info.account_contract_address,
             tx_info.signature_len, tx_info.signature,
             call_array_len, call_0_to, call_0_sel,
-            remove_signer_req,
             block_timestamp,
             in_multisig_mode,
         );
@@ -439,7 +434,6 @@ namespace Signers {
         self: felt,
         signature_len: felt, signature: felt*,
         call_array_len: felt, call_0_to: felt, call_0_sel: felt,
-        remove_signer_req: DeferredRemoveSignerRequest,
         block_timestamp: felt,
         in_multisig_mode: felt,
     ) -> () {
@@ -455,13 +449,6 @@ namespace Signers {
             return ();
         }
 
-        // We expected all expired requests to be removed prior to signer authorization
-        let have_etd = is_not_zero(remove_signer_req.expire_at);
-        let etd_expired = is_le_felt(remove_signer_req.expire_at, block_timestamp);
-        with_attr error_message("Signers: expired request not removed") {
-            assert have_etd * etd_expired = 0;
-        }
-
         let (valid_signer_type) = _is_valid_secp256r1_signer_type(signer.signer.type);
         if (valid_signer_type == 1) {
             // We either don't have a pending removal, or it wasn't expired yet

tests/test_Account.py

@@ -2020,6 +2020,65 @@ async def test_multisig_disable_with_etd(init_contracts):
     assert response.call_info.retdata[1] == signer.public_key
 
 
+@pytest.mark.asyncio
+async def test_multisig_disable_after_remove_signer_etd_expire(init_contracts):
+    starknet, _, account1, _, _ = init_contracts
+
+    ecc_signer = TestECCSigner()
+
+    response = await signer.send_transactions(
+        account1,
+        [
+            (
+                account1.contract_address,
+                "add_signer",
+                [
+                    *ecc_signer.pk_x_uint256,
+                    *ecc_signer.pk_y_uint256,
+                    2,  # secp256r1
+                    0,
+                    0,
+                ],
+            ),
+            (account1.contract_address, "set_multisig", [2]),
+        ],
+    )
+    signer_id = response.call_info.retdata[1]
+
+    response = await signer.send_transactions(
+        account1,
+        [
+            (account1.contract_address, "remove_signer_with_etd", [signer_id]),
+        ],
+    )
+
+    # Make sure no multisig as remove_signer should not be deferred
+    execution_info = await account1.get_pending_multisig_transaction().call()
+    assert execution_info.result.pending_multisig_transaction.transaction_hash == 0
+
+    # Make sure we have a deferred request
+    exec_info = await account1.get_deferred_remove_signer_req().call()
+    deferred_request = exec_info.result.deferred_request
+    assert deferred_request.expire_at != 0
+
+    starknet.state.state.block_info = BlockInfo.create_for_testing(
+        0, deferred_request.expire_at + 1
+    )
+
+    # Deferred expired so we expect multisig to be removed, i.e. txn will execute as usual
+    response = await signer.send_transactions(
+        account1, [(account1.contract_address, "getPublicKey", [])]
+    )
+    assert_event_emitted_in_call_info(
+        response.validate_info.internal_calls[0],
+        from_address=account1.contract_address,
+        keys="MultisigDisabled",
+        data=[],
+    )
+
+    assert response.call_info.retdata[1] == signer.public_key
+
+
 @pytest.mark.asyncio
 async def test_multisig_cancel_disable_with_etd(init_contracts):
     starknet, _, account1, _, _ = init_contracts