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

no handling or timeout for a non-response from 4byte #6514

Closed
topocount opened this issue Apr 26, 2019 · 10 comments
Closed

no handling or timeout for a non-response from 4byte #6514

topocount opened this issue Apr 26, 2019 · 10 comments

Comments

@topocount
Copy link

topocount commented Apr 26, 2019

Describe the bug
4byte is currently down, so any contract interaction that isn't resolved by the parity registry lookup fails ungracefully. This causes users to wait like 60+ seconds for metamask to finally resolve with the confirm/reject buttons.

To Reproduce
Steps to reproduce the behavior:

  1. Go to remix
  2. Paste this simple contract code into a clean contract tab:
pragma solidity 0.5.7;
contract SimpleStore {
  function set(uint _value) public {
    value = _value;
  }

  function get() public view returns (uint) {
    return value;
  }

  uint value;
}
  1. compile the contract in the compile tab using the 0.5.7 production release
  2. click on the "run" IDE tab and click the deploy button in the run pane
  3. observe the metamask popup spin for a long time (if 4byte is still down) before ultimately resolving.

Expected behavior
should resolve to "contract deployment" in a second or two

Browser details (please complete the following information):

  • OS: Ubuntu Bionic
  • Browser chrome
  • MetaMask Version 6.4.0
@freakitties
Copy link

This issue is certainly affecting many users. Recommend removing reliance on as many 3rd party services as possible to prevent similar breakages in the future. 4byte, in particular, encourages you to scrap and host your own copy of their data "You are encouraged to scrape your own copy of this data if your use case is likely to produce heavy load on the API."

@nikita-fuchs
Copy link

Could someone please explain what exactly MetaMask would rely on 4byte for to deploy a contract? It's contract code -> compiled bytecode -> transaction data -> serialized and signed, all done client-side. No need for absolutely anything extra at all, especially not some function signature DB - or am I totally missing something here? Someone please brighten me up.

@MicahZoltu
Copy link

@nikita-fuchs By the time the transaction reaches MetaMask, the contract function signature has been lost and all that is available is the 4-byte signature hash prefix. In order for MetaMask to present the name/parameter parameters of the called function, it needs to map the 4-byte back to the function name.

A better solution would be something like ethereum/EIPs#719, which still needs a champion.

@bdresser
Copy link
Contributor

bdresser commented Apr 26, 2019

We're investigating the issue, will report back with a fix ETA

@bdresser
Copy link
Contributor

Publishing a hotfix momentarily, hang tight

@bdresser
Copy link
Contributor

bdresser commented Apr 26, 2019

rolled back in 6.4.1, live on Firefox already, will be live on Chrome momentarily.

if the problem is urgent for you, you can download 6.4.1 from our Releases page

Update: four hours later, still pending review on Chrome - they've added an additional review step for some reason. You can download 6.4.1 from our Releases page if you need it in the meantime. Thanks for your patience~

@nikita-fuchs
Copy link

@nikita-fuchs By the time the transaction reaches MetaMask, the contract function signature has been lost and all that is available is the 4-byte signature hash prefix. In order for MetaMask to present the name/parameter parameters of the called function, it needs to map the 4-byte back to the function name.

A better solution would be something like ethereum/EIPs#719, which still needs a champion.

Although this feature (I havn't noticed before) is really benevolent, it might turn out as the opposite: People might tend to think that they would be doing e.g. a token transfer, only because MetaMask says so, even though actually something totally different is going on in the contract.

@MicahZoltu
Copy link

@nikita-fuchs The user needs to trust the signing tool they are using (e.g., MetaMask) and they need to trust that the contract they are transacting with is who they intend to transact with. Given those two things, the user is safe from malicious contracts and malicious dapps.

If the user is communicating with a malicious dapp, the dapp may make a signing request for calling .transfer on some token contract. In this case, the user is communicating with a contract they don't intend to. The signing tool, unfortunately, doesn't have enough information to inform the user of this unless the signing tool happens to have a user-friendly name for the contract being communicated with, in which case it can present that to the user instead. ENS would help a lot with this.

However, even in the attack scenario, the situation is strictly better to show the user .transfer on some unknown contract than it is to show the user just a hext string.

@MicahZoltu
Copy link

MicahZoltu commented Apr 27, 2019

Keep in mind, the signing tool can very easily validate that the provided signature matches the transaction being signed. (though, there is risk of collision)

@bdresser
Copy link
Contributor

bdresser commented Apr 30, 2019

Chrome is up-to-date. Closing this via #6521

You can read a full post-mortem of the incident here

We're aware of function names showing up as [undefined] which will be fixed in #6524

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants