[Bug]: Metamask removes any data appended to an ERC20 approval transaction data

georgeroman commented 1 year ago

Describe the bug

Any data which is appended to the calldata of an ERC20 transaction (the standard function approve(address owner, uint256 amount)) will be removed by Metamask.

Steps to reproduce

1.

Error messages or log output

No response

Version

10.34.0

Build type

None

Browser

Firefox

Operating system

Linux

Hardware wallet

No response

Additional context

No response

anaamolnar commented 1 year ago

Hello, @georgeroman. Thanks for reporting! Before I pass this along to the team, could you please provide more information about the issue? Thank you!

georgeroman commented 1 year ago

Sure, the steps to reproduce are the following:

Trigger an approval transaction with some additional data appended at the end of the calldata.

For example for this transaction:

{
  "from": "0x8B5E4dB198FfC7f69f8F11F6592f682717dF1D92",
  "to": "0xB4FBF271143F4FBf7B91A5ded31805e42b2208d6",
  "data": "0x095ea7b300000000000000000000000069f2888491ea07bb10936aa110a5e0481122efd300000000000000000000000000000000000000000000000000038d7ea4c680000000000000000000000000008b5e4db198ffc7f69f8f11f6592f682717df1d920000000000000000000000000000000000000000000000000000000000000000000000000000000000000000b4fbf271143f4fbf7b91a5ded31805e42b2208d600000000000000000000000007865c6e87b9f70255377e024ace6630c1eaa37f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000064db2ff000000000000000000000000000000000000000000000000000038d7ea4c680000000000000000000000000000000000000000000000000000000000bd7e8988d0000000000000000000000000000000000000000000000000000000bd7e8988d0000000000000000000000000000000000000000000000000000000bd7e8988d00000000000000000000000000000000000000000000000000000000000001a00000000000000000000000000000000000000000000000000000000000000041125a180435391a91adf1b0bc0fb449157b3408b9099b3865406130bb901e1e6700a3bc5b96b9226e810cc65b7deab994ede4127bb7fa411d4de873ea09c20a991b00000000000000000000000000000000000000000000000000000000000000"
}

The actual approval calldata is: 0x095ea7b300000000000000000000000069f2888491ea07bb10936aa110a5e0481122efd300000000000000000000000000000000000000000000000000038d7ea4c6800000

While the rest was added on top: 0x00000000000000000000008b5e4db198ffc7f69f8f11f6592f682717df1d920000000000000000000000000000000000000000000000000000000000000000000000000000000000000000b4fbf271143f4fbf7b91a5ded31805e42b2208d600000000000000000000000007865c6e87b9f70255377e024ace6630c1eaa37f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000064db2ff000000000000000000000000000000000000000000000000000038d7ea4c680000000000000000000000000000000000000000000000000000000000bd7e8988d0000000000000000000000000000000000000000000000000000000bd7e8988d0000000000000000000000000000000000000000000000000000000bd7e8988d00000000000000000000000000000000000000000000000000000000000001a00000000000000000000000000000000000000000000000000000000000000041125a180435391a91adf1b0bc0fb449157b3408b9099b3865406130bb901e1e6700a3bc5b96b9226e810cc65b7deab994ede4127bb7fa411d4de873ea09c20a991b00000000000000000000000000000000000000000000000000000000000000

Any additional data added at the end will get ignored and the transaction that Metamask triggers will only contain the first part of the calldata, ignoring anything that was added at the end:

{
  "from": "0x8B5E4dB198FfC7f69f8F11F6592f682717dF1D92",
  "to": "0xB4FBF271143F4FBf7B91A5ded31805e42b2208d6",
  "data": "0x095ea7b300000000000000000000000069f2888491ea07bb10936aa110a5e0481122efd300000000000000000000000000000000000000000000000000038d7ea4c6800000"
}

This issue only seem to be relevant for ERC20 approvals (for which Metamask seems to do custom handling/parsing). Ideally, the format of the transaction gets preserved, and data added at the end doesn't get stripped off.

bschorchit commented 1 year ago

Hey @georgeroman, thank you for your report! Could you share more on the use case for appending this extra data? This will allow us to prioritize accordingly. Thank you!

For organization purposes, I've labeled this as a feature request instead of as a bug.

georgeroman commented 1 year ago

@bschorchit Thanks for looking into it. There's a few use-cases for this:

source attribution for easily determining the app that prompted the user to trigger the transaction (see https://twitter.com/stephanminkj/status/1579529197784993792 for more details)
using the mempool as an orderbook (in a lot of cases the ERC20 approvals are done as a prerequisite of creating various orders / intents - while these orders live off-chain it would be very cool to attach them ABI-encoded to the approval transaction so that they're shared in a decentralized manner at a minimal gas cost)

MetaMask / metamask-extension