Add Multi-Signing Support for EIP-712 Cosmos Signatures

[0a1c]

Introduction

Ethermint's EIP-712 signature verification module does not allow for multi-signature verification in any capacity. The verification code artificially enforces a signer limit of exactly one, and by accepting EIP-712 signatures only in a specific extension (ExtensionOptionsWeb3Tx), the current implementation does not allow EIP-712 signatures to mix with software keys in a multi-signed payload.

This is important for Ledger users, since in order to participate in multi-signed transactions, they must move their private key out of their Ledger, which disturbs security and the user experience.

Proposal

We can accept EIP-712 signatures from the ethsecp256k1.PubKey class in order to support multi-signing for EIP-712-signed signatures, and include EIP-712 signatures in the transaction as though they were standard signatures, instead of using an ExtensionOptionsWeb3Tx extension.

Since the signature verification code in Ethermint for both single- and multi-signer payloads defers to each PubKey's VerifySignature(...) method, accepting EIP-712 signatures from that class will satisfy verification in all instances. EIP-712 signatures can thus be treated identically to standard signatures.

Current behavior

EIP-712 signatures are handled in an ExtensionOptionsWeb3Tx extension, which are limited to one signature and isolated from software key/non-EIP-712 signatures.

Desired behavior

EIP-712 signatures should be accepted if found within the transaction body, or in multi-key signatures. In other words, they should be treated identically to standard Ethermint signatures.

Implementation

Change crypto/ethsecp256k1/ethsecp256k1.go as follows:

1. Add a function verifySignatureAsEIP712(msg, sig []byte) bool which uses this code to convert the message to an EIP-712 object, and performs signature verification on the object hash and signature
2. Modify VerifySignature(msg, sig []byte) bool to return the following:
   return crypto.VerifySignature(pubKey.Key, crypto.Keccak256Hash(msg).Bytes(), sig) || verifySignatureAsEIP712(msg, sig)

Alternative Solution

Import and modify the Cosmos signature anteHandlerand verifySignature(...) methods, and create a new multi-key object to attempt signature verification for both signing methods in each case (change all relevant code from the top-down).

1. Create a new EIP712EnabledMultiKey multikey object that copies most of the infrastructure from LegacyAminoPubKey
2. Add a new method VerifyEIP712EnabledMultisignature that takes the following as input

pubKey cryptotypes.PubKey, signerData SignerData, sigData signing.SignatureData, handler SignModeHandler, tx sdk.Tx

and performs both standard signature verification and EIP-712 verification, and approves if either is approved. If a dependent public key is a multikey of type EIP712EnabledMultiKey, call VerifyEIP712EnabledMultisignature on that key.

1. Copy the authsigning.VerifySignature(...) method and:
   1. Replace instances of VerifyMultisignature with VerifyEIP712EnabledMultisignature
   2. Perform both standard and EIP-712 signature verification for single-pubkeys
2. Copy the ante.NewSigVerificationDecorator infrastructure and replace instances of authsigning.VerifySignature(...) with the local “forked” version

This would introduce hundreds of lines of new code to maintain, most of which is very critical. Recommend against this solution for that reason.

Release

On release, we will rename the current EIP-712 signature verification methods to indicate a legacy status, and phase it out in a number of months.

As we do so, we must refactor evmosjs and other EIP-712 signing libraries to move the signature to the transaction body from the extension, to participate with the new standard.

[github-actions]

This issue is stale because it has been open 45 days with no activity. Remove Status: Stale label or comment or this will be closed in 7 days.

[fedekunze]

@austinchandra can you leave a comment and close this issue if applicable?

[0a1c]

This has been implemented on Ethermint to be included in v0.20.0.

PR: #1397