Sign and SignWithPrivKey produce different signatures

[RiccardoM]

Summary of Bug

Currently, the client/tx/tx.go file contains two methods: SignWithPrivKey and Sign. These produce two different signatures with the same input due to a difference in their behaviors.

Version

v0.40.1

Steps to Reproduce

While trying to use the Cosmos SDK as a library to build, sign and broadcast some transactions I ran into a difference of behavior between Sign and SignWithPrivKey.

Here is the code I used to sign with SignWithPrivKey:

Sign with SignWithPrivKey code

func (client *Client) BroadcastTx(msgs ...sdk.Msg) error {
	// Build the factory CLI
	factoryCLI := tx.NewFactoryCLI(client.cliCtx, &pflag.FlagSet{}).
		WithFees(client.fees).
		WithSignMode(signing.SignMode_SIGN_MODE_DIRECT)

	// Create the factory
	factory, err := tx.PrepareFactory(client.cliCtx, factoryCLI)
	if err != nil {
		return err
	}

	// Build an unsigned transaction
	builder, err := tx.BuildUnsignedTx(factory, msgs...)
	if err != nil {
		return err
	}

	// Sign the transaction with the private key
	sigs, err := tx.SignWithPrivKey(
		factory.SignMode(),
		authsigning.SignerData{
			ChainID:       factory.ChainID(),
			AccountNumber: factory.AccountNumber(),
			Sequence:      factory.Sequence(),
		},
		builder,
		client.privKey,
		client.cliCtx.TxConfig,
		factory.Sequence(),
	)
	if err != nil {
		return err
	}

	// Set the signatures
	err = builder.SetSignatures(sigs)
	if err != nil {
		return err
	}

	// Encode the transaction
	txBytes, err := client.cliCtx.TxConfig.TxEncoder()(builder.GetTx())
	if err != nil {
		return err
	}

	// Broadcast the transaction to a Tendermint node
	res, err := client.cliCtx.BroadcastTx(txBytes)
	if err != nil {
		return err
	}

	return client.cliCtx.PrintProto(res)
}

At the same time, I was trying to sign the same transaction with the following CLI command:

Sign with Sign method code

desmos tx bank send faucet desmos1kmw9et4e99ascgdw0mmkt63mggjuu0xuqjx30w 100000udaric --chain-id morpheus-13001 --yes --node http://172.105.247.238:26657 --fees 50000udaric

I made sure that the account address, number and sequence were the same. However, using SignWithPrivKey kept returning the following error when I tried broadcasting the transaction:

signature verification failed; please verify account number (4) and chain-id (morpheus-13001): unauthorized

After debugging the behavior of Sign and SignWithPrivKey, I noticed that Sign performs the following operations:

key, err := txf.keybase.Key(name)
if err != nil {
	return err
}
pubKey := key.GetPubKey()
signerData := authsigning.SignerData{
	ChainID:       txf.chainID,
	AccountNumber: txf.accountNumber,
	Sequence:      txf.sequence,
}

// For SIGN_MODE_DIRECT, calling SetSignatures calls setSignerInfos on
// TxBuilder under the hood, and SignerInfos is needed to generated the
// sign bytes. This is the reason for setting SetSignatures here, with a
// nil signature.
//
// Note: this line is not needed for SIGN_MODE_LEGACY_AMINO, but putting it
// also doesn't affect its generated sign bytes, so for code's simplicity
// sake, we put it here.
sigData := signing.SingleSignatureData{
	SignMode:  signMode,
	Signature: nil,
}
sig := signing.SignatureV2{
	PubKey:   pubKey,
	Data:     &sigData,
	Sequence: txf.Sequence(),
}
var prevSignatures []signing.SignatureV2
if !overwriteSig {
	prevSignatures, err = txBuilder.GetTx().GetSignaturesV2()
	if err != nil {
		return err
	}
}
if err := txBuilder.SetSignatures(sig); err != nil {
	return err
}

As you can see, the Sign method adds a nil signature under the hood, so that it can properly sign the data using the SIGN_MODE_DIRECT method. This, however, is not done by the SignWithPrivKey method.

After I added the following code before calling SignWithPrivKey, everything worked properly:

sigData := signing.SingleSignatureData{
	SignMode:  signing.SignMode_SIGN_MODE_DIRECT,
	Signature: nil,
}
sig := signing.SignatureV2{
	PubKey:   client.privKey.PubKey(),
	Data:     &sigData,
	Sequence: factory.Sequence(),
}
err = builder.SetSignatures(sig)
if err != nil {
	return err
}

I think that it might be better to add the same code also to SignWithPrivKey, as it might be a method particularly used by third-parties that want to make sure they sign everything correctly.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[educlerici-zondax]

@tac0turtle can we take this issue?

[tac0turtle]

yea!! would need to check if its still the case since the issue is so old

[raynaudoe]

Just checked with a test if both signing methods are returning the same result, and in fact they are.
But SignWithPrivKey cannot be used as a direct replacement of Sign:

• Some test in tx_test like this one fails because SignWithPrivKey never calls txBuilder.SetSignatures(sigV2) and also it doesn't know about overwriteSig logic that Sign has.
• In fact, all tests that need to append signatures will fail

If that is a desirable fix, I'm happy to fix it and add a specific test that checks that both signatures match.

[alexanderbez]

Sorry @raynaudoe can you re-summarize the proposal? Are you suggesting to modify SignWithPrivKey to be functionally congruent with Sign?

[raynaudoe]

Sorry @raynaudoe can you re-summarize the proposal? Are you suggesting to modify SignWithPrivKey to be functionally congruent with Sign?

yes, I meant that by pointing out that the same tests that run with Sign won't work with SignWithPrivKey

[alexanderbez]

Yeah, let's fix that and ensure they're in line.

[tac0turtle]

Chiming in here. We should have one sign function, we should better understand why there are two and see if we can deprecate one. If we have to ways of signing that do the samething one should be removed not fixed to match the other. Please always think of less code is less to maintain.

[raynaudoe]

Chiming in here. We should have one sign function, we should better understand why there are two and see if we can deprecate one. If we have to ways of signing that do the samething one should be removed not fixed to match the other. Please always think of less code is less to maintain.

Absolutely, I'll check that

[alexanderbez]

IIRC, there is one way to sign. But we provide methods with explicit and implicit usage of private keys.

[raynaudoe]

Both Sign and SignWithPrivKey perform similar steps for signing, but they are not interchangeable, as I previously mentioned. Otherwise, the tests in tx_test.go would function correctly when replacing Sign with the other function. Since SignWithPrivKey is only used in test files like this one, it might be a good idea to remove that function and refactor those tests to use a test keystore and call Sign.

Another option, if the intention is to keep both, is to have Sign internally call SignWithPrivKey. This way, they would be layered instead of being two alternative methods of signing.

[julienrbrt]

Another option, if the intention is to keep both, is to have Sign internally call SignWithPrivKey. This way, they would be layered instead of being two alternative methods of signing.

After a quick search on sourcegraph this function is definitely used:

context:global lang:Go -file:(^|/)vendor/ -file:_test\.go$ file:has.content(github.com/cosmos/cosmos-sdk) SignWithPrivKey

https://sourcegraph.com/search?q=context:global+lang:Go+-file:%28%5E%7C/%29vendor/+-file:_test%5C.go%24+file:has.content%28github.com/cosmos/cosmos-sdk%29++SignWithPrivKey&patternType=standard&sm=1&groupBy=repo

[raynaudoe]

ok, so refactoring it would have a considerable impact on those repos.
So, back to the issue posted by op, the signed bytes are the same in the end on both functions, so in the context of this issue the bug is there no more. Should we close this issue then ?

[tac0turtle]

works with me. In the future they may both be deprecated so may not be worth doing it now