Disable eth_sign by default

[bschorchit]

Description

From Harry:
After looking at various phishing kits (and de-obfuscating), there are enormous trends between different kits that they all abuse eth_sign to ask users to inadvertently sign a transaction (as MetaMask UI is opaque to an on-chain transaction being signed). You can read more about the attack here.

@409H'd estimate that the abuse of eth_sign is near equal to the abuse of Wyvern and Seaport contracts in a manner to steal NFTs through creating orders for 0 eth to a specific beneficiary.

In short: eth_sign has been consistently abused by phishers/scammers to steal funds and the case may be that only power-users who use very legacy dapps may need to still use eth_sign.

• Disable eth_sign by default - throw an ethErrors.rpc.methodNotFound exception
• Allow the user to enable eth_sign through "Advanced Settings"

RPC Method preferences are stored within rpcMethodPreferences in the store state, allow for us to develop this further if someone wants to lock down rpc calls into MetaMask.

UI:

Technical Details

• In Advanced settings, introduce a new toggle with title Toggle eth_sign requests and description Turn this on to let dapps request your signature using eth_sign requests. eth_sign is an open-ended signing method that lets you sign an arbitrary hash, making it a dangerous phishing risk. Only sign eth_sign requests if you can read what you are signing and trust the origin of the request.. This should be the last item on advanced settings page.
• Include translations (Will need to submit a PR for Crowdin translations)
• Introduce a new state to track disabled RPC methods in the PreferencesController. The state should look like disabledRpcMethodPreferences: {eth_sign: false}
• Also introduce a method to set the rpc method preferences value with name and params setDisabledRpcMethodPreference(methodName: string, isEnabled: boolean)
• In RPCMethodMiddleware.ts and within eth_sign, check for PreferencesController.state.disabledRpcMethodPreferences.eth_sign and throw error if feature is disabled. Use error ethErrors.rpc.methodNotSupported(). Reference eth_signTransaction
• Follow up with a controllers PR to update PreferencesController with controller changes

Acceptance Criteria

• When toggle is off - interacting with eth_sign from a dapp should not work (fails silently)
• When toggle is on - interacting with eth_sign should prompt a signature in the app

References

Extension's PR: MetaMask/metamask-extension#17308
Figma - https://www.figma.com/file/X5tDiDfJN8IA9TXA1gM95S/Add-option-to-opt-in-(Disable-eth_sign-by-default)?node-id=0%3A1&t=0jFOOCjakJF0NNP5-1

[Cal-L]

Hey team! Please add your planning poker estimate with Zenhub @Fatxx @tommasini @jpcloureiro

[Cal-L]

Please add your planning poker estimate with Zenhub @chrisleewilcox

[Cal-L]

@bschorchit What would the user interaction look like when eth_sign is disabled and a Dapp attempts to make that RPC call? Does it fail silently, gracefully, or will a user see an alert? It looks like Extension silently fails, can you confirm?

[bschorchit]

Great question, Cal. It fails silently to the user and this is an intended behavior.
We'll revaluate wether we want to include a recovery path to the user in the future as we gather user and dapp feedback from this change. But right now it's intentional to not provide a recovery path as this method is mainly being used by scammers.

[Cal-L]

cc @yanrong-chen for design support

[Cal-L]

@holantonela
When you have the chance - could you update the ticket to reference Figma with the mobile UI? :thank-you: I think we're clear from the Product aspect but it would be great @bschorchit if you could take a look in case we missed anything. Thanks!

[bschorchit]

I made a slight update to the above to note that this should be the last toggle on Advanced Settings to match parity with Extension. Below is a screenshot of extension prod for reference as well.

[yanrong-chen]

Design for mobile https://www.figma.com/file/X5tDiDfJN8IA9TXA1gM95S/Add-option-to-opt-out-(Disable-eth_sign-by-default)?node-id=0%3A1&t=9u0CJbbBiGfPB0JE-0

[vandan]

On request for DevEx involvement here..
@bschorchit Given the benefits of deprecating eth_sign and urgency, I would like to confirm if we're scoping this to the minimum viable solution. For example, is it essential for Mobile to have precise parity with Extension on the "Fallback" option where we:

• Allow the user to enable eth_sign through "Advanced Settings" (which requires UI changes)

Would it be that problematic to only offer the enabling of eth_sign on Extension?

[vandan]

Confirmed with @rekmarks that we should aim for parity with Extension because the effort to implement the mobile UI is considered reasonable for DevEx (still react-based with pre-existing design components).