The contract doesn’t validate permissions correctly when adding new pair

[hats-bug-reporter[bot]]

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0x2f8b36d57c2774b4d81c659d3d21e8a6d6e0047cb033f2de5d95068a1f13685f Severity: medium

Description: Description\

The contract doesn't make sure it has permission to burn tokens and checks only if it's able to mint them.

Attack Scenario\

In the lib.rs, when adding new pair, it checks whether MOST has permission to mint in this line:

https://github.com/Cardinal-Cryptography/most/blob/70ab234cc3322fda82784413f5e0704907a0e1fe/azero/contracts/most/lib.rs#L671-672

 let psp22_address: AccountId = from.into();
            let psp22: ink::contract_ref!(Mintable) = psp22_address.into();

However, it's instead should make sure that it can burn the from token as it's burnt on the source chain initially:

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

 self.burn_from(src_token_address.into(), sender, amount)?;

Attachments

Provided above.

[rodiontr]

reassigned #16 to the medium severity. Basically, any token that implements "mintable" and doesn't implement "burnable" will cause tx reverts when send_request() is called as burn_from() is used to burn the tokens on the user's behalf

[krzysztofziobro]

Out of scope: "Attacks that require special conditions: malicious tokens, mistakes in governance actions, unless specifically allowed in impact categories." - no issue if most's token is used

[rodiontr]

Out of scope: "Attacks that require special conditions: malicious tokens, mistakes in governance actions, unless specifically allowed in impact categories." - no issue if most's token is used

Sorry but that’s not what the issue says. It basically the mistake in validating the token when adding the pair. It’s not about malicious token, it’s about token that doesn’t implement burnable functionality. So if such token is added, all the tx when interacting with most will revert as the contract only checks if the token implements mintable functionality.

[krzysztofziobro]

Yes, but whitelisting such a token would be a mistake on the owner's side.

[rodiontr]

Yes, but whitelisting such a token would be a mistake on the owner's side.

What’s the sense then checking for minting implementation?

[rodiontr]

Yes, but whitelisting such a token would be a mistake on the owner's side.

Ok i agree with this issue even though I think it’s still maybe low / minor. But please review again #27