pair/lib.rs : `remove_liquidity` is using incorrect token address to transfer the liquidity tokens.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xb9811c25bdf84fb491abe51a4f0bf2f4cab7c1139a3f7a159f47b116f9ef376a Severity: high

Description:

Vulnerability Report

Description

while removing the liquidity, incorrect token address is used to transfer the token from user to the pair contract.

Attack Scenario

The actual liqudity will not be removed. after adding the liqudity, the attacker can call remove liqudity function continously and withdraw their deposits.

also, this might lead to situation where the liquiday can not be removed.

Attachments

1. Proof of Concept (PoC) File

https://github.com/Cardinal-Cryptography/common-amm/blob/bf4e48e3257894dcc8e6ab359321d1406533ad8b/amm/contracts/router/lib.rs#L297-L329

fn remove_liquidity(
            &mut self,
            token_0: AccountId,
            token_1: AccountId,
            liquidity: u128,
            amount_0_min: u128,
            amount_1_min: u128,
            to: AccountId,
            deadline: u64,
        ) -> Result<(u128, u128), RouterError> {
            self.check_timestamp(deadline)?;
            ensure!(token_0 != token_1, RouterError::IdenticalAddresses);
            let pair_contract = self.get_pair(token_0, token_1)?;

            psp22_transfer_from(pair_contract, self.env().caller(), pair_contract, liquidity)?; ------>>> audit find -- refer ``the token`` and ``to`` address both are same

            let mut pair: contract_ref!(Pair) = pair_contract.into();

            let (amount_0, amount_1) =
                pair.call_mut().burn(to).try_invoke().map_err(|_| {
                    RouterError::CrossContractCallFailed(String::from("Pair:burn"))
                })???;
            let (amount_0, amount_1) = if token_0 < token_1 {
                (amount_0, amount_1)
            } else {
                (amount_1, amount_0)
            };

            ensure!(amount_0 >= amount_0_min, RouterError::InsufficientAmountA);
            ensure!(amount_1 >= amount_1_min, RouterError::InsufficientAmountB);

            Ok((amount_0, amount_1))
        }

2. Revised Code File (Optional)

Update the following line of codes

https://github.com/Cardinal-Cryptography/common-amm/blob/bf4e48e3257894dcc8e6ab359321d1406533ad8b/amm/contracts/router/lib.rs#L311

psp22_transfer_from(pair_contract, self.env().caller(), pair_contract, liquidity)?;

here for token use the psp22 which is the token that is minted when adding the liquidity.

[deuszx]

Please provide a PoC reproducing the issue. While doing it, I think you will realise this isn't the case.

psp22_transfer_from takes the following arguments:

• token for which the transfer is done
• source of the transfer
• recipient of the transfer

here we do the following:

• transfer LP tokens for the specific Pair
• from the caller's account
• to the Pair contract

These are LP tokens which are being burnt for the underlying tokens.

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue is INVALID.

We hope you participate in the future audits of ink!.