relayers would be permanently inoperative due to permanent emergency

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x3b84661faf4c4dff6ff1fc70b033bb7646966a35be01f5c8de754ac3df6c5809 Severity: minor

Description: Description\

Advisory contract has global flag i.e emergency variable which signals to relayers that there is some issue with the bridge and they should stop observing the chains and submitting transactions.

The state of true/false of emergency will act as the heart of relayer working.

The contract owner of Advisory can call set_emergency() to change the emergency state which will affect directly or indirectly working of relatyers.

    impl Advisory {
        #[ink(message)]
        pub fn set_emergency(&mut self, new_state: bool) -> Result<(), AdvisoryError> {
            self.ensure_owner()?;

            if new_state != self.emergency {
                self.env().emit_event(EmergencyChanged {
                    previous_state: self.emergency,
                    new_state,
                    caller: self.env().caller(),
                });
                self.emergency = new_state;
            }

            Ok(())
        }

Consider the below scenario to get the issue, 1) The owner of Advisory calls set_emergency() function to set it as true means the emergency is applied and active till the owner change the state.

2) Now, the owner and the protocol team thought to change the ownership of Advisorycontract so owner calls transfer_ownership(). The owner accidentally/mistakenly/intensionally passed zero address as contract owner. Since, the ownership in this contract is single step ownership transfer, therefore the owner of Advisory is now zero address(whose private key is unknown). There will be no revert while setting new_owner to zero address due to missing validation checks.

3) To remind, the emergency state was enabled and owner is now zero address, therefore relayers working is permanently stopped.

This issue would hugely affect the relayers as relayers uses the Advisory contract instance to get the emergency state and further decide to work accordingly.

Most of ink! contracts has used 2 step ownership pattern which simply disallow such renounce of ownership in contracts.

This issue is identified as Minor severity because owner is setting the new_owner but this issue will create a severe damage to functionality of relayers. This issue also highlights the inconsistent ownership transfer pattern across ink! contracts, Therefore, this not purely centralize issue if the impact of this issue is deeply understand then its a major issue which can result in redeployment of both Advisory and relayers contracts so it should be judged as Minor severity.

Recommendation to fix\ To prevent stopping of relayers functionality, disable renounce of ownership.

+         const ZERO_ADDRESS: [u8; 32] = [0; 32];

    . . .  some code

    #[derive(Debug, PartialEq, Eq, Encode, Decode)]
    #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
    pub enum AdvisoryError {
        /// Error when calling a function from contracts environment
        InkEnvError(String),
        /// Caller is not an owner
        NotOwner(AccountId),
+      /// Error when setting zero address
+      ZeroAddress
    }

    . . .  some code

        #[ink(message)]
        pub fn transfer_ownership(&mut self, new_owner: AccountId) -> Result<(), AdvisoryError> {
            self.ensure_owner()?;
+            if new_owner == ZERO_ADDRESS {
+                return Err(AdvisoryError::ZeroAddress);
+            }
            self.env().emit_event(OwnershipTransferred {
                previous_owner: self.owner,
                new_owner,
            });
            self.owner = new_owner;
            Ok(())
        }

[krzysztofziobro]

This assumes owner error - out of scope

[0xRizwan]

@krzysztofziobro @fbielejec

Thanks for the comment. I would request the reconsideration of this issue.

Due to integration with relayers, if the issue happens then the impact is severe. Most of ink! contracts has used 2 step ownership pattern which simply don't allow renounce of ownership in contracts but in Advisory contract ownership transfer pattern is one step.

This is not consistent with other ink! contracts as far as transfer of ownership is concerned. This is reason the issue submitted as only Advisory allows to send ownership to zero address whereas other contracts can not do it.

The issue can be prevented by two ways- 1) Recommendation given as above. 2) Implement 2 step ownership transfer pattern in Advisory contract similar to as done in other contract.

Given that only owner can access the transfer_ownership() function, the issue is identified as Minor severity based on contest rules.

Attacks not mentioned in higher categories, but having a negative impact on user experience, or putting one of the contracts in an unexpected state.

The issue would have been Medium severity if owner was not there but still the issue deserves minor severity.