From 2e45a0665edb5184377f857dc235eb7f14d11c94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Tue, 7 Sep 2021 19:29:24 +0200 Subject: [PATCH] crypto: updates from reviews (#535) --- .github/workflows/test.yml | 2 +- app/ante/ante_test.go | 5 +-- app/ante/eth_test.go | 14 ++++----- app/ante/utils_test.go | 10 ------ client/export.go | 9 ++++-- crypto/ethsecp256k1/ethsecp256k1.go | 40 +++++++++++++++--------- crypto/ethsecp256k1/ethsecp256k1_test.go | 9 ++++-- crypto/hd/algorithm.go | 10 +++++- tests/signer.go | 7 ++++- x/evm/types/logs_test.go | 11 ++----- x/evm/types/msg_test.go | 12 ++----- x/evm/types/utils_test.go | 12 ------- 12 files changed, 71 insertions(+), 70 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 454d4946..15d281c3 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -78,7 +78,7 @@ jobs: test-solidity: runs-on: ubuntu-latest - timeout-minutes: 10 + timeout-minutes: 15 steps: - uses: actions/checkout@v2.3.4 - uses: technote-space/get-diff-action@v5 diff --git a/app/ante/ante_test.go b/app/ante/ante_test.go index 9f48507a..8818caef 100644 --- a/app/ante/ante_test.go +++ b/app/ante/ante_test.go @@ -6,12 +6,13 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/tharsis/ethermint/tests" evmtypes "github.com/tharsis/ethermint/x/evm/types" ) func (suite AnteTestSuite) TestAnteHandler() { - addr, privKey := newTestAddrKey() - to, _ := newTestAddrKey() + addr, privKey := tests.NewAddrKey() + to := tests.GenerateAddress() acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr.Bytes()) suite.Require().NoError(acc.SetSequence(1)) diff --git a/app/ante/eth_test.go b/app/ante/eth_test.go index acc2b51f..34ae1655 100644 --- a/app/ante/eth_test.go +++ b/app/ante/eth_test.go @@ -19,7 +19,7 @@ func nextFn(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { func (suite AnteTestSuite) TestEthSigVerificationDecorator() { dec := ante.NewEthSigVerificationDecorator(suite.app.EvmKeeper) - addr, privKey := newTestAddrKey() + addr, privKey := tests.NewAddrKey() signedTx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil) signedTx.From = addr.Hex() @@ -61,7 +61,7 @@ func (suite AnteTestSuite) TestNewEthAccountVerificationDecorator() { suite.app.AccountKeeper, suite.app.BankKeeper, suite.app.EvmKeeper, ) - addr, _ := newTestAddrKey() + addr := tests.GenerateAddress() tx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil) tx.From = addr.Hex() @@ -142,7 +142,7 @@ func (suite AnteTestSuite) TestNewEthAccountVerificationDecorator() { func (suite AnteTestSuite) TestEthNonceVerificationDecorator() { dec := ante.NewEthNonceVerificationDecorator(suite.app.AccountKeeper) - addr, _ := newTestAddrKey() + addr := tests.GenerateAddress() tx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil) tx.From = addr.Hex() @@ -199,7 +199,7 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { suite.app.AccountKeeper, suite.app.BankKeeper, suite.app.EvmKeeper, ) - addr, _ := newTestAddrKey() + addr := tests.GenerateAddress() tx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil) tx.From = addr.Hex() @@ -302,7 +302,7 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { func (suite AnteTestSuite) TestCanTransferDecorator() { dec := ante.NewCanTransferDecorator(suite.app.EvmKeeper) - addr, privKey := newTestAddrKey() + addr, privKey := tests.NewAddrKey() tx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, ðtypes.AccessList{}) tx2 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, ðtypes.AccessList{}) @@ -360,7 +360,7 @@ func (suite AnteTestSuite) TestCanTransferDecorator() { func (suite AnteTestSuite) TestAccessListDecorator() { dec := ante.NewAccessListDecorator(suite.app.EvmKeeper) - addr, _ := newTestAddrKey() + addr := tests.GenerateAddress() al := ðtypes.AccessList{ {Address: addr, StorageKeys: []common.Hash{{}}}, } @@ -418,7 +418,7 @@ func (suite AnteTestSuite) TestAccessListDecorator() { func (suite AnteTestSuite) TestEthIncrementSenderSequenceDecorator() { dec := ante.NewEthIncrementSenderSequenceDecorator(suite.app.AccountKeeper) - addr, privKey := newTestAddrKey() + addr, privKey := tests.NewAddrKey() contract := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 0, big.NewInt(10), 1000, big.NewInt(1), nil, nil) contract.From = addr.Hex() diff --git a/app/ante/utils_test.go b/app/ante/utils_test.go index ad231332..8ce8d604 100644 --- a/app/ante/utils_test.go +++ b/app/ante/utils_test.go @@ -4,9 +4,7 @@ import ( "testing" "time" - "github.com/ethereum/go-ethereum/common" ethtypes "github.com/ethereum/go-ethereum/core/types" - "github.com/ethereum/go-ethereum/crypto" "github.com/stretchr/testify/suite" @@ -23,7 +21,6 @@ import ( "github.com/tharsis/ethermint/app" ante "github.com/tharsis/ethermint/app/ante" - "github.com/tharsis/ethermint/crypto/ethsecp256k1" "github.com/tharsis/ethermint/encoding" "github.com/tharsis/ethermint/tests" evmtypes "github.com/tharsis/ethermint/x/evm/types" @@ -139,13 +136,6 @@ func (suite *AnteTestSuite) CreateTestTxBuilder( return txBuilder } -func newTestAddrKey() (common.Address, cryptotypes.PrivKey) { - privkey, _ := ethsecp256k1.GenerateKey() - addr := crypto.PubkeyToAddress(privkey.ToECDSA().PublicKey) - - return addr, privkey -} - var _ sdk.Tx = &invalidTx{} type invalidTx struct{} diff --git a/client/export.go b/client/export.go index 0518f00a..64975fa7 100644 --- a/client/export.go +++ b/client/export.go @@ -74,14 +74,19 @@ func UnsafeExportEthKeyCommand() *cobra.Command { return fmt.Errorf("invalid key algorithm, got %s, expected %s", algo, ethsecp256k1.KeyType) } - // Converts key to Ethermint secp256 implementation + // Converts key to Ethermint secp256k1 implementation ethPrivKey, ok := privKey.(*ethsecp256k1.PrivKey) if !ok { return fmt.Errorf("invalid private key type %T, expected %T", privKey, ðsecp256k1.PrivKey{}) } + key, err := ethPrivKey.ToECDSA() + if err != nil { + return err + } + // Formats key for output - privB := ethcrypto.FromECDSA(ethPrivKey.ToECDSA()) + privB := ethcrypto.FromECDSA(key) keyS := strings.ToUpper(hexutil.Encode(privB)[2:]) fmt.Println(keyS) diff --git a/crypto/ethsecp256k1/ethsecp256k1.go b/crypto/ethsecp256k1/ethsecp256k1.go index cc1fe2b3..38f16c33 100644 --- a/crypto/ethsecp256k1/ethsecp256k1.go +++ b/crypto/ethsecp256k1/ethsecp256k1.go @@ -54,12 +54,20 @@ func GenerateKey() (*PrivKey, error) { // Bytes returns the byte representation of the ECDSA Private Key. func (privKey PrivKey) Bytes() []byte { - return privKey.Key + bz := make([]byte, len(privKey.Key)) + copy(bz, privKey.Key) + + return bz } -// PubKey returns the ECDSA private key's public key. +// PubKey returns the ECDSA private key's public key. If the privkey is not valid +// it returns a nil value. func (privKey PrivKey) PubKey() cryptotypes.PubKey { - ecdsaPrivKey := privKey.ToECDSA() + ecdsaPrivKey, err := privKey.ToECDSA() + if err != nil { + return nil + } + return &PubKey{ Key: crypto.CompressPubkey(&ecdsaPrivKey.PublicKey), } @@ -106,21 +114,22 @@ func (privKey *PrivKey) UnmarshalAminoJSON(bz []byte) error { // provided hash of the message. The produced signature is 65 bytes // where the last byte contains the recovery ID. func (privKey PrivKey) Sign(digestBz []byte) ([]byte, error) { + // TODO: remove if len(digestBz) != crypto.DigestLength { digestBz = crypto.Keccak256Hash(digestBz).Bytes() } - return crypto.Sign(digestBz, privKey.ToECDSA()) + key, err := privKey.ToECDSA() + if err != nil { + return nil, err + } + + return crypto.Sign(digestBz, key) } // ToECDSA returns the ECDSA private key as a reference to ecdsa.PrivateKey type. -// The function will panic if the private key is invalid. -func (privKey PrivKey) ToECDSA() *ecdsa.PrivateKey { - key, err := crypto.ToECDSA(privKey.Bytes()) - if err != nil { - panic(err) - } - return key +func (privKey PrivKey) ToECDSA() (*ecdsa.PrivateKey, error) { + return crypto.ToECDSA(privKey.Bytes()) } // ---------------------------------------------------------------------------- @@ -132,11 +141,11 @@ var ( ) // Address returns the address of the ECDSA public key. -// The function will panic if the public key is invalid. +// The function will return an empty address if the public key is invalid. func (pubKey PubKey) Address() tmcrypto.Address { pubk, err := crypto.DecompressPubkey(pubKey.Key) if err != nil { - panic(err) + return nil } return tmcrypto.Address(crypto.PubkeyToAddress(*pubk).Bytes()) @@ -144,7 +153,10 @@ func (pubKey PubKey) Address() tmcrypto.Address { // Bytes returns the raw bytes of the ECDSA public key. func (pubKey PubKey) Bytes() []byte { - return pubKey.Key + bz := make([]byte, len(pubKey.Key)) + copy(bz, pubKey.Key) + + return bz } // String implements the fmt.Stringer interface. diff --git a/crypto/ethsecp256k1/ethsecp256k1_test.go b/crypto/ethsecp256k1/ethsecp256k1_test.go index c6b9d55e..b8d6e7b0 100644 --- a/crypto/ethsecp256k1/ethsecp256k1_test.go +++ b/crypto/ethsecp256k1/ethsecp256k1_test.go @@ -28,7 +28,9 @@ func TestPrivKey(t *testing.T) { // validate Ethereum address equality addr := privKey.PubKey().Address() - expectedAddr := crypto.PubkeyToAddress(privKey.ToECDSA().PublicKey) + key, err := privKey.ToECDSA() + require.NoError(t, err) + expectedAddr := crypto.PubkeyToAddress(key.PublicKey) require.Equal(t, expectedAddr.Bytes(), addr.Bytes()) // validate we can sign some bytes @@ -37,7 +39,7 @@ func TestPrivKey(t *testing.T) { expectedSig, err := secp256k1.Sign(sigHash.Bytes(), privKey.Bytes()) require.NoError(t, err) - sig, err := privKey.Sign(msg) + sig, err := privKey.Sign(sigHash.Bytes()) require.NoError(t, err) require.Equal(t, expectedSig, sig) } @@ -59,7 +61,8 @@ func TestPrivKey_PubKey(t *testing.T) { // validate signature msg := []byte("hello world") - sig, err := privKey.Sign(msg) + sigHash := crypto.Keccak256Hash(msg) + sig, err := privKey.Sign(sigHash.Bytes()) require.NoError(t, err) res := pubKey.VerifySignature(msg, sig) diff --git a/crypto/hd/algorithm.go b/crypto/hd/algorithm.go index ced596ce..53b92a0c 100644 --- a/crypto/hd/algorithm.go +++ b/crypto/hd/algorithm.go @@ -67,6 +67,7 @@ func (s ethSecp256k1Algo) Derive() hd.DeriveFn { return nil, err } + // create a BTC-utils hd-derivation key chain masterKey, err := hdkeychain.NewMaster(seed, &chaincfg.MainNetParams) if err != nil { return nil, err @@ -80,11 +81,15 @@ func (s ethSecp256k1Algo) Derive() hd.DeriveFn { } } + // btc-utils representation of a secp256k1 private key privateKey, err := key.ECPrivKey() if err != nil { return nil, err } + // cast private key to a convertible form (single scalar field element of secp256k1) + // and then load into ethcrypto private key format. + // TODO: add links to godocs of the two methods or implementations of them, to compare equivalency privateKeyECDSA := privateKey.ToECDSA() derivedKey := crypto.FromECDSA(privateKeyECDSA) @@ -98,6 +103,9 @@ func (s ethSecp256k1Algo) Generate() hd.GenerateFn { bzArr := make([]byte, ethsecp256k1.PrivKeySize) copy(bzArr, bz) - return ðsecp256k1.PrivKey{Key: bzArr} + // TODO: modulo P + return ðsecp256k1.PrivKey{ + Key: bzArr, + } } } diff --git a/tests/signer.go b/tests/signer.go index 4335a9a9..b90ecd6d 100644 --- a/tests/signer.go +++ b/tests/signer.go @@ -16,7 +16,12 @@ import ( // NewAddrKey generates an Ethereum address and its corresponding private key. func NewAddrKey() (common.Address, cryptotypes.PrivKey) { privkey, _ := ethsecp256k1.GenerateKey() - addr := crypto.PubkeyToAddress(privkey.ToECDSA().PublicKey) + key, err := privkey.ToECDSA() + if err != nil { + return common.Address{}, nil + } + + addr := crypto.PubkeyToAddress(key.PublicKey) return addr, privkey } diff --git a/x/evm/types/logs_test.go b/x/evm/types/logs_test.go index 1302d3fe..ebfcba65 100644 --- a/x/evm/types/logs_test.go +++ b/x/evm/types/logs_test.go @@ -5,16 +5,13 @@ import ( "github.com/stretchr/testify/require" - "github.com/tharsis/ethermint/crypto/ethsecp256k1" + "github.com/tharsis/ethermint/tests" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/crypto" ) func TestTransactionLogsValidate(t *testing.T) { - priv, err := ethsecp256k1.GenerateKey() - require.NoError(t, err) - addr := crypto.PubkeyToAddress(priv.ToECDSA().PublicKey).String() + addr := tests.GenerateAddress().String() testCases := []struct { name string @@ -90,9 +87,7 @@ func TestTransactionLogsValidate(t *testing.T) { } func TestValidateLog(t *testing.T) { - priv, err := ethsecp256k1.GenerateKey() - require.NoError(t, err) - addr := crypto.PubkeyToAddress(priv.ToECDSA().PublicKey).String() + addr := tests.GenerateAddress().String() testCases := []struct { name string diff --git a/x/evm/types/msg_test.go b/x/evm/types/msg_test.go index 456b96f3..4789b12c 100644 --- a/x/evm/types/msg_test.go +++ b/x/evm/types/msg_test.go @@ -8,12 +8,10 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keyring" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/tharsis/ethermint/crypto/ethsecp256k1" "github.com/tharsis/ethermint/tests" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" - "github.com/ethereum/go-ethereum/crypto" ) const invalidFromAddress = "0x0000" @@ -32,15 +30,11 @@ func TestMsgsTestSuite(t *testing.T) { } func (suite *MsgsTestSuite) SetupTest() { - privFrom, err := ethsecp256k1.GenerateKey() - suite.Require().NoError(err) - - privTo, err := ethsecp256k1.GenerateKey() - suite.Require().NoError(err) + from, privFrom := tests.NewAddrKey() suite.signer = tests.NewSigner(privFrom) - suite.from = crypto.PubkeyToAddress(privFrom.ToECDSA().PublicKey) - suite.to = crypto.PubkeyToAddress(privTo.ToECDSA().PublicKey) + suite.from = from + suite.to = tests.GenerateAddress() suite.chainID = big.NewInt(1) } diff --git a/x/evm/types/utils_test.go b/x/evm/types/utils_test.go index e98e4642..4c0127d3 100644 --- a/x/evm/types/utils_test.go +++ b/x/evm/types/utils_test.go @@ -10,26 +10,14 @@ import ( authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" proto "github.com/gogo/protobuf/proto" "github.com/tharsis/ethermint/app" - "github.com/tharsis/ethermint/crypto/ethsecp256k1" "github.com/tharsis/ethermint/encoding" evmtypes "github.com/tharsis/ethermint/x/evm/types" "github.com/stretchr/testify/require" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/crypto" ) -// GenerateEthAddress generates an Ethereum address. -func GenerateEthAddress() common.Address { - priv, err := ethsecp256k1.GenerateKey() - if err != nil { - panic(err) - } - - return crypto.PubkeyToAddress(priv.ToECDSA().PublicKey) -} - func TestEvmDataEncoding(t *testing.T) { ret := []byte{0x5, 0x8}