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]: Cannot handle URI Scheme with Metadata, Value and Bytecode, ERC-681 #8308

Closed
Tracked by #11952 ...
0xPuddi opened this issue Jan 17, 2024 · 8 comments
Closed
Tracked by #11952 ...
Labels
external-contributor Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations Push issues to confirmations team type-bug Something isn't working

Comments

@0xPuddi
Copy link

0xPuddi commented Jan 17, 2024

Describe the bug

The QR can be generated with any library you wish, the Deep Link String is:

ethereum:0x4758aCF2d393A0f011C603eAfC9B4769322b2a94@43113/BetGame?bytes32=0x3863e63da67d85dcffede3cace2ebcbb39e67df5f6fdaf6437319cbe68cc3b94&bytes32=0x0fbf1ff35ca8785e1812f7745b1332b4b9b3c64ccd769f19869170beb4e4f513&value=1.0e15

The contract is deployed (0x4758aCF2d393A0f011C603eAfC9B4769322b2a94) on the Avalanche Fuji Testnet (ID: 43113) and it has a BetGame function that is payable and that takes two bytes32 as input. Function definition:

/**
* BetGame accetps and registers a player's game bet
*/
function BetGame(bytes32 gameId, bytes32 playerId) external payable minBet returns (bool) {
address caller = _msgSender();
uint256 betAmount = msg.value;

    // if firts game make him start it
    if (!getActiveGame(gameId)) {
        emit GameStarted(gameId, playerId, caller, betAmount, block.timestamp);
    }

    // Already in
    if (playerAddress[playerId] != address(0)) {
        revert BetAlreadyPlaced(caller, playerGameBalance[playerId][caller]);
    }

    gamePlayersIds[gameId].push(playerId);
    gameBalance[gameId] += betAmount;
    playerAddress[playerId] = caller;
    playerGameBalance[playerId][caller] = betAmount;
    emit PlayerBet(gameId, playerId, caller, betAmount, block.timestamp);
    return true;
}

/**
 * getActiveGame reutrns true is a game is active
 */
function getActiveGame(bytes32 gameId) public view returns (bool) {
    if (gamePlayersIds[gameId].length > 0) {
        return true;
    }

    return false;
}

The MetaMask Wallet reads the URL, and creates a transaction, yet the gas is miscalculated. Miscalculation of gas is probably a consequence that when you try to send the transaction it fails.

The error is a "Transaction error Internal JSON-RPC error." I tried changing also the RPC endpoint multiple times with different providers and I get the same error. Sending native coin around works.

If it can help: The value to send is correct, the chain is correct, the only problem there is is that the calldata with the function signature and input data is wrong, or not even there at all.

Expected behavior

  1. Read the QR Code

  2. Generate a transaction:

On the cainId: 43113
To: 0x4758aCF2d393A0f011C603eAfC9B4769322b2a94
With function signature: BetGame
With Input data: bytes32=0x3863e63da67d85dcffede3cace2ebcbb39e67df5f6fdaf6437319cbe68cc3b94 and bytes32=0x0fbf1ff35ca8785e1812f7745b1332b4b9b3c64ccd769f19869170beb4e4f513
And value sent of 1.0e15

  1. Send the transaction and notify on confirmation.

Screenshots/Recordings

No response

Steps to reproduce

Use the Deep Link string:

ethereum:0x4758aCF2d393A0f011C603eAfC9B4769322b2a94@43113/BetGame?bytes32=0x3863e63da67d85dcffede3cace2ebcbb39e67df5f6fdaf6437319cbe68cc3b94&bytes32=0x0fbf1ff35ca8785e1812f7745b1332b4b9b3c64ccd769f19869170beb4e4f513&value=1.0e15

Choose your favourite QR code generation method (Attached a QR code generated from Brave Browser with QR code feature. The String is the Deep Link string just mentioned above
33ca327c520c47e0a0ca681fa0bb0d41720e35cc
).

Have some gas on Avalanche Fuji Testner.

Generate the QR.

Scan the QR with MetaMask Mobile.

Error messages or log output

Internal JSON-RCP Error popup.

Version

MetaMask v7.12.3 (1230)

Build type

None

Device

Iphone XR

Operating system

iOS

Additional context

No response

Severity

Critical to all MetaMask users and developers.

The problem is that some apps functionality might not work as expected, since the ERC is finalised and well accepted developers should be able to interact using the standard without any problems or drawbacks.

