hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

Advisory contract is meant to follow 2step ownership but doesn't do it #20

Open hats-bug-reporter[bot] opened 7 months ago

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

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0xcf2ab635e555c7bfccdec1e15860a250eed521cafdfe984f2ca75b29a7f22a1a Severity: low

Description: Description\

Advisory smart contract is meant to follow ownable2step but doesn't use it at all. It has only transfer_ownership() function but there is no setting pending owner and the accept_ownership() function meaning that the contract is prone to a situation where the owner puts incorrect address and the ownership over the contract is lost. The fact that the contract handles emergency situations and basically sets an emergency adds additional severity to the issue.

Attack Scenario\

https://github.com/Cardinal-Cryptography/most/blob/70ab234cc3322fda82784413f5e0704907a0e1fe/azero/contracts/advisory/lib.rs#L72-81

  #[ink(message)]
        pub fn transfer_ownership(&mut self, new_owner: AccountId) -> Result<(), AdvisoryError> {
            self.ensure_owner()?;
            self.env().emit_event(OwnershipTransferred {
                previous_owner: self.owner,
                new_owner,
            });
            self.owner = new_owner;
            Ok(())
        }https://github.com/Cardinal-Cryptography/most/blob/70ab234cc3322fda82784413f5e0704907a0e1fe/azero/contracts/advisory/lib.rs#L72-81

As you can see, there is a transfer_ownership() function that directly transfers the ownership without following 2step pattern.

Recommendations

Add Ownable2Step implementation.

krzysztofziobro commented 6 months ago

Submission out of scope (design choices): https://app.hats.finance/audit-competitions/most-aleph-zero-bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d/scope#:~:text=Design%20choices%20such%20as%20committee%20rotation%20not%20being%202%2Dstep