committee_sizes is not properly updated

hats-bug-reporter[bot] commented 5 months ago

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0x4b2b1181f80fffc7b9398573841ce7bc0d171de481009f1ef332f2ad124273b8 Severity: medium

Description: Description\

committee_sizes variable is not updated when calling set_committee() function that may put the contract into an undesirable state as the variable is used for accounting purposes.

Attack Scenario\

In the lib.rs, there is a function called set_committee() that effectively can change the committee. However, the new length of the committee is not inserted into committee_sizes like it’s done in the constructor():

https://github.com/Cardinal-Cryptography/most/blob/70ab234cc3322fda82784413f5e0704907a0e1fe/azero/contracts/most/lib.rs#L238-239

  let mut committee_sizes = Mapping::new();
            committee_sizes.insert(committee_id, &(committee.len() as u128));

And how it's done in the set_committee():

self.ensure_owner()?;
            self.ensure_halted()?;
            Self::check_committee(&committee, signature_threshold)?;

            let mut data = self.data()?;

            let committee_id = data.committee_id + 1;
            let mut committee_set = Mapping::new();
            committee.into_iter().for_each(|account| {
                committee_set.insert((committee_id, account), &());
            });

            self.committees = committee_set;
            data.committee_id = committee_id;

            self.data.set(&data);

            self.signature_thresholds
                .insert(committee_id, &signature_threshold);

            Ok(())
        }

As this mapping is used for accounting purposes, all the places where it’s used will refer to outdated size:

   let total_amount = self
                .get_collected_committee_rewards(committee_id)
                .checked_div(
                    self.committee_sizes
                        .get(committee_id)
                        .ok_or(MostError::NoSuchCommittee)?,
                )
                .ok_or(MostError:

Recommendations

Add the following line to the set_commitee() function:

  committee_sizes.insert(committee_id, &(committee.len() as u128));

rodiontr commented 5 months ago

PoC:

As you can see here, we first set commitee_sizes to 5 in the constructor, then set new committee with only alice later again

 #[ink::test] 
        fn incorrect_commitee_size() {
            let accounts = default_accounts::<DefEnv>();
            set_caller::<DefEnv>(accounts.alice);

            // create new contract
            let mut most = Most::new(
                guardian_accounts(), // 5
                THRESHOLD,
                POCKET_MONEY,
                RELAY_GAS_USAGE,
                MIN_FEE,
                MAX_FEE,
                DEFAULT_FEE,
                None,
                accounts.alice,
            ).expect("Threshold is valid.");

            assert_eq!(most.set_committee(vec![accounts.alice], 1), Ok(()));

            let committee_size = most.committee_sizes.get(1).ok_or(MostError::NoSuchCommittee);
            assert_eq!(committee_size, Ok(1));

        }

This throws an error and shows that the commitee_sizes for commitee_id = 1 is not updated. However, if we write the test like this, it'd work:

 #[ink::test] 
        fn incorrect_commitee_size() {
            let accounts = default_accounts::<DefEnv>();
            set_caller::<DefEnv>(accounts.alice);

            // create new contract
            let mut most = Most::new(
                guardian_accounts(), // 5
                THRESHOLD,
                POCKET_MONEY,
                RELAY_GAS_USAGE,
                MIN_FEE,
                MAX_FEE,
                DEFAULT_FEE,
                None,
                accounts.alice,
            ).expect("Threshold is valid.");

            assert_eq!(most.set_committee(vec![accounts.alice], 1), Ok(()));

            let committee_size = most.committee_sizes.get(0).ok_or(MostError::NoSuchCommittee);
            assert_eq!(committee_size, Ok(5));

        }

Still think it can be considered as HIGH as I first stated in the issue #15 even though you initially disagreed. commitee_sizes is used when calculating rewards, and it will throw an error when calculating rewards for alice

krzysztofziobro commented 4 months ago

Valid submission - minor

It just prevents committee members from claiming their rewards, which does not qualify for any higher severity + can be recovered/fixed by contract owner/governance.