crypto: updates from reviews (#535)

.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

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()
+	addr, privKey := tests.NewAddrKey()
-	to, _ := newTestAddrKey()
+	to := tests.GenerateAddress()
 
 	acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr.Bytes())
 	suite.Require().NoError(acc.SetSequence(1))

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, &ethtypes.AccessList{})
 	tx2 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, &ethtypes.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 := &ethtypes.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()

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{}

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, &ethsecp256k1.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)

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, error) {
-func (privKey PrivKey) ToECDSA() *ecdsa.PrivateKey {
+	return crypto.ToECDSA(privKey.Bytes())
-	key, err := crypto.ToECDSA(privKey.Bytes())
-	if err != nil {
-		panic(err)
-	}
-	return key
 }
 
 // ----------------------------------------------------------------------------
@@ -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.

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)

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 &ethsecp256k1.PrivKey{Key: bzArr}
+		// TODO: modulo P
+		return &ethsecp256k1.PrivKey{
+			Key: bzArr,
+		}
 	}
 }

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
 }

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()
+	addr := tests.GenerateAddress().String()
-	require.NoError(t, err)
-	addr := crypto.PubkeyToAddress(priv.ToECDSA().PublicKey).String()
 
 	testCases := []struct {
 		name    string
@@ -90,9 +87,7 @@ func TestTransactionLogsValidate(t *testing.T) {
 }
 
 func TestValidateLog(t *testing.T) {
-	priv, err := ethsecp256k1.GenerateKey()
+	addr := tests.GenerateAddress().String()
-	require.NoError(t, err)
-	addr := crypto.PubkeyToAddress(priv.ToECDSA().PublicKey).String()
 
 	testCases := []struct {
 		name    string

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()
+	from, privFrom := tests.NewAddrKey()
-	suite.Require().NoError(err)
-
-	privTo, err := ethsecp256k1.GenerateKey()
-	suite.Require().NoError(err)
 
 	suite.signer = tests.NewSigner(privFrom)
-	suite.from = crypto.PubkeyToAddress(privFrom.ToECDSA().PublicKey)
+	suite.from = from
-	suite.to = crypto.PubkeyToAddress(privTo.ToECDSA().PublicKey)
+	suite.to = tests.GenerateAddress()
 	suite.chainID = big.NewInt(1)
 }
 

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}