Fix sequence number handling for LegacyAmino > SignatureV2 (#7285)

* add multi-sequence ante_test with explicit amino, test out alternative without SkipSequenceCheck

* add attempt at rest based test for full transactions

* drop extraneous ante_handler explicit amino test

* add rest handler test for multiple broadcasts, remove SkipSequenceCheck flag

* add godoc & cleanups

* add test case for incorrect sequence number

* remove artifact files

* Update x/auth/ante/sigverify.go

Co-authored-by: Zaki Manian <zaki@manian.org>

* Update sigverify.go

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Zaki Manian <zaki@manian.org>

types/tx/signing/signature.go

@@ -22,20 +22,6 @@ type SignatureV2 struct {
 	// Sequence is the sequence of this account. Only populated in
 	// SIGN_MODE_DIRECT.
 	Sequence uint64
-
-	// Ugly flag to keep backwards-compatibility with Amino StdSignatures.
-	// In SIGN_MODE_DIRECT, sequence is in AuthInfo, and will thus be populated
-	// in the Sequence field above. The ante handler then checks this Sequence
-	// with the actual sequence on-chain.
-	// In SIGN_MODE_LEGACY_AMINO_JSON, sequence is signed via StdSignDoc, and
-	// checked during signature verification. It's not populated in the
-	// Sequence field above. This flag indicates that the Sequence field should
-	// be skipped in ante handlers.
-	// TLDR;
-	// - false (by default) in SIGN_MODE_DIRECT
-	// - true in SIGN_MODE_LEGACY_AMINO_JSON
-	// ref: https://github.com/cosmos/cosmos-sdk/issues/7229
-	SkipSequenceCheck bool
 }
 
 // SignatureDataToProto converts a SignatureData to SignatureDescriptor_Data.

x/auth/ante/sigverify.go

@@ -172,6 +172,27 @@ func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler authsigning.S
 	}
 }
 
