Correct double hashing issue

Signed-off-by: Peter Broadhurst <[email protected]>

pkg/ethsigner/typed_data.go

@@ -21,7 +21,6 @@ import (
 
 	"github.com/hyperledger/firefly-signer/pkg/eip712"
 	"github.com/hyperledger/firefly-signer/pkg/ethtypes"
-	"github.com/hyperledger/firefly-signer/pkg/rlp"
 	"github.com/hyperledger/firefly-signer/pkg/secp256k1"
 )
 
@@ -38,20 +37,20 @@ func SignTypedDataV4(ctx context.Context, signer secp256k1.Signer, payload *eip7
 	if err != nil {
 		return nil, err
 	}
-	sig, err := signer.Sign(encodedData)
+	// Note that signer.Sign performs the hash
+	sig, err := signer.SignDirect(encodedData)
 	if err != nil {
 		return nil, err
 	}
 
-	rlpList := make(rlp.List, 0, 4)
-	rlpList = append(rlpList, rlp.Data(encodedData))
-	rlpList = append(rlpList, rlp.WrapInt(sig.R))
-	rlpList = append(rlpList, rlp.WrapInt(sig.S))
-	rlpList = append(rlpList, rlp.WrapInt(sig.V))
+	signatureBytes := make([]byte, 65)
+	signatureBytes[0] = byte(sig.V.Int64())
+	sig.R.FillBytes(signatureBytes[1:33])
+	sig.S.FillBytes(signatureBytes[33:65])
 
 	return &EIP712Result{
 		Hash:      encodedData,
-		Signature: rlpList.Encode(), // per eth_signTypedData_v4 convention
+		Signature: signatureBytes, // compact ECDSA signature
 		V:         ethtypes.HexInteger(*sig.V),
 		R:         sig.R.FillBytes(make([]byte, 32)),
 		S:         sig.S.FillBytes(make([]byte, 32)),

pkg/ethsigner/typed_data_test.go

@@ -23,11 +23,13 @@ import (
 	"math/big"
 	"testing"
 
+	"github.com/btcsuite/btcd/btcec/v2/ecdsa"
 	"github.com/hyperledger/firefly-common/pkg/log"
 	"github.com/hyperledger/firefly-signer/mocks/secp256k1mocks"
 	"github.com/hyperledger/firefly-signer/pkg/eip712"
 	"github.com/hyperledger/firefly-signer/pkg/ethtypes"
 	"github.com/hyperledger/firefly-signer/pkg/secp256k1"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 )
@@ -58,7 +60,7 @@ func TestSignTypedDataV4(t *testing.T) {
 	foundSig.S.SetBytes(sig.S)
 
 	signaturePayload := ethtypes.HexBytes0xPrefix(sig.Hash)
-	addr, err := foundSig.Recover(signaturePayload, -1 /* chain id is in the domain (not applied EIP-155 style to the V value) */)
+	addr, err := foundSig.RecoverDirect(signaturePayload, -1 /* chain id is in the domain (not applied EIP-155 style to the V value) */)
 	assert.NoError(t, err)
 	assert.Equal(t, keypair.Address.String(), addr.String())
 
@@ -91,9 +93,77 @@ func TestSignTypedDataV4SignFail(t *testing.T) {
 	}
 
 	msn := &secp256k1mocks.Signer{}
-	msn.On("Sign", mock.Anything).Return(nil, fmt.Errorf("pop"))
+	msn.On("SignDirect", mock.Anything).Return(nil, fmt.Errorf("pop"))
 
 	ctx := context.Background()
 	_, err := SignTypedDataV4(ctx, msn, payload)
 	assert.Regexp(t, "pop", err)
 }
+
+func TestMessage_2(t *testing.T) {
+	logrus.SetLevel(logrus.TraceLevel)
+
+	var p eip712.TypedData
+	err := json.Unmarshal([]byte(`{
+		"domain": {
+			"name": "test-app",
+			"version": "1",
+			"chainId": 31337,
+			"verifyingContract": "0x9fe46736679d2d9a65f0992f2272de9f3c7fa6e0"
+		},
+		"types": {
+			"Issuance": [
+				{
+					"name": "amount",
+					"type": "uint256"
+				},
+				{
+					"name": "to",
+					"type": "address"
+				}
+			],
+			"EIP712Domain": [
+				{
+					"name": "name",
+					"type": "string"
+				},
+				{
+					"name": "version",
+					"type": "string"
+				},
+				{
+					"name": "chainId",
+					"type": "uint256"
+				},
+				{
+					"name": "verifyingContract",
+					"type": "address"
+				}
+			]
+		},
+		"primaryType": "Issuance",
+		"message": {
+			"amount": "1000",
+			"to": "0xce3a47d24140cca16f8839357ca5fada44a1baef"
+		}
+	}`), &p)
+	assert.NoError(t, err)
+
+	ctx := context.Background()
+	ed, err := eip712.EncodeTypedDataV4(ctx, &p)
+	assert.NoError(t, err)
+	assert.Equal(t, "0xb0132202fa81cafac0e405917f86705728ba02912d185065697cc4ba4e61aec3", ed.String())
+
+	keys, err := secp256k1.NewSecp256k1KeyPair([]byte(`8d01666832be7eb2dbd57cd3d4410d0231a91533f895de76d0930c689618aefd`))
+	assert.NoError(t, err)
+	assert.Equal(t, "0xbcef501facf72ddacdb055acc2716786ff038728", keys.Address.String())
+
+	signed, err := SignTypedDataV4(ctx, keys, &p)
+	assert.NoError(t, err)
+
+	assert.Equal(t, "0xb0132202fa81cafac0e405917f86705728ba02912d185065697cc4ba4e61aec3", signed.Hash.String())
+
+	pubKey, _, err := ecdsa.RecoverCompact(signed.Signature, signed.Hash)
+	assert.NoError(t, err)
+	assert.Equal(t, "0xbcef501facf72ddacdb055acc2716786ff038728", secp256k1.PublicKeyToAddress(pubKey).String())
+}

pkg/secp256k1/signer.go

@@ -1,4 +1,4 @@
-// Copyright © 2022 Kaleido, Inc.
+// Copyright © 2023 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -34,6 +34,7 @@ type SignatureData struct {
 // Signer is the low level common interface that can be implemented by any module which provides signature capability
 type Signer interface {
 	Sign(message []byte) (*SignatureData, error)
+	SignDirect(message []byte) (*SignatureData, error)
 }
 
 // getVNormalized returns the original 27/28 parity
@@ -67,32 +68,44 @@ func (s *SignatureData) UpdateEIP2930() {
 	s.V = s.V.Sub(s.V, big.NewInt(27))
 }
 
-// Recover obtains the original signer
+// Recover obtains the original signer from the hash of the message
 func (s *SignatureData) Recover(message []byte, chainID int64) (a *ethtypes.Address0xHex, err error) {
 	msgHash := sha3.NewLegacyKeccak256()
 	msgHash.Write(message)
+	return s.RecoverDirect(msgHash.Sum(nil), chainID)
+}
+
+// Recover obtains the original signer
+func (s *SignatureData) RecoverDirect(message []byte, chainID int64) (a *ethtypes.Address0xHex, err error) {
+
 	signatureBytes := make([]byte, 65)
 	signatureBytes[0], err = s.getVNormalized(chainID)
 	if err != nil {
 		return nil, err
 	}
 	s.R.FillBytes(signatureBytes[1:33])
 	s.S.FillBytes(signatureBytes[33:65])
-	pubKey, _, err := ecdsa.RecoverCompact(signatureBytes, msgHash.Sum(nil)) // uses S256() by default
+	pubKey, _, err := ecdsa.RecoverCompact(signatureBytes, message) // uses S256() by default
 	if err != nil {
 		return nil, err
 	}
 	return PublicKeyToAddress(pubKey), nil
 }
 
-// Sign performs raw signing - give legacy 27/28 V values
+// Sign hashes the input then signs it
 func (k *KeyPair) Sign(message []byte) (ethSig *SignatureData, err error) {
+	msgHash := sha3.NewLegacyKeccak256()
+	msgHash.Write(message)
+	hashed := msgHash.Sum(nil)
+	return k.SignDirect(hashed)
+}
+
+// SignDirect performs raw signing - give legacy 27/28 V values
+func (k *KeyPair) SignDirect(message []byte) (ethSig *SignatureData, err error) {
 	if k == nil {
 		return nil, fmt.Errorf("nil signer")
 	}
-	msgHash := sha3.NewLegacyKeccak256()
-	msgHash.Write(message)
-	sig, err := ecdsa.SignCompact(k.PrivateKey, msgHash.Sum(nil), false) // uses S256() by default
+	sig, err := ecdsa.SignCompact(k.PrivateKey, message, false) // uses S256() by default
 	if err == nil {
 		// btcec does all the hard work for us. However, the interface of btcec is such
 		// that we need to unpack the result for Ethereum encoding.