Immutable `relay_gas_usage` will have issues after change in committee

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xf5592957189a233b4c4fe9c758b1f4cce88554c3c37a036fd780617a750f6fef Severity: medium

Description: Description\

relay_gas_usage is used a part of base_fee calculation which is shown as:

        pub fn get_base_fee(&self) -> Result<Balance, MostError> {

        . . . some code

            let base_fee = gas_price
@>           .checked_mul(self.data()?.relay_gas_usage)
                .ok_or(MostError::Arithmetic)?
                .checked_mul(100u128 + BASE_FEE_BUFFER_PERCENTAGE)
                .ok_or(MostError::Arithmetic)?
                .checked_div(100u128)
                .ok_or(MostError::Arithmetic)?;
            Ok(base_fee)
        }

Per contract Natspec relay_gas_usage is calculated with formula as:

This value is calculated by summing the total gas usage of all the transactions it takes to relay a single request and dividing it by the current committee size and multiplying by 1.2

To simplify the formula it looks as:

relay_gas_usage = Total gas usage / committee size * 1.2

The denominator has committee size which usually varies with different committee_Ids.

ink! Most contract has provision of changing the committee members via. set_committee() function.

        pub fn set_committee(
            &mut self,
            committee: Vec<AccountId>,
            signature_threshold: u128,
        ) -> Result<(), MostError> {

     . . . some code

}

The contract does not have function to set the relay_gas_usage in case the committe members are changed. This means the committe members can either be reduced or increased as the set_committee function takes Vec<AccountId> which is an array taking the committee members adress.

The tests considered RELAY_GAS_USAGE as 50000 and committe_size as 5.

POC\ 1) The owner of contract has set the relay_gas_usage value in constructor by calculating the formula as defined above and here. As a part of setup, committe sizes with threshold is also set and finalized.

2) It should be noted that relay_gas_usage value is heavily dependent on committe_size, therefore any change in committe should directly result changes in value of relay_gas_usage.

3) The contract owner and protocol team decides to modify the committee say either they want to increase the committe size or probably decrease. The contract owner simply set the committee again.

4) Since, with the change in committee, the value of relay_gas_usage will also be changed so contract owner tries to change it but he can not as there is no setter function to change relay_gas_usage. Once the relay_gas_usage is set in constructor or contract initialization then it will be fixed forever and it can not be changed. No matter how large the committee size would be, the value of relay_gas_usage will always be fixed.

The value of relay_gas_usage can heavily increase or decrease depending on committee size, therefore incorrect base_fee will reported by get_base_fee() so the base_fee used by functions like send_Request will have to behave incorrectly as far as base fee is concerned. This would make any calculation related to base_fee as wrong, the value will be heavily deviated than it is supposed to be as per intended design.

The issue is identified as Medium severity as its breaking the intended design for calculation of relay_gas_usage. The Natspec compliance won't be followed by contract once the contract is deployed.

Recommended Mitigation steps\ Add setter function named as set_relay_gas_usage(). This function should be accessed by contract owner and it should be used when there is change in committe size to determine the relay_gas_usage respectively.

Note: For formula implementation of relay_gas_usage, This comment can be referred.

[krzysztofziobro]

Duplicate of https://github.com/hats-finance/Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d/issues/30

[0xRizwan]

@krzysztofziobro Since it's conveyed via discord, submissions would need POC. Doesn't #30. lacks POC? I think this issue is described in detailed, so should be valid.