Changing committee to a higher signature threshold will render a request from the previous committee un-processable

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @rnemes4 Submission hash (on-chain): 0x19c4ac85b9934ad8658ebdf55086e7b84773f7e023ba43c39f25fdb00a9fb99c Severity: medium

Description:

Title:

Changing committee to a higher signature threshold will render a request from the previous committee un-processable

Severity:

Medium

Description

The function azero/contracts/most/lib.rs::receive_request incorrectly uses the current committee signature threshold value as can be seen in the following snippet:

let signature_threshold = self
                .signature_thresholds
                .get(data.committee_id) // @audit This shuld be using `committee_id` instead of `data.committee_id`?
                .ok_or(MostError::InvalidThreshold)?;

This means that a request from a previous committee with a lower threshold than the current would not be able to be processed if the committee members are changed and the threshold is increased.

Scenario:

• An initial committee is setup with 4 members charlie, dave, eve and ferdie with a threshold of 4
• Alice creates a request
• A new committee is created with 5 members charlie, dave, eve, mat and jenny with a threshold of 5
• Alices request is sent by charlie, dave, eve and ferdie, how ever the request will not get processed due to not meeting the current threshold of 5
• Alices request will be stuck and the funds she sent with will not be recoverable unless a new committee is created with exactly the same members and threshold as the original committee

Recommendation

In order top fix this issue the following ammendment is recommended

let signature_threshold = self
                .signature_thresholds
                .get(committee_id) // @audit This shuld be using `committee_id` instead of `data.committee_id`?
                .ok_or(MostError::InvalidThreshold)?;

ie: use the committee_id supplied in the request rather than the current committee Id

Note

After checking the Solidity version, it was confirmed that the recommendation above is implemented correctly there.

POC

Add the follwoing e2e test to azero/contracts/tests/lib.rs

#[ink_e2e::test]
    fn can_process_previous_committee_requests_after_updating_committee_with higher_threshold(mut client: ink_e2e::Client<C, E>) {
        let (most_address, token_address) = setup_default_most_and_token(&mut client, true).await;

        most_set_halted(&mut client, &alice(), most_address, true)
            .await
            .expect("can set halted");

        most_set_committee(
            &mut client,
            &alice(),
            most_address,
            &guardian_ids()[1..],
            4, // Threshold
        )
        .await
        .expect("can set committee");

        most_set_halted(&mut client, &alice(), most_address, false)
        .await
        .expect("Set halt should succeed");

        let old_committee_id = 1;

        let amount = 841189100000000;

        let receiver_address = account_id(AccountKeyring::One);
        let request_nonce = 1;

        let request_hash = hash_request_data(
            old_committee_id,
            token_address,
            amount,
            receiver_address,
            request_nonce,
        );

        // Change Committee

        most_set_halted(&mut client, &alice(), most_address, true)
            .await
            .expect("can set halted");

        most_set_committee(
            &mut client,
            &alice(),
            most_address,
            &guardian_ids(),
            5, // Threshold,
        )
        .await
        .expect("can set committee");

        most_set_halted(&mut client, &alice(), most_address, false)
            .await
            .expect("can set halted");

        for i in 1..(guardian_ids().len() as usize) {
            let signer = &guardian_keys()[i];

            let receive_res = most_receive_request(
                &mut client,
                signer,
                most_address,
                request_hash,
                old_committee_id, // initial committee
                *token_address.as_ref(),
                amount,
                *receiver_address.as_ref(),
                request_nonce,
            )
            .await;

            match receive_res {
                Ok(call_res) => {
                    let events = call_res.events;
                    if i == ((guardian_ids().len() - 1) as usize) {
                        // @audit-info this is the last signature, so the request should be processed
                        assert_eq!(events.len(), 3); // Test fails here
                        assert_eq!(
                            filter_decode_events_as::<RequestProcessed>(vec![events[2].clone()])[0],
                            RequestProcessed {
                                request_hash,
                                dest_token_address: *token_address.as_ref(),
                            }
                        );
                    } else {
                        assert_eq!(events.len(), 1);
                        assert_eq!(filter_decode_events_as::<RequestSigned>(events).len(), 1);
                    }
                }
                Err(e) => panic!("Receive request should succeed: {:?}", e),
            }
        }
    }

[krzysztofziobro]

Valid submission - medium

[rodiontr]

Valid submission - medium

@krzysztofziobro this seems to be the expected behavior, old committee should not be able to process requests at all or am i missing something ? The committee that is set in set_committee() is used to process requests

[rodiontr]

and when the owner sets new commitee, he knows that there are pending requests so it's up to him whether to do it or no

[Jonsey]

Line 369 suggests that this function should deal with previous committees by checking that the committee id provided through the request should be allowed to be processed.

        self.only_committee_member(committee_id, caller)?;

However, later on in the function the committee is restricted to the current committee. This suggests that there is an incongruous intention within the function The initial check is for any committee, but later on the function will fail if the request is not from the current committee. If the admin changes the committee, the previous requests will be stuck. If this is the intention, previous requests should be cancelled and refunded before changing the committee

[rodiontr]

Line 369 suggests that this function should deal with previous committees by checking that the committee id provided through the request should be allowed to be processed.

        self.only_committee_member(committee_id, caller)?;

However, later on in the function the committee is restricted to the current committee. This suggests that there is an incongruous intention within the function The initial check is for any committee, but later on the function will fail if the request is not from the current committee. If the admin changes the committee, the previous requests will be stuck. If this is the intention, previous requests should be cancelled and refunded before changing the committee

yes but it's the owner's responsibility to cancel them and vulnerabilities related to owner actions are out of scope

From the guidelines:

Attacks that require special conditions: malicious tokens, mistakes in governance actions, unless specifically allowed in impact categories.

It basically needs the owner to make a mistake and not cancel the requests, otherwise, this vulnerability will not work