@0xPuddi 0xPuddi added the type-bug Something isn't working label Jan 17, 2024
@DanielTech21 DanielTech21 added Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Jan 18, 2024
@DanielTech21
Copy link

Hi @Puddi1
Thanks for bringing this to our attention. I have assigned this to the right team to investigate it more.

@bschorchit bschorchit added team-sdk SDK team and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Feb 7, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Feb 19, 2024
@christopherferreira9
Copy link
Contributor

Hi @0xPuddi ! This has just caught our attention, can you please check if the issue persists with the latest version of the MetaMask mobile wallet?

@0xPuddi
Copy link
Author

0xPuddi commented Apr 12, 2024

Hey @christopherferreira9 thanks for your help and time. It is yes working, meaning that it identifies the contract, the chain and the value amount to send and there is no RPC-ERROR. Yet it is not including any data bytes, meaning that if you need to make a contract call you won't be able to do so, as it is able to send just eth, and if there is no eth (value) to send over (think about making a contract call without ether) it will prompt you to add some before sending the transaction, like if it was trying to make a native send.

In the example ethereum:0x4758aCF2d393A0f011C603eAfC9B4769322b2a94@43113/BetGame?bytes32=0x3863e63da67d85dcffede3cace2ebcbb39e67df5f6fdaf6437319cbe68cc3b94&bytes32=0x0fbf1ff35ca8785e1812f7745b1332b4b9b3c64ccd769f19869170beb4e4f513&value=1.0e15

It is like it's getting this right: ethereum:0x4758aCF2d393A0f011C603eAfC9B4769322b2a94@43113
this wrong (missing): /BetGamebytes32=0x3863e63da67d85dcffede3cace2ebcbb39e67df5f6fdaf6437319cbe68cc3b94&bytes32=0x0fbf1ff35ca8785e1812f7745b1332b4b9b3c64ccd769f19869170beb4e4f513
this right: &value=1.0e15

For any parsing question refer to the approved EIP standard

Thank you!

@christopherferreira9
Copy link
Contributor

Hi @gauthierpetetin ! What should be the responsible team to handle this topic since the deeplink part is being properly handled?
cc: @andreahaku

@bschorchit bschorchit added team-transactions Transactions team and removed team-sdk SDK team labels May 16, 2024
@gauthierpetetin
Copy link
Contributor

Hi @christopherferreira9 , bschorchit reviewed it and assigned it to Transactions team.

@forest-diggs-consensys
Copy link
Contributor

Hey @bschorchit - could you give some context on why this falls under Transactions?

@bschorchit
Copy link

Hey @forest-diggs-consensys, based on the original report and since @christopherferreira9 mentioned that the deeplink part is working correctly, it seems that the problem is around how we're calculating gas for the transaction proposed through the deeplink or even how we're not persisting the correct calldata for it.

The MetaMask Wallet reads the URL, and creates a transaction, yet the gas is miscalculated.
The value to send is correct, the chain is correct, the only problem there is is that the calldata with the function signature and input data is wrong, or not even there at all.

@dbrans dbrans added team-confirmations Push issues to confirmations team and removed team-transactions Transactions team labels Jul 30, 2024
@github-actions github-actions bot added the stale Issues that have not had activity in the last 90 days label Oct 28, 2024
@gauthierpetetin gauthierpetetin removed the stale Issues that have not had activity in the last 90 days label Oct 28, 2024
@MetaMask MetaMask deleted a comment from github-actions bot Dec 4, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 13, 2024
## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**


https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

---------

Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
runway-github bot added a commit that referenced this issue Dec 13, 2024
## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**


https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

---------

Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
runway-github bot added a commit that referenced this issue Dec 13, 2024
## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7

## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980
- [x] #12008
- [x] #11978

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**

https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

---------

Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
runway-github bot pushed a commit that referenced this issue Dec 13, 2024
)

## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**


https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

---------

Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
sethkfman added a commit that referenced this issue Dec 13, 2024
)

- fix: replace legacy eth-json-rpc deps (#11952)

## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**



https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

---------

Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
[d967a76](d967a76)

Co-authored-by: legobeat <[email protected]>
Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
@NicolasMassart
Copy link
Contributor

NicolasMassart commented Jan 24, 2025

Fixed in latest release

@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Jan 24, 2025
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations Push issues to confirmations team type-bug Something isn't working
Projects
Archived in project
Status: Fixed
Development

No branches or pull requests

9 participants