fix(crypto): error if incorrect ledger public key (backport #19691) (#…

…19745)

Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: sontrinh16 <trinhleson2000@gmail.com>

CHANGELOG.md

@@ -38,6 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+## Bug Fixes
+
+* (crypto) [#19691](https://github.com/cosmos/cosmos-sdk/pull/19745) Fix tx sign doesn't throw an error when incorrect Ledger is used.
+
 ## [v0.50.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.5) - 2024-03-12
 
 ### Features

crypto/keyring/keyring.go

@@ -634,6 +634,14 @@ func SignWithLedger(k *Record, msg []byte, signMode signing.SignMode) (sig []byt
 	if err != nil {
 		return
 	}
+	ledgerPubKey := priv.PubKey()
+	pubKey, err := k.GetPubKey()
+	if err != nil {
+		return nil, nil, err
+	}
+	if !pubKey.Equals(ledgerPubKey) {
+		return nil, nil, fmt.Errorf("the public key that the user attempted to sign with does not match the public key on the ledger device. %v does not match %v", pubKey.String(), ledgerPubKey.String())
+	}
 
 	switch signMode {
 	case signing.SignMode_SIGN_MODE_TEXTUAL:

crypto/keyring/keyring_ledger_test.go

@@ -8,9 +8,13 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
+	"github.com/cosmos/cosmos-sdk/crypto/ledger"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	"github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 )
@@ -133,3 +137,65 @@ func TestAltKeyring_SaveLedgerKey(t *testing.T) {
 	path := ledgerInfo.GetPath()
 	require.Equal(t, "m/44'/118'/3'/0/1", path.String())
 }
+
+func TestSignWithLedger(t *testing.T) {
+	// Create two distinct Ledger records: recordA and recordB.
+	// RecordA is added to the Ledger but recordB is not added.
+	pathA := hd.NewFundraiserParams(0, types.CoinType, 0)
+	privA, _, err := ledger.NewPrivKeySecp256k1(*pathA, "cosmos")
+	require.NoError(t, err)
+	recordA, err := NewLedgerRecord("ledgerA", privA.PubKey(), pathA)
+	require.NoError(t, err)
+	pubA, err := recordA.GetPubKey()
+	require.NoError(t, err)
+
+	pathB := hd.NewFundraiserParams(0, types.CoinType, 1)
+	// privB won't be added to the Ledger because it doesn't use ledger.NewPrivKeySecp256k1
+	privB := secp256k1.GenPrivKey()
+	recordB, err := NewLedgerRecord("ledgerB", privB.PubKey(), pathB)
+	require.NoError(t, err)
+	pubB, err := recordB.GetPubKey()
+	require.NoError(t, err)
+
+	require.NotEqual(t, pubA, pubB)
+	type testCase struct {
+		name            string
+		record          *Record
+		msg             []byte
+		wantSig         []byte
+		wantPub         cryptotypes.PubKey
+		wantErr         bool
+		wantErrContains string
+	}
+	testCases := []testCase{
+		{
+			name:    "ordinary ledger tx",
+			record:  recordA,
+			msg:     []byte("msg"),
+			wantSig: []byte{0xfb, 0x93, 0x1b, 0xb9, 0x75, 0x25, 0xe7, 0x99, 0x64, 0xc2, 0x78, 0xf7, 0x94, 0x9a, 0x63, 0x83, 0xe2, 0x59, 0x76, 0x48, 0x1d, 0x2, 0xbc, 0xc2, 0x83, 0x21, 0x24, 0x4b, 0x95, 0x99, 0x25, 0x8b, 0x30, 0x38, 0x6, 0x61, 0x79, 0x9a, 0x9e, 0x8, 0x98, 0xfd, 0x34, 0xc6, 0x7e, 0x47, 0x4d, 0x5f, 0xe, 0xf3, 0xc3, 0xe7, 0xdd, 0xe3, 0x89, 0x80, 0xda, 0x8b, 0x48, 0x15, 0x34, 0xce, 0xdf, 0x1c},
+			wantPub: pubA,
+			wantErr: false,
+		},
+		{
+			name:            "want error when the public key the user attempted to sign with doesn't match the public key on the ledger",
+			record:          recordB,
+			msg:             []byte("msg"),
+			wantSig:         []byte(nil),
+			wantPub:         nil,
+			wantErr:         true,
+			wantErrContains: "the public key that the user attempted to sign with does not match the public key on the ledger device",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			sig, pub, err := SignWithLedger(tc.record, tc.msg, signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
+			assert.Equal(t, tc.wantSig, sig)
+			assert.Equal(t, tc.wantPub, pub)
+			if tc.wantErr {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tc.wantErrContains)
+			}
+		})
+	}
+}