set-root-override fix (#590)

contracts/src/mcms.cairo

@@ -350,8 +350,7 @@ mod ManyChainMultiSig {
             // new root can be set only if the current op_count is the expected post op count
             // (unless an override is requested)
             assert(
-                op_count == current_root_metadata.post_op_count
-                    || current_root_metadata.override_previous_root,
+                op_count == current_root_metadata.post_op_count || metadata.override_previous_root,
                 'pending operations remain',
             );
             assert(op_count == metadata.pre_op_count, 'wrong pre-operation count');

contracts/src/tests/test_mcms/test_set_root.cairo

@@ -21,7 +21,8 @@ use chainlink::mcms::{
 };
 use chainlink::tests::test_mcms::utils::{
     insecure_sign, setup_signers, SignerMetadata, setup_mcms_deploy_and_set_config_2_of_2,
-    setup_mcms_deploy_set_config_and_set_root, set_root_args, merkle_root,
+    setup_mcms_deploy_set_config_and_set_root, set_root_args, set_root_args_override_root,
+    merkle_root,
 };
 use chainlink::utils::{keccak};
 use snforge_std::{
@@ -226,6 +227,48 @@ fn test_set_root_success() {
         );
 }
 
+#[test]
+fn test_root_sucess_and_override() {
+    let (
+        _,
+        mcms_address,
+        mcms,
+        _,
+        _,
+        _,
+        _,
+        _,
+        _,
+        _,
+        root,
+        valid_until,
+        metadata,
+        metadata_proof,
+        signatures,
+        _,
+        _,
+    ) =
+        setup_mcms_deploy_set_config_and_set_root();
+
+    mcms.set_root(root, valid_until, metadata, metadata_proof, signatures);
+
+    let actual_root_metadata = mcms.get_root_metadata();
+    assert(actual_root_metadata == metadata, 'root metadata not equal');
+    assert(!actual_root_metadata.override_previous_root, 'override false');
+
+    let (_, _, _, _, signer_metadata) = setup_signers();
+    let (new_root, new_valid_until, new_metadata, new_metadata_proof, new_signatures, _, _) =
+        set_root_args_override_root(
+        mcms_address, contract_address_const::<123123>(), signer_metadata, 0, 2,
+    );
+
+    mcms.set_root(new_root, new_valid_until, new_metadata, new_metadata_proof, new_signatures);
+
+    let new_root_metadata = mcms.get_root_metadata();
+    assert(new_root_metadata == new_metadata, 'root metadata not equal');
+    assert(new_root_metadata.override_previous_root, 'override true');
+}
+
 #[test]
 #[feature("safe_dispatcher")]
 fn test_set_root_hash_seen() {
@@ -549,7 +592,7 @@ fn test_pending_ops_remain() {
 
     // sign a different set of operations with same signers
     let (_, _, _, _, signer_metadata) = setup_signers();
-    let (root, valid_until, metadata, metadata_proof, signatures, ops, ops_proof) = set_root_args(
+    let (root, valid_until, metadata, metadata_proof, signatures, _, _) = set_root_args(
         mcms_address, contract_address_const::<123123>(), signer_metadata, 0, 2,
     );
 

contracts/src/tests/test_mcms/utils.cairo

@@ -214,6 +214,78 @@ fn merkle_root(leafs: Array<u256>) -> (u256, Span<u256>, Span<Span<u256>>) {
     (root, array![metadata_proof].span(), array![proof1.span(), proof2.span()].span())
 }
 
+fn set_root_args_override_root(
+    mcms_address: ContractAddress,
+    target_address: ContractAddress,
+    mut signers_metadata: Array<SignerMetadata>,
+    pre_op_count: u64,
+    post_op_count: u64,
+) -> (u256, u32, RootMetadata, Span<u256>, Array<Signature>, Array<Op>, Span<Span<u256>>) {
+    let mock_chain_id = 732;
+
+    // first operation
+    let selector1 = selector!("set_value");
+    let calldata1: Array<felt252> = array![1234123];
+    let op1 = Op {
+        chain_id: mock_chain_id.into(),
+        multisig: mcms_address,
+        nonce: 0,
+        to: target_address,
+        selector: selector1,
+        data: calldata1.span(),
+    };
+
+    // second operation
+    let selector2 = selector!("flip_toggle");
+    let calldata2 = array![];
+    let op2 = Op {
+        chain_id: mock_chain_id.into(),
+        multisig: mcms_address,
+        nonce: 1,
+        to: target_address,
+        selector: selector2,
+        data: calldata2.span(),
+    };
+
+    let metadata = RootMetadata {
+        chain_id: mock_chain_id.into(),
+        multisig: mcms_address,
+        pre_op_count: pre_op_count,
+        post_op_count: post_op_count,
+        override_previous_root: true,
+    };
+    let valid_until = 9;
+
+    let op1_hash = hash_op(op1);
+    let op2_hash = hash_op(op2);
+
+    let metadata_hash = hash_metadata(metadata);
+
+    // create merkle tree
+    let (root, metadata_proof, ops_proof) = merkle_root(array![op1_hash, op2_hash, metadata_hash]);
+
+    let encoded_root = BytesTrait::new_empty().encode(root).encode(valid_until);
+    let message_hash = eip_191_message_hash(keccak(@encoded_root.into()));
+
+    let mut signatures: Array<Signature> = ArrayTrait::new();
+
+    while let Option::Some(signer_metadata) = signers_metadata.pop_front() {
+        let (r, s, y_parity) = insecure_sign(message_hash, signer_metadata.private_key);
+        let signature = Signature { r: r, s: s, y_parity: y_parity };
+        let address = recover_eth_ecdsa(message_hash, signature).unwrap();
+
+        // sanity check
+        assert(address == signer_metadata.address, 'signer not equal');
+
+        signatures.append(signature);
+    };
+
+    let ops = array![op1.clone(), op2.clone()];
+
+    (root, valid_until, metadata, metadata_proof, signatures, ops, ops_proof)
+}
+
+
 fn set_root_args(
     mcms_address: ContractAddress,
     target_address: ContractAddress,