Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Approve button disabled for Approvals of ERC20 tokens #10834

Closed
marctatham opened this issue Aug 27, 2024 · 18 comments · Fixed by #11532
Closed

[Bug]: Approve button disabled for Approvals of ERC20 tokens #10834

marctatham opened this issue Aug 27, 2024 · 18 comments · Fixed by #11532
Labels
external-contributor needs-reproduction regression-prod-7.28.1 release-7.34.0 Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-assets type-bug Something isn't working

Comments

@marctatham
Copy link

Describe the bug

This issue was already opened here: #6654
It has since been closed, however it is clearly still an issue

Behaviour:

  • Approvals are impossible to complete with the Metamask mobile application due to the Approval button being disabled.
  • The same wallet being used via the metamask browser extension is able to complete the Approval just fine.

Expected behavior

I should be able to approve the transaction, however the approve button is disabled 🙅

Screenshots/Recordings

Recording:
https://github.com/user-attachments/assets/7d2cb0f7-b346-4111-a59c-8572d6047b04

Steps to reproduce

  • connect to UniSwap (i did so using WalletConnect, may or may not be relevant)
  • initiate a transaction that requires Approval (first time you're interacting with a token, you can revoke previous approvals via something like revoke.cash)
  • receive the eth_sendTransaction request to perform the approval

Error messages or log output

Approve button is disabled

Detection stage

In production (default)

Version

7.28.1

Build type

None

Device

Pixel 6, OS 15

Operating system

Android

Additional context

As this is expected behaviour the first time you're interacting with a token via the uniswap protocol, it means this will affect ALL users and completely block them from being able to use this sort of service.

In my opinion, this is High in terms of severity.

Severity

No response

@marctatham marctatham added the type-bug Something isn't working label Aug 27, 2024
@bschorchit bschorchit added the team-confirmations Push issues to confirmations team label Aug 27, 2024
@bschorchit bschorchit changed the title [Bug]: Approve button disabled for Approvals of ERC721 tokens [Bug]: Approve button disabled for Approvals of ERC20 tokens Aug 27, 2024
@bschorchit bschorchit added Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking needs-reproduction labels Aug 27, 2024
@jpuri jpuri self-assigned this Sep 5, 2024
@jpuri
Copy link
Contributor

jpuri commented Sep 6, 2024

I tried this out in android emulator and could not replicate:

Screen.Recording.2024-09-06.at.5.56.12.PM.mov

@marctatham
Copy link
Author

So @jpuri , i think i've discovered the cause. I've reproduced it with your test dApp: https://metamask.github.io/test-dapp/

  • when my Metamask wallet has the ethereum network selected, it works ✅ (though i don't have sufficient ETH in there to cover the gas fees)
  • when my Metamask wallet has the base network selected, it is always disabled

See new recording:
https://github.com/user-attachments/assets/75f4972d-c774-41e1-97e5-fc11d8ea6344

@sleepytanya
Copy link
Contributor

@marctatham
Thank you for reporting this! I'm trying to reproduce on physical devices. What I see that in some cases I see disabled 'Approve' button for a short time and the 'Next' button appears and I can proceed with approval. I wasn't able to reproduce the bug on Android.

Samsung S24+, Android 14, MetMask 7.29.2

Swap on Base:

approvePancakeAndroid.mp4

Malicious ERC20 approval from test dapp:

Uploading maliciousApprovalAndroid.mp4…

ERC20 token approval from test dapp

approvalAndroid.mp4

@sleepytanya
Copy link
Contributor

iPhone 15, iOS 17.5.1, MetaMask 7.29.0

Approvals from test dapp:

iosApprovalTestdapp.mov

Approval from Pancake swaps:

approvalIos.mp4

Here you can see disabled 'Approve' button for a short time:

disabledButton.mov

@marctatham
Copy link
Author

marctatham commented Sep 9, 2024

Fascinating... thank you for your effort in attempting reproducing @sleepytanya
What's interesting is using my reproduction steps I am able to reproduce this 10/10 (including new recording of transaction that requires approval via uniswap here: https://github.com/user-attachments/assets/6255c13c-9932-4736-8427-38eeb0a50a0c) Perhaps something to do with the state of my wallet 🤷 (worth noting that this is consistent even when I uninstall and re-import this same wallet)

I realise this is unconventional, but as this just is a one of our wallets we use for testing purposes and is very replaceable. If you are open to it, you can share your details with me and I would be open to sharing this wallet's privateKey with you to facilitate your debugging process.

I have a vested interest in seeing this one solved.

@sleepytanya
Copy link
Contributor

sleepytanya commented Sep 11, 2024

@marctatham @jpuri @bschorchit

Unfortunately, we're unable to share test wallet details, but we're equally keen on delving into this bug.
I think I can repro the bug: swap WETH to another token. When WETH is a destination token bug doesn't happen.

Uniswap, Base, in-app MetaMask browser, iPhone 15, iOS 17.5.1

USDC -> WETH - success

usdcWethBase.mp4

WETH -> 1Inch - Aprrove is disabled

weth1inchBase.mov

WETH <-> AXLUSDC - Approve is available only when WETH is destination token

wethAxlusdc.mp4

@marctatham
Copy link
Author

Amazing that you are able to reproduce!
If you need anything else from my side please don't hesitate to get in touch.

If you could be so kind as to let me know when I can hope to see this fix in a new version, that would be appreciated

@sleepytanya
Copy link
Contributor

@marctatham Sure! Will keep you updated.

@jpuri
Copy link
Contributor

jpuri commented Sep 13, 2024

Hey @sleepytanya : I tried the above steps, but its is working for me:

Screen.Recording.2024-09-13.at.5.10.33.PM.mov

@sleepytanya
Copy link
Contributor

@jpuri

WETH <-> ERC20 works fine on Ethereum, seems like the bug is specific to Base.

'Approve' works on mainnet:

Ethereum.mov

'Approve' disabled on Base:

base.mov

@jpuri
Copy link
Contributor

jpuri commented Sep 18, 2024

I could replicate this issue on Base Sepolia, I found that this is happening we try to interact a token not present on base for instance I was trying to approve 0x4Fabb145d64652a948d72533023f6E7A623C7C53 on base and I found this error and later found that token does not exist on base network.

In these cases error is originating in AssetsContractController and token details, balance are not obtained and this page is blank.

@sleepytanya
Copy link
Contributor

cbETH -> WETH on Base, 'Approval' works:

RPReplay_Final1726669208.MP4

@bschorchit
Copy link

I could replicate this issue on Base Sepolia, I found that this is happening we try to interact a token not present on base for instance I was trying to approve 0x4Fabb145d64652a948d72533023f6E7A623C7C53 on base and I found this error and later found that token does not exist on base network.

In these cases error is originating in AssetsContractController and token details, balance are not obtained and this page is blank.

in Tanya's case above, WETH should be present on Base, right?

It seems approving WETH on Base has this issue on mobile, but not on extension. Is there something we might be doing on extension that we're not on mobile?

@jpuri
Copy link
Contributor

jpuri commented Sep 19, 2024

Hey @bschorchit from @sleepytanya 's comment WETH transaction is workiing on mobile.

@jpuri
Copy link
Contributor

jpuri commented Sep 24, 2024

The issue is caused by rate limiting in Base provider which causes query for user balance and asset details to fail.

@marctatham
Copy link
Author

@jpuri what strikes me as key information is that

  • this transaction (swapping WETH for 1INCH on Base as per my initial video, and my subsequent try that exhibits the same behaviour, which is consistent)
  • using this wallet
  • using the Metamask mobile application
    Fails.

But performing the exact same transaction, using the exact same wallet but using the metamask chrome extension instead of the mobile app works without issue 🤔

@jpuri
Copy link
Contributor

jpuri commented Sep 24, 2024

@marctatham : yep that is valid point why its breaking in mobile only. We will check this further.

@cryptotavares cryptotavares removed the team-confirmations Push issues to confirmations team label Sep 24, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 10, 2024
## **Description**

PR to fix the approve flow when users swap tokens.

## **Related issues**

Fixes: #10834

## **Manual testing steps**

1. Switch to base network
2. Swap WETH against any other token, exp USDC
3. See approval screen; it should have the buttons enabled and you
should be able to see token icon and spending cap

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->


https://github.com/user-attachments/assets/d8be6306-be74-4c6c-8463-567f35ee891e


### **After**

<!-- [screenshots/recordings] -->


https://github.com/user-attachments/assets/8382cc83-e4e7-42ce-b913-fcac7e906d4b


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@marctatham
Copy link
Author

Thank you to all of you involved in looking into this issue, it is appreciated 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor needs-reproduction regression-prod-7.28.1 release-7.34.0 Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-assets type-bug Something isn't working
Projects
Archived in project
Status: Fixed
Development

Successfully merging a pull request may close this issue.

7 participants