[Bug]: tx sign doesn't throw an error when incorrect Ledger is used

[rootulp]

Cosmos SDK Version

0.46

Is there an existing issue for this?

• I have searched the existing issues

What happened

It looks like there's a bug when transactions are signed with the incorrect Ledger. In my scenario, I plugged in a Ledger that does not contain a key corresponding to the value provided for the --from flag but the tx sign command generates a signature and includes the pub key from the value provided for the --from flag. This means the user gets no signal that they created a output document that is invalid because the signature wasn't generated using the secret key of the pub key in the output document. The user only learns of their error later when the output document is combined with other output documents (i.e. in the multi-sign step).

What I would expect to happen

An error to be thrown informing the user that the key specified for the --from flag does not match the keys present on the Ledger.

How to reproduce?

The repro steps are a bit involved but I created a gist which can be used to repro on celestia-app. You'll need two Ledgers. The rough steps are:

1. Add a bunch of keys: test1, test2, test3 (from LedgerA), test4 (from LedgerB)
2. Generate a multisig composed of: test1, test2, test3
3. Fund the multisig
4. Generate an unsigned transaction from the multisig
5. Attempt to sign the generated transaction with the flags: --from test3 --ledger and LedgerB plugged in.
6. Observe no error

celestia-appd tx sign unsignedTx.json --multisig $MULTISIG --from test3 --output-document test3sig.json --chain-id $CHAIN_ID --ledger

7. Observe that the pub key in the generated signature is from test3 but the signature was generated from the test4 key (because LedgerB was plugged in).

$ cat test3sig.json
{"signatures":[{"public_key":{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Ao7rp7RgTTxCbe814Kb07zjgf8X3+Bl/gjX+hpeIloDW"},"data":{"single":{"mode":"SIGN_MODE_LEGACY_AMINO_JSON","signature":"zYBHYhUMSQe5TyddOYMWFknwtZpyAOSET5QGasbyhKgSVZNh+sggya4SxdunV7nyReuuoJEB6lBb+QaB6DG51w=="}},"sequence":"0"}]}

$ celestia-appd keys list
- address: celestia15zle3x6mgs6dqc4twlrwq6mvcwe8ueq76t8lsr
  name: multisig
  pubkey: '{"@type":"/cosmos.crypto.multisig.LegacyAminoPubKey","threshold":2,"public_keys":[{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AzQalBsVmpBt0ZEXlcL+SM+TWTj5sWh3kx/h7B1JDv/g"},{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AvOy0dhyVRR5KlrGrW8EfstLzVtWBvSWcDHJFIlfT5i5"},{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Ao7rp7RgTTxCbe814Kb07zjgf8X3+Bl/gjX+hpeIloDW"}]}'
  type: multi
- address: celestia1cjj2chd33292ahv7qfxnn6h5lalzt7ywj6tmew
  name: test1
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AvOy0dhyVRR5KlrGrW8EfstLzVtWBvSWcDHJFIlfT5i5"}'
  type: local
- address: celestia1n4qxe4juqn87v534ueemvfh4kkym4ef3gzl5lv
  name: test2
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AzQalBsVmpBt0ZEXlcL+SM+TWTj5sWh3kx/h7B1JDv/g"}'
  type: local
- address: celestia1eeyx3klj5p9lnyepl6cls9388l7qml4yxph92p
  name: test3
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Ao7rp7RgTTxCbe814Kb07zjgf8X3+Bl/gjX+hpeIloDW"}'
  type: ledger
- address: celestia169n4w3pys8qqz2p598636v0n3decd2fjq4zyy9
  name: test4
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Arz6D/H2A1Q8X8JOLlxiuqL8kZ2gSEdL7+/xl486Smod"}'
  type: ledger

[rootulp]

I have an approach that will at least throw an error after a user has attempted to sign with their Ledger. Apply the diff below: here

	// Sign those bytes
-	sigBytes, _, err := txf.keybase.Sign(name, bytesToSign)
+	sigBytes, ledgerPubKey, err := txf.keybase.Sign(name, bytesToSign)
	if err != nil {
		return err
	}
+	if !pubKey.Equals(ledgerPubKey) {
+		return fmt.Errorf("pubkey mismatch: %s ≠ %s", pubKey, ledgerPubKey)
+	}

which results in:

$ ./build/celestia-appd tx sign unsignedTx.json --multisig $MULTISIG --from test3 --output-document test3sig.json --chain-id $CHAIN_ID --ledger

Error: pubkey mismatch: PubKeySecp256k1{028EEBA7B4604D3C426DEF35E0A6F4EF38E07FC5F7F8197F8235FE8697889680D6} ≠ PubKeySecp256k1{037E01A47C24C8BC14AE0501C9C1D2CC96E27D5E0C5F7BE0ED679B50EA1701E37A}

I think a better approach would be to inform the user even prior to them trying to sign but I don't see a way to do that yet.

[rootulp]

I figured out an approach that will error before the user attempts to sign. Will publish a PR shortly. The important part of the diff is in keyring.go

+	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 %v does not match the public key on the ledger device %v", pubKey.String(), ledgerPubKey.String())
+	}

which results in

$ ./build/celestia-appd tx sign unsignedTx.json --multisig $MULTISIG --from test3 --output-document test3sig.json --chain-id $CHAIN_ID --ledger
Default sign-mode 'direct' not supported by Ledger, using sign-mode 'amino-json'.
Error: the public key that the user attempted to sign with PubKeySecp256k1{028EEBA7B4604D3C426DEF35E0A6F4EF38E07FC5F7F8197F8235FE8697889680D6} does not match the public key on the ledger device PubKeySecp256k1{037E01A47C24C8BC14AE0501C9C1D2CC96E27D5E0C5F7BE0ED679B50EA1701E37A}

I think there's probably a better way to print those public keys but defer to Cosmos SDK maintainers.

[rootulp]

Ahh while attempting to upstream, I discovered #14460 which wasn't backported to v0.46.x. Hmm, I don't see why the signature needs to be made to return the error. The approach in Zaki's PR returns an error after the user has signed with their Ledger but we can error before they sign with the diff above.