+// OnlyLegacyAminoSigners checks SignatureData to see if all
+// signers are using SIGN_MODE_LEGACY_AMINO_JSON. If this is the case
+// then the corresponding SignatureV2 struct will not have account sequence
+// explicitly set, and we should skip the explicit verification of sig.Sequence
+// in the SigVerificationDecorator's AnteHandler function.
+func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool {
+	switch v := sigData.(type) {
+	case *signing.SingleSignatureData:
+		return v.SignMode == signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON
+	case *signing.MultiSignatureData:
+		for _, s := range v.Signatures {
+			if !OnlyLegacyAminoSigners(s) {
+				return false
+			}
+		}
+		return true
+	default:
+		return false
+	}
+}
+
 func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
 	// no need to verify signatures on recheck tx
 	if ctx.IsReCheckTx() {
@@ -212,8 +233,9 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 		// When using Amino StdSignatures, we actually don't have the Sequence in
 		// the SignatureV2 struct (it's only in the SignDoc). In this case, we
 		// cannot check sequence directly, and must do it via signature
-		// verification.
-		if !sig.SkipSequenceCheck {
+		// verification (in the VerifySignature call below).
+		onlyAminoSigners := OnlyLegacyAminoSigners(sig.Data)
+		if !onlyAminoSigners {
 			if sig.Sequence != acc.GetSequence() {
 				return ctx, sdkerrors.Wrapf(
 					sdkerrors.ErrWrongSequence,
@@ -239,7 +261,9 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 			err := authsigning.VerifySignature(pubKey, signerData, sig.Data, svd.signModeHandler, tx)
 			if err != nil {
 				var errMsg string
-				if sig.SkipSequenceCheck {
+				if onlyAminoSigners {
+					// If all signers are using SIGN_MODE_LEGACY_AMINO, we rely on VerifySignature to check account sequence number,
+					// and therefore communicate sequence number as a potential cause of error.
 					errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s)", accNum, acc.GetSequence(), chainID)
 				} else {
 					errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d) and chain-id (%s)", accNum, chainID)

x/auth/client/rest/rest_test.go

@@ -4,6 +4,10 @@ package rest_test
 
 import (
 	"fmt"
+	"github.com/cosmos/cosmos-sdk/client/tx"
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+	"github.com/cosmos/cosmos-sdk/types/tx/signing"
+	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 	"testing"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -105,6 +109,90 @@ func (s *IntegrationTestSuite) TestBroadcastTxRequest() {
 	s.Require().NotEmpty(txRes.TxHash)
 }
 
+func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() {
+
+	val0 := s.network.Validators[0]
+	txConfig := authtypes.StdTxConfig{Cdc: s.cfg.LegacyAmino}
+
+	// First test transaction from validator should have sequence=1 (non-genesis tx)
+	testCases := []struct {
+		desc      string
+		sequence  uint64
+		shouldErr bool
+	}{
+		{
+			"First tx (correct sequence)",
+			1,
+			false,
+		},
+		{
+			"Second tx (correct sequence)",
+			2,
+			false,
+		},
+		{
+			"Third tx (incorrect sequence)",
+			9,
+			true,
+		},
+	}
+	for _, tc := range testCases {
+		s.Run(fmt.Sprintf("Case %s", tc.desc), func() {
+
+			msg := &types.MsgSend{
+				val0.Address,
+				val0.Address,
+				sdk.Coins{sdk.NewInt64Coin("foo", 100)},
+			}
+
+			// prepare txBuilder with msg
+			txBuilder := txConfig.NewTxBuilder()
+			feeAmount := sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, 10)}
+			gasLimit := testdata.NewTestGasLimit()
+			txBuilder.SetMsgs(msg)
+			txBuilder.SetFeeAmount(feeAmount)
+			txBuilder.SetGasLimit(gasLimit)
+
+			// setup txFactory
+			txFactory := tx.Factory{}
+			txFactory = txFactory.
+				WithChainID(val0.ClientCtx.ChainID).
+				WithKeybase(val0.ClientCtx.Keyring).
+				WithTxConfig(txConfig).
+				WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON).
+				WithSequence(tc.sequence)
+
+			// sign Tx (offline mode so we can manually set sequence number)
+			err := authclient.SignTx(txFactory, val0.ClientCtx, val0.Moniker, txBuilder, true)
+			s.Require().NoError(err)
+
+			// broadcast test with sync mode, as we want to run CheckTx to verify account sequence is correct
+			stdTx := txBuilder.GetTx().(authtypes.StdTx)
+			res, err := s.broadcastReq(stdTx, "sync")
+			s.Require().NoError(err)
+
+			var txRes sdk.TxResponse
+			// NOTE: this uses amino explicitly, don't migrate it!
+			s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &txRes))
+			// we check for a exitCode=0, indicating a successful broadcast
+			if tc.shouldErr {
+				var sigVerifyFailureCode uint32 = 4
+				s.Require().Equal(sigVerifyFailureCode, txRes.Code,
+					"Testcase '%s': Expected signature verification failure {Code: %d} from TxResponse. "+
+						"Found {Code: %d, RawLog: '%v'}",
+					tc.desc, sigVerifyFailureCode, txRes.Code, txRes.RawLog,
+				)
+			} else {
+				s.Require().Equal(uint32(0), txRes.Code,
+					"Testcase '%s': TxResponse errored unexpectedly. Err: {Code: %d, RawLog: '%v'}",
+					tc.desc, txRes.Code, txRes.RawLog,
+				)
+			}
+		})
+	}
+
+}
+
 func (s *IntegrationTestSuite) broadcastReq(stdTx authtypes.StdTx, mode string) ([]byte, error) {
 	val := s.network.Validators[0]
 

x/auth/testutil/suite.go

@@ -239,7 +239,6 @@ func (s *TxConfigTestSuite) TestTxEncodeDecode() {
 			SignMode:  signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON,
 			Signature: dummySig,
 		},
-		SkipSequenceCheck: s.TxConfig.SignModeHandler().DefaultMode() == signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON,
 	}
 
 	txBuilder := s.TxConfig.NewTxBuilder()

x/auth/types/stdtx.go

@@ -395,9 +395,8 @@ func StdSignatureToSignatureV2(cdc *codec.LegacyAmino, sig StdSignature) (signin
 	}
 
 	return signing.SignatureV2{
-		PubKey:            pk,
-		Data:              data,
-		SkipSequenceCheck: true,
+		PubKey: pk,
+		Data:   data,
 	}, nil
 }