Denial of service in `most/lib.rs::send_request` and `most/lib.rs::receive_request` when token owner change minter address after it has been added in the `supported_pairs`.

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x58face896519a15ce81a9b441084a6fe46ebb8048e4c79a5421a47784337bc86 Severity: medium

Description: Description

Denial of service in most/lib.rs::send_request and most/lib.rs::receive_request when token owner change minter address after it has been added in the supported_pairs.

Attack Scenario

In most/lib.rs::add_pair:

#[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);
            }

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

This code will check whether the most is the minter role in the from token before adding to the supported_pairs. However, nothing stops the token owner from modifying the minter address in the future by calling token/lib.rs::set_minter_burner function.

As a result, token owner can cause denial of service in most/lib.rs::send_request and most/lib.rs::receive_request functions as these two functions use burn_from & mint_to functions respectively which requires minter_burner role.

A proof of concept cargo test is attached as follow to showcase the issue.

Attachments

NA

1. Proof of Concept (PoC) File

add the following content in drink-tests/src/utils.rs under pub mod token {:

pub fn set_minter_burner(
        session: &mut Session,
        token: &Token,
        account: AccountId,
        caller: drink::AccountId32,
    ) -> Result<(), token::PSP22Error> {
        let _ = session.set_actor(caller);

        handle_ink_error(
            session
                .execute(Token::set_minter_burner(token,account))
                .unwrap(),
        )
    }

add the following content under the outdated_oracle_price test in drink-tests/src/tests.rs:

#[drink::test]
fn denial_of_service_when_reset_minter(mut session: Session) {
    mint_to_default_accounts(&mut session);
    let most = most::setup(
        &mut session,
        guardian_accounts(),
        DEFAULT_THRESHOLD,
        POCKET_MONEY,
        RELAY_GAS_USAGE,
        MIN_GAS_PRICE,
        MAX_GAS_PRICE,
        DEFAULT_GAS_PRICE,
        None,
        owner(),
        BOB,
    );
    let token = token::setup(&mut session, "TestToken".to_string(), most.into(), BOB);

    let token_address: ink_primitives::AccountId = token.into();
    most::add_pair(
        &mut session,
        &most,
        *token_address.as_ref(),
        REMOTE_TOKEN,
        OWNER,
    )
    .expect("Add pair should succeed");

    most::set_halted(&mut session, &most, false, OWNER).expect("Unhalt should succeed");
    token::transfer(&mut session, &token, most.into(), 1000, BOB).expect("Transfer should succeed");

    //BOB decides to reset minter to other address, causing most contract to lose minter role.
    let result = token::set_minter_burner(&mut session, &token,alice(),BOB);
    assert_eq!(result, Ok(()));

    let alice_balance_before = token::balance_of(&mut session, &token, alice());

    let committee_id: u128 = 0;
    let amount: u128 = 100;
    let nonce: u128 = 1;

    let request_hash = hash_request_data(committee_id, token_address, amount, alice(), nonce);

    //erictee: most contract no longer the minter for the token, therefore all the receive_request calls will revert.
    GUARDIANS
        .iter()
        .take(DEFAULT_THRESHOLD as usize)
        .for_each(|guardian| {
            let result = most::receive_request(
                &mut session,
                &most,
                request_hash,
                committee_id,
                *token_address.as_ref(),
                amount,
                *alice().as_ref(),
                nonce,
                guardian.clone(),
            );

            assert_eq!(result, Ok(()));

        });

Inside /most/azero directory, run npm run drink-tests.

Result:

mac@macs-MacBook-Pro azero % npm run drink-tests

> most-bridge@1.0.0 predrink-tests
> npm config set script-shell "/bin/bash"; ./scripts/predrink_tests.sh

Wrapping advisory
Wrapping most
Wrapping token
Wrapping migrations
Wrapping gas price oracle
Copying contract gas price oracle
Copying contract for advisory
Copying contract for most
Copying contract for token
Copying contract for migrations

> most-bridge@1.0.0 drink-tests
> npm config set script-shell "/bin/bash"; ./scripts/drink_tests.sh

Running drink tests: /Users/mac/Documents/Defi/Hats.Finance/most/azero/scripts
warning: unused variable: `alice_balance_before`
   --> src/tests.rs:245:9
    |
245 |     let alice_balance_before = token::balance_of(&mut session, &token, alice());
    |         ^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_alice_balance_before`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: `most-tests-drink` (lib test) generated 1 warning (run `cargo fix --lib -p most-tests-drink --tests` to apply 1 suggestion)
    Finished test [unoptimized + debuginfo] target(s) in 0.67s
     Running unittests src/lib.rs (target/debug/deps/most_tests_drink-2c086f47403c6735)

running 6 tests
test tests::no_duplicate_guardians_allowed ... ok
test tests::most_needs_to_be_token_minter_to_add_pair ... ok
test tests::outdated_oracle_price ... ok
test tests::no_zero_amount_allowed ... ok
thread 'tests::denial_of_service_when_reset_minter' panicked at src/tests.rs:270:13:
assertion `left == right` failed
  left: Err(PSP22(Custom("Caller has to be the minter.")))
 right: Ok(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::denial_of_service_when_reset_minter ... FAILED
test tests::correct_receive_request ... ok

failures:

failures:
    tests::denial_of_service_when_reset_minter

test result: FAILED. 5 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.42s

error: test failed, to rerun pass `--lib`
mac@macs-MacBook-Pro azero % 

Noticed that the test denial_of_service_when_reset_minter failed in the very last line assert_eq!(result, Ok(()));.

With actual result being Err(PSP22(Custom("Caller has to be the minter."))), this indicates denial of service in most::receive_request function.

2. Revised Code File (Optional)

[krzysztofziobro]

Protocol operates under the assumption that the initial owner of the tokens on azero side is the same as owner of the Most contract, so this would require malicious owner/owner error.