`send_request()` will always revert due to insufficient access validation

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

Description: Description\ In ink Most contract, send_request() is used to initiate funds transfer to the destination chain. Here the function caller will give allowance to his psp22 tokens which will burn by Most contract so that on destination chain, the caller can get his tokens thereby fulfilling this briding process.

Let's see how this looks in function,

        #[ink(message, payable)]
        pub fn send_request(
            &mut self,
            src_token_address: [u8; 32],
            amount: u128,
            dest_receiver_address: [u8; 32],
        ) -> Result<(), MostError> {

    . . . some code

            // burn the psp22 tokens
            self.burn_from(src_token_address.into(), sender, amount)?;

    . . . some code

}

See, how burn of user provided psp22 tokens looks as:


        fn burn_from(
            &self,
            token: AccountId,
            from: AccountId,
            amount: u128,
        ) -> Result<(), PSP22Error> {
            let mut psp22: ink::contract_ref!(PSP22) = token.into();
            psp22.transfer_from(from, self.env().account_id(), amount, vec![])?;

            let mut psp22_burnable: ink::contract_ref!(Burnable) = token.into();
@>          psp22_burnable.burn(amount)
        }

This means that, when the user will call send_request(), he will give his psp22 tokens as allowance to Most contract and then Most contract will call psp22_burnable.burn(amount) to burn the tokens as a part of bridging process so that on destination chain user can get his tokens.

The issue is that, Most contract does not check whether it has psp22 token burn access or not. It should be noted that, while adding the pair, the psp22 tokens minting access to Most contract is checked and this can be checked here

To get the issue, why revert will happen with Most contract calling psp22_burnable.burn(amount). This is because psp22 tokens burn function has burner access control which is given by psp22 token contract owner and this looks as:

https://github.com/hats-finance/Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d/blob/70ab234cc3322fda82784413f5e0704907a0e1fe/azero/contracts/token/lib.rs#L245-L258

POC: 1) Bob calls send_request() on ink! Most contract to bridge his tokens by providing his psp22 tokens to Most contract as allowance.

2) As shown above, Most contract will try to burn the psp22 tokens but it will revert as the Most contract does not have access to burn the psp tokens.

3) The transaction initiated by Bob will revert in middle as the function will throw error as:

                Err(PSP22Error::Custom(String::from(
                    "Caller has to be the burner.",
                )))

This is because Most contract does not have burn function access.

4) The transaction by Bob will only be successful when the psp22 owner grant Most contract burn function access.

This issue is identified as Medium severity due to below major impacts: 1) Denial of service until the owner grants Most contract burn function access. 2) The send_request() will always revert therefore the bridging won't work as no burning of psp22 token wont unlock tokens on destination chain. 3) This breaks the intended bridging process as designed for Most contract.

Therefore, its always need to be sure the Most contract has burn function access while adding the pair to contract. In case, the psp22 tokens` dont have burner access then then function will revert while adding pair itself so that such whole issue could not happen.

Recommendation to fix\ Similar to how psp22 minting access to Most contract is checked, check burn access so that the functions should not revert and bridging should work as designed.

Make the following changes to Most ink! contract


    #[derive(Debug, PartialEq, Eq, Encode, Decode)]
    #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
    pub enum MostError {
        Constructor,
        InvalidThreshold,
        DuplicateCommitteeMember,
        ZeroTransferAmount,
        NotInCommittee,
        NoSuchCommittee,
        HashDoesNotMatchData,
        PSP22(PSP22Error),
        Ownable(Ownable2StepError),
        UnsupportedPair,
        InkEnvError(String),
        RequestAlreadySigned,
        BaseFeeTooLow,
        Arithmetic,
        CorruptedStorage,
        IsHalted,
        HaltRequired,
        NoMintPermission,
+      NoBurnPermission,
        ZeroAddress,
    }

    . . . some code

        #[ink(message)]
        pub fn add_pair(&mut self, from: [u8; 32], to: [u8; 32]) -> Result<(), MostError> {
            self.ensure_owner()?;
            self.ensure_halted()?;

            // Check if MOST has mint permission to the PSP22 token
            let psp22_address: AccountId = from.into();

            let psp22: ink::contract_ref!(Mintable) = psp22_address.into();
            if psp22.minter() != self.env().account_id() {
                return Err(MostError::NoMintPermission);
            }

+          let psp22: ink::contract_ref!(Burnable) = psp22_address.into();
+          if psp22.burner() != self.env().account_id() {
+              return Err(MostError::NoBurnPermission);
+           }

            self.supported_pairs.insert(from, &to);
            Ok(())
        }

    . . . some code

}

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

`send_request()` will always revert due to insufficient access validation #48