1) Appending bytes to tx payload in data field scrambles e.g. displayed ERC20 transfer value & using sendTransaction to transfer ERC20s results in contract deployment

[hilmarx]

Describe the bug:

When sending transactions on Ethereum, e.g. an ERC transfer, you can append bytes to the encoded transfer payload. This is used by dApps to enrich the information of certain transactions with additional data. In our case, it is used to later verify on-chain that certain data which was appended to a regular ERC20 transfer was indeed signed by the user or not.

It can look something like this:

const erc20Interface = new ethers.utils.Interface(["function transfer(address recipient, uint256 amount)"])

const encodedData = erc20Interface.encodeFunctionData("transfer", [receiver, inputAmount])

const appendedData = new ethers.utils.AbiCoder().encode(
        ["address", "uint256"],
        [toCurrency, minimumReturn]
);

const concatData = encodedData + appendedData.substr(2)

const res = await provider.getSigner().sendTransaction({
        data: concatData,
        to: fromCurrency,
        gasLimit: 100000,
})

On desktop using Metamask or any other wallet, this works fine. However on Metamask mobile, the extra data which was appended to the regular erc20 encoded data seems to completely mess with the transaction, leading to the amount to be transferred to be completely messed up (Very dangerous) and the to field being undefined, resulting in the transaction trying to deploy a contract.

Screenshots

How it should look like:

How it actually looks like:

=> Sending this transaction will result in a contract being deployed which fails.

To Reproduce
Check out this codebase of using sendTransaction with a regular erc20 transfer payload (without appended data):

https://github.com/gelatodigital/sorbet-finance/blob/288c42f1dbb786d510265b95aad0cff164a47607/src/components/ExchangePage/index.jsx#L617-L625

You can try it out here:
https://sorbet-finance-git-fix-metamask-mobile-bug-gelato.vercel.app

It works fine, however defeats the purpose of the dApp (we need to append some additional payload to the data)

Now, check out this codebase, where we do append additional payload to the data field:
https://github.com/gelatodigital/sorbet-finance/blob/fe8e198d5d2d4c32d0a838b6b9e696c5bd04e496/src/components/ExchangePage/index.jsx#L617-L630

You can try it out here:
https://sorbet-finance-git-fix-metamask-mobile-bug-2-gelato.vercel.app

There you will see the bug occurs. As mentioned before, on Web Metamask and using other Wallets such as Formatic, it works fine. This issue also occurs on Mainnet, not only Matic.

Smartphone

• Device: iPhoneX
• OS: iOS14.4
• App Version: v2.4.0

EDIT:

Replying to @MaartenWeber14 comment.

I just checked again and I think you are right, I am talking about two separate issues here.

Issue 1) If you append data to the e.g. transfer payload, the displayed values of how will be transferred get completely scrambled as seen in the screenshots of my original comment above.

Issue 2) If you use sendTransaction, even if you dont append any extra bytes to the payload, the to address is interpreted as undefined and a contract gets deployed instead of the to address being the ERC20 token.

[EerieEight]

Hey all! Half of our users come through MM mobile and this issue is directly affecting them, as well.

[MaartenWeber14]

@hilmarx are you sure this is caused by appending bytes to the encoded transfer payload? We are experiencing the same issue it seems. Whenever we try to transfer an amount, MM mobile says it's a transfer. But when you accept the tranfer, it tries to deploy a new contract. When inspecting the transaction, the payload is fine, but it seems the to address has been set to undefined indeed, resulting in a failed contract deployment.

However, we aren't appending anything to the payload (as far as I know). We also only have this issue on mobile. On web it works as it should. For reference, we're using WalletConnect to connect to MM and do the transfer.

Hope this thing can be fixed/cleared up soon, as MM is used by a lot of users.

[hilmarx]

@MaartenWeber14 thanks for your comment, you are right, I added an EDIT to my original comment above.

[Gazoh]

Is there any updates or workarounds?

[gitpusha]

Yeah would also be keen to get an update on this!

[ooGwei]

Still affected by this bug and appreciate any workarounds as it is directly preventing Mobile users from using a portion of a platform.

[DeepakMotamarri]

Appreciate any workarounds or updates on this issue

[hsnGh67]

It's very important issue. No updates yet?

[CodeXSky]

It still have this problem when send token erc20 through walletconnect service by web3js. Anyone have solution for that?

[quan32]

This bug is so serious, please update ASAP.
Anyone have any temporary solution for this?

[scrubotage]

Following up to see if this has been addressed yet. This is impacting countless users.

[1brahimsaid]

Hi this issue has been plaguing me for weeks, it would be great if we could solve this soon. For now I have to use my PC each time to do this same thing I enjoy on mobile.

[midas104]

This issue is caused by code that substitute value and to transaction properties by encoded transfer data(this one)

The easiest fix is to introduce case insensitive search here because contract map has vary case keys.

Possible fix

// `to` is lowercased on line #316
let asset = props.tokens.find(({ address }) => address.toLowerCase() === to);
if (!asset) {
  // try to lookup contract by lowercased address `to`
  asset = contractMap[Object.keys(contractMap).find(key => key.toLowerCase() === to) || ''];
  if (!asset) {
    try {
      asset = {};
      asset.decimals = await AssetsContractController.getTokenDecimals(to);
      asset.symbol = await AssetsContractController.getAssetSymbol(to);
      // adding `to` here as well

      asset.to = to;
    } catch (e) {
      // This could fail when requesting a transfer in other network
      // adding `to` here as well
      asset = { symbol: 'ERC20', decimals: new BN(0), to: to };
    }
  }
}

[midas104]

PS My dApp is affected when try to submit transaction via WalletConnect

[0xBebis]

facing same issue

[leon-do]

same issue. waiting for a solution.

[zmyqucai8]

This is a very serious problem why not solve it first？😅

[mobularay]

Treat this ticket to represent the 1st issue; @gantunesr will create a separate ticket for the 2nd issue. ]

Estimation for this ticket represents the 1st issue.

[LouiseCat]

same issue. waiting for a solution.

[ptorrent]

any news ?

[Yuaimin]

the problem is too fatal!

[llioor]

Big number issue? Community?
+1

[EtienneHelfer]

Any news ? this is a huge problem and should be easy to fix

[mobularay]

@hilmarx @MaartenWeber14

Thanks to your investigation and clarification with the last section of the ticket description:

• we are currently QA-ing the solution for Issue 2) and aim to roll out the solution next week (part of v3.2.0)
• we are currently implementing the solution for Issue 1) and aim to roll out the solution in 2 weeks, as per our sprint cadence (part of v3.3.0)

Issue 1) If you append data to the e.g. transfer payload, the displayed values of how will be transferred get completely scrambled as seen in the screenshots of my original comment above.

Issue 2) If you use sendTransaction, even if you dont append any extra bytes to the payload, the to address is interpreted as undefined and a contract gets deployed instead of the to address being the ERC20 token.

Thank you for your patience and understanding. Pls keep an eye for the version updates in the next 2-4 weeks, and let us know if you encounter these issues again after upgrading to these version(s) starting mid next week.

[hilmarx]

sweet!

[MaartenWeber14]

Thanks for the update! Looking forward to it.