From 07817ad365c51ec9d52d951d4c7c94b9a54ccfda Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 08:44:38 +0300 Subject: [PATCH 1/9] SIGN_MODE_EIP_191 --- x/auth/tx/eip191.go | 74 +++++++++++++++++++++++++++++++++++++++ x/auth/tx/mode_handler.go | 5 ++- 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 x/auth/tx/eip191.go diff --git a/x/auth/tx/eip191.go b/x/auth/tx/eip191.go new file mode 100644 index 000000000000..23d4753bbb13 --- /dev/null +++ b/x/auth/tx/eip191.go @@ -0,0 +1,74 @@ +package tx + +import ( + "bytes" + "encoding/json" + "fmt" + "strconv" + + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" + "github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx" + "github.com/cosmos/cosmos-sdk/x/auth/signing" +) + +const EIP191MessagePrefix = "\x19Ethereum Signed Message:\n" + +const eip191NonCriticalFieldsError = "protobuf transaction contains unknown non-critical fields. This is a transaction malleability issue and SIGN_MODE_EIP_191 cannot be used." + +var _ signing.SignModeHandler = signModeEIP191Handler{} + +// signModeEIP191Handler defines the SIGN_MODE_EIP191 +// SignModeHandler. +type signModeEIP191Handler struct{} + +func (s signModeEIP191Handler) DefaultMode() signingtypes.SignMode { + return signingtypes.SignMode_SIGN_MODE_EIP_191 +} + +func (s signModeEIP191Handler) Modes() []signingtypes.SignMode { + return []signingtypes.SignMode{signingtypes.SignMode_SIGN_MODE_EIP_191} +} + +func (s signModeEIP191Handler) GetSignBytes(mode signingtypes.SignMode, data signing.SignerData, tx sdk.Tx) ([]byte, error) { + if mode != signingtypes.SignMode_SIGN_MODE_EIP_191 { + return nil, fmt.Errorf("expected %s, got %s", signingtypes.SignMode_SIGN_MODE_EIP_191, mode) + } + + protoTx, ok := tx.(*wrapper) + if !ok { + return nil, fmt.Errorf("can only handle a protobuf Tx, got %T", tx) + } + + if protoTx.txBodyHasUnknownNonCriticals { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, eip191NonCriticalFieldsError) + } + + body := protoTx.tx.Body + + if len(body.ExtensionOptions) != 0 || len(body.NonCriticalExtensionOptions) != 0 { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "SignMode_SIGN_MODE_EIP_191 does not support protobuf extension options.") + } + + aminoJSONBz := legacytx.StdSignBytes( + data.ChainID, data.AccountNumber, data.Sequence, protoTx.GetTimeoutHeight(), + legacytx.StdFee{Amount: protoTx.GetFee(), Gas: protoTx.GetGas()}, + tx.GetMsgs(), protoTx.GetMemo(), + ) + + var out bytes.Buffer + if err := json.Indent(&out, aminoJSONBz, "", " "); err != nil { + return nil, err + } + + bz := append( + []byte(EIP191MessagePrefix), + []byte(strconv.Itoa(len(out.Bytes())))..., + ) + + bz = append(bz, out.Bytes()...) + // bz = append(bz, aminoJSONBz...) + + return bz, nil +} diff --git a/x/auth/tx/mode_handler.go b/x/auth/tx/mode_handler.go index f49ee16198d7..f56fa57692e4 100644 --- a/x/auth/tx/mode_handler.go +++ b/x/auth/tx/mode_handler.go @@ -11,10 +11,11 @@ import ( var DefaultSignModes = []signingtypes.SignMode{ signingtypes.SignMode_SIGN_MODE_DIRECT, signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, + signingtypes.SignMode_SIGN_MODE_EIP_191, } // makeSignModeHandler returns the default protobuf SignModeHandler supporting -// SIGN_MODE_DIRECT and SIGN_MODE_LEGACY_AMINO_JSON. +// SIGN_MODE_DIRECT, SIGN_MODE_LEGACY_AMINO_JSON, and SIGN_MODE_SIGN_MODE_EIP_191. func makeSignModeHandler(modes []signingtypes.SignMode) signing.SignModeHandler { if len(modes) < 1 { panic(fmt.Errorf("no sign modes enabled")) @@ -28,6 +29,8 @@ func makeSignModeHandler(modes []signingtypes.SignMode) signing.SignModeHandler handlers[i] = signModeDirectHandler{} case signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON: handlers[i] = signModeLegacyAminoJSONHandler{} + case signingtypes.SignMode_SIGN_MODE_EIP_191: + handlers[i] = signModeEIP191Handler{} default: panic(fmt.Errorf("unsupported sign mode %+v", mode)) } From e304fe0649673424e778f581e64708f1e6153db6 Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 11:58:49 +0300 Subject: [PATCH 2/9] SIGN_MODE_EIP_191: Fix VerifySignature --- crypto/keys/secp256k1/secp256k1_nocgo.go | 26 ++++++++++++++++++++++-- x/auth/signing/verify.go | 14 +++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/crypto/keys/secp256k1/secp256k1_nocgo.go b/crypto/keys/secp256k1/secp256k1_nocgo.go index 60fd9577978c..99d6144a37f7 100644 --- a/crypto/keys/secp256k1/secp256k1_nocgo.go +++ b/crypto/keys/secp256k1/secp256k1_nocgo.go @@ -9,12 +9,13 @@ import ( secp256k1 "github.com/btcsuite/btcd/btcec" "github.com/tendermint/tendermint/crypto" + "golang.org/x/crypto/sha3" ) // used to reject malleable signatures // see: -// - https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93 -// - https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/crypto.go#L39 +// - https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93 +// - https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/crypto.go#L39 var secp256k1halfN = new(big.Int).Rsh(secp256k1.S256().N, 1) // Sign creates an ECDSA signature on curve Secp256k1, using SHA256 on the msg. @@ -49,6 +50,27 @@ func (pubKey *PubKey) VerifySignature(msg []byte, sigStr []byte) bool { return signature.Verify(crypto.Sha256(msg), pub) } +// VerifyBytes verifies a signature of the form R || S. +// It rejects signatures which are not in lower-S form. +func (pubKey *PubKey) VerifySignatureEip191(msg []byte, sigStr []byte) bool { + if len(sigStr) != 64 { + return false + } + pub, err := secp256k1.ParsePubKey(pubKey.Key, secp256k1.S256()) + if err != nil { + return false + } + // parse the signature: + signature := signatureFromBytes(sigStr) + // Reject malleable signatures. libsecp256k1 does this check but btcec doesn't. + // see: https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93 + if signature.S.Cmp(secp256k1halfN) > 0 { + return false + } + + return signature.Verify(sha3.NewLegacyKeccak256().Sum(msg), pub) +} + // Read Signature struct from R || S. Caller needs to ensure // that len(sigStr) == 64. func signatureFromBytes(sigStr []byte) *secp256k1.Signature { diff --git a/x/auth/signing/verify.go b/x/auth/signing/verify.go index 5a5395de69f3..f7fb92d1dc79 100644 --- a/x/auth/signing/verify.go +++ b/x/auth/signing/verify.go @@ -3,6 +3,7 @@ package signing import ( "fmt" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/crypto/types/multisig" sdk "github.com/cosmos/cosmos-sdk/types" @@ -18,8 +19,17 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s if err != nil { return err } - if !pubKey.VerifySignature(signBytes, data.Signature) { - return fmt.Errorf("unable to verify single signer signature") + + if data.SignMode == signing.SignMode_SIGN_MODE_EIP_191 { + // do this to not have to register a new type of pubkey like here: + // https://github.com/scrtlabs/cosmos-sdk/blob/07817ad365/crypto/keys/secp256k1/keys.pb.go#L120 + if !pubKey.(*secp256k1.PubKey).VerifySignatureEip191(signBytes, data.Signature) { + return fmt.Errorf("unable to verify single signer EIP191 signature") + } + } else { + if !pubKey.VerifySignature(signBytes, data.Signature) { + return fmt.Errorf("unable to verify single signer signature") + } } return nil From dbb6e18d098d4c097aaadb07e532c1860cb10389 Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 13:38:28 +0300 Subject: [PATCH 3/9] SIGN_MODE_EIP_191: Better error messages --- x/auth/ante/sigverify.go | 5 ++--- x/auth/signing/verify.go | 10 ++++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index dc2fedbc0772..f05ac2ce4a4c 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -289,9 +289,9 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul if OnlyLegacyAminoSigners(sig.Data) { // If all signers are using SIGN_MODE_LEGACY_AMINO, we rely on VerifySignature to check account sequence number, // and therefore communicate sequence number as a potential cause of error. - errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s)", accNum, acc.GetSequence(), chainID) + errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s): %s", accNum, acc.GetSequence(), chainID, err.Error()) } else { - errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d) and chain-id (%s)", accNum, chainID) + errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d) and chain-id (%s): %s", accNum, chainID, err.Error()) } return ctx, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, errMsg) @@ -419,7 +419,6 @@ func ConsumeMultisignatureVerificationGas( meter sdk.GasMeter, sig *signing.MultiSignatureData, pubkey multisig.PubKey, params types.Params, accSeq uint64, ) error { - size := sig.BitArray.Count() sigIndex := 0 diff --git a/x/auth/signing/verify.go b/x/auth/signing/verify.go index f7fb92d1dc79..aa352f11a249 100644 --- a/x/auth/signing/verify.go +++ b/x/auth/signing/verify.go @@ -23,8 +23,14 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s if data.SignMode == signing.SignMode_SIGN_MODE_EIP_191 { // do this to not have to register a new type of pubkey like here: // https://github.com/scrtlabs/cosmos-sdk/blob/07817ad365/crypto/keys/secp256k1/keys.pb.go#L120 - if !pubKey.(*secp256k1.PubKey).VerifySignatureEip191(signBytes, data.Signature) { - return fmt.Errorf("unable to verify single signer EIP191 signature") + + secp256k1PubKey, ok := pubKey.(*secp256k1.PubKey) + if !ok { + return fmt.Errorf("eip191 sign mode requires pubkey to be of type secp256k1") + } + + if !secp256k1PubKey.VerifySignatureEip191(signBytes, data.Signature) { + return fmt.Errorf("unable to verify single signer eip191 signature") } } else { if !pubKey.VerifySignature(signBytes, data.Signature) { From 40a76d35151ed42a7337ac11f8df0f5c2a4605fe Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 15:27:12 +0300 Subject: [PATCH 4/9] SIGN_MODE_EIP_191: Fix usage of keccak256 AKA 5 hours of debugging saved Assaf 5 minutes of reading the docs --- crypto/keys/secp256k1/secp256k1_nocgo.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crypto/keys/secp256k1/secp256k1_nocgo.go b/crypto/keys/secp256k1/secp256k1_nocgo.go index 99d6144a37f7..5cde04e82554 100644 --- a/crypto/keys/secp256k1/secp256k1_nocgo.go +++ b/crypto/keys/secp256k1/secp256k1_nocgo.go @@ -68,7 +68,13 @@ func (pubKey *PubKey) VerifySignatureEip191(msg []byte, sigStr []byte) bool { return false } - return signature.Verify(sha3.NewLegacyKeccak256().Sum(msg), pub) + return signature.Verify(keccak256(msg), pub) +} + +func keccak256(bytes []byte) []byte { + hasher := sha3.NewLegacyKeccak256() + hasher.Write(bytes) + return hasher.Sum(nil) } // Read Signature struct from R || S. Caller needs to ensure From 95e10d583a7959b60c4c43644a498cdad1247356 Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 20:24:21 +0300 Subject: [PATCH 5/9] SIGN_MODE_EIP_191: Better error messages --- x/auth/signing/verify.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/auth/signing/verify.go b/x/auth/signing/verify.go index aa352f11a249..c761fee5b295 100644 --- a/x/auth/signing/verify.go +++ b/x/auth/signing/verify.go @@ -1,6 +1,7 @@ package signing import ( + "encoding/hex" "fmt" "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" @@ -30,7 +31,7 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s } if !secp256k1PubKey.VerifySignatureEip191(signBytes, data.Signature) { - return fmt.Errorf("unable to verify single signer eip191 signature") + return fmt.Errorf("unable to verify single signer eip191 signature %s for signBytes %s", hex.EncodeToString(signBytes), string(signBytes)) } } else { if !pubKey.VerifySignature(signBytes, data.Signature) { From 2eb1ebcb2f3f5eddaa2ee3794c05c5f5c0370338 Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 21:06:33 +0300 Subject: [PATCH 6/9] EIP191: Fix error message --- x/auth/signing/verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/signing/verify.go b/x/auth/signing/verify.go index c761fee5b295..ffcd79236e38 100644 --- a/x/auth/signing/verify.go +++ b/x/auth/signing/verify.go @@ -31,7 +31,7 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s } if !secp256k1PubKey.VerifySignatureEip191(signBytes, data.Signature) { - return fmt.Errorf("unable to verify single signer eip191 signature %s for signBytes %s", hex.EncodeToString(signBytes), string(signBytes)) + return fmt.Errorf("unable to verify single signer eip191 signature %s for signBytes %s", hex.EncodeToString(data.Signature), hex.EncodeToString(signBytes)) } } else { if !pubKey.VerifySignature(signBytes, data.Signature) { From 3b6f26fed2f372df98f7a891d626ec8674013ba0 Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 21:53:13 +0300 Subject: [PATCH 7/9] EIP191: Correctly pass Amino encoding to sigverify --- x/auth/tx/eip191.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/x/auth/tx/eip191.go b/x/auth/tx/eip191.go index 23d4753bbb13..d21cd13aee6b 100644 --- a/x/auth/tx/eip191.go +++ b/x/auth/tx/eip191.go @@ -1,8 +1,6 @@ package tx import ( - "bytes" - "encoding/json" "fmt" "strconv" @@ -57,18 +55,12 @@ func (s signModeEIP191Handler) GetSignBytes(mode signingtypes.SignMode, data sig tx.GetMsgs(), protoTx.GetMemo(), ) - var out bytes.Buffer - if err := json.Indent(&out, aminoJSONBz, "", " "); err != nil { - return nil, err - } - bz := append( []byte(EIP191MessagePrefix), - []byte(strconv.Itoa(len(out.Bytes())))..., + []byte(strconv.Itoa(len(aminoJSONBz)))..., ) - bz = append(bz, out.Bytes()...) - // bz = append(bz, aminoJSONBz...) + bz = append(bz, aminoJSONBz...) return bz, nil } From 62e39f0e3b134d1f6e52ad4757b163633648f1dc Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 23:13:03 +0300 Subject: [PATCH 8/9] Better error messages for mempool sigverify --- x/auth/signing/verify.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/auth/signing/verify.go b/x/auth/signing/verify.go index ffcd79236e38..281d61b2ca26 100644 --- a/x/auth/signing/verify.go +++ b/x/auth/signing/verify.go @@ -27,7 +27,7 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s secp256k1PubKey, ok := pubKey.(*secp256k1.PubKey) if !ok { - return fmt.Errorf("eip191 sign mode requires pubkey to be of type secp256k1") + return fmt.Errorf("eip191 sign mode requires pubkey to be of type cosmos.crypto.secp256k1.PubKey") } if !secp256k1PubKey.VerifySignatureEip191(signBytes, data.Signature) { @@ -35,7 +35,7 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s } } else { if !pubKey.VerifySignature(signBytes, data.Signature) { - return fmt.Errorf("unable to verify single signer signature") + return fmt.Errorf("unable to verify single signer eip191 signature %s for signBytes %s", hex.EncodeToString(data.Signature), hex.EncodeToString(signBytes)) } } return nil From 789a8536a6415c8efcac310aec46858d33fc67ba Mon Sep 17 00:00:00 2001 From: Assaf Morami Date: Mon, 15 Aug 2022 23:31:35 +0300 Subject: [PATCH 9/9] Fix copy-paste error from previous commit --- x/auth/signing/verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/signing/verify.go b/x/auth/signing/verify.go index 281d61b2ca26..6d3a6a60902c 100644 --- a/x/auth/signing/verify.go +++ b/x/auth/signing/verify.go @@ -35,7 +35,7 @@ func VerifySignature(pubKey cryptotypes.PubKey, signerData SignerData, sigData s } } else { if !pubKey.VerifySignature(signBytes, data.Signature) { - return fmt.Errorf("unable to verify single signer eip191 signature %s for signBytes %s", hex.EncodeToString(data.Signature), hex.EncodeToString(signBytes)) + return fmt.Errorf("unable to verify single signer signature %s for signBytes %s", hex.EncodeToString(data.Signature), hex.EncodeToString(signBytes)) } } return nil