Custom RPC incorrectly inserts chainId(4-rinkeby) eth_signTypedData_v3, signing tx affected.

[rstormsf]

Describe the bug

To Reproduce
Steps to reproduce the behavior:

1. Set Custom RPC URL to https://api.myetherwallet.com/eth
2. Add some extra custom RPC such as https://api.myetherwallet.com/rop
3. Go to https://rstormsf.github.io/js-eth-personal-sign-examples/
4. Click Connect
5. Click sign typed data v3
6. See error

AssertionError: Provided chainId (1) must match the active chainId (4)

Expected behavior
Custom RPC must not use hardcoded values

Browser details (please complete the following information):

• OS: [OSX]
• Browser [chrome]
• MetaMask Version [5.2.2]

Additional context

I have created PR to remove hardcoded chainId
danfinlay/js-eth-personal-sign-examples#7

I also wasn't able to send any transactions from custom RPC URLs, and when I click on Tx Hash, it redirects me to rinkeby.etherscan.io
I believe it's ignoring custom rpcs and reading from last network id from dropdown which is rinkeby

[rstormsf]

I'm not familiar with metamask code base, but I suspect
https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/controllers/network/network.js#L118

Basically when you do

ethQuery.sendAsync({ method: 'net_version' }, (err, network) => {
        log.info('web3.getNetwork returned ' + network)
        this.setNetworkState(network, type)
      }
    })

you are not getting network, instead, you are receiving object.
Also, in some RPC urls, you have to provide jsonrpc: "2.0" when using sendAsync
Example: https://api.myetherwallet.com/eth
I haven't ran the code, but I think there might be an issue

ethQuery.sendAsync({ method: 'net_version' }, (err, result) => {
        const network = result.result
        log.info('web3.getNetwork returned ' + network)
        this.setNetworkState(network, type)
      }
    })

@danfinlay

I personally learned on that PR, playing around with different type of params.

[danfinlay]

Good catch, thanks! I’m surprised this bug sat in prod for so long! I guess it shows signTypedData is finally getting some exploration? I’m definitely excited to see it combined with solidity 0.5.0 structured params.

I think your diagnosis looks right, but I’ll take a closer look and verify.

[bdresser]

#5934 makes it sound like it's an issue with applying custom RPC networks, not limited to signTypedData

[frankiebee]

missing repro step:

• ensure that you are switching from a non mainnet to custom rpc

bug notes:
seems to be a bad cached value in the provider for type: rpc

solution ensure the look up of chainId before _configureProvider

[frankiebee]

more notes:

Seems part of this issue is that what ever the "provider" inpage is returning for chain Id does not seem to update properly on network switches the simple hack of setting the chainId during networkController.lookupNetwork does not solve this issue and chainId is also persisted in preferences controller for some reason. and could possibly be causing some of these problems

[rstormsf]

@frankiebee that's what I tried to say at the end of the bug report that even sending ANY tx on custom RPCs doesn't work

[danjm]

@frankiebee @rstormsf I followed the repro steps (see gif below), but did not see the error. Is there an inconsistency between the steps I followed and what you are doing to hit this error?

[rstormsf]

@danjm Make sure you have more than 1 Custom RPC in your list

[rstormsf]

@danjm

[mnugumanova]

@danjm I’m able to reproduce it as well

[rstormsf]

just to clarify with 2 custom rpcs, you cannot send a transaction from that rpc

[frankiebee]

@rstormsf even with fixes 1 and 2 you will still need to remove the custom rpc you added and re add it. unfortunately theirs no way to tell if a custom rpc has a wrong entered chaid with settings. only if its NaN

[rstormsf]

@frankiebee it's ok, I can re-add them. Please let me know when I can test the fix on which metamask version release.
I believe it should be able to have more than 1 custom rpc without hard-coded rinkeby(4) chainId.
In order to know whether the chainId is correct or not - I don't believe it's responsibility of metamask, it should only query net_version API and that should be it.