Add `SkipSequenceCheck` flag in SignatureV2 for Amino signatures (#7234)

* Add SkipSequenceCheck

* Fix test

* Fix test

* Add explicit amino test

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

client/testutil/suite.go

@@ -239,6 +239,7 @@ 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()

types/tx/signing/signature.go

@@ -19,8 +19,23 @@ type SignatureV2 struct {
 	// the signatures themselves for either single or multi-signatures.
 	Data SignatureData
 
-	// Sequence is the sequence of this account.
+	// 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

@@ -207,12 +207,18 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 		}
 
 		// Check account sequence number.
+		// 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 {
 			if sig.Sequence != acc.GetSequence() {
 				return ctx, sdkerrors.Wrapf(
 					sdkerrors.ErrWrongSequence,
 					"account sequence mismatch, expected %d, got %d", acc.GetSequence(), sig.Sequence,
 				)
 			}
+		}
 
 		// retrieve signer data
 		genesis := ctx.BlockHeight() == 0
@@ -230,9 +236,14 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 		if !simulate {
 			err := authsigning.VerifySignature(pubKey, signerData, sig.Data, svd.signModeHandler, tx)
 			if err != nil {
-				return ctx, sdkerrors.Wrapf(
-					sdkerrors.ErrUnauthorized,
-					"signature verification failed; please verify account number (%d) and chain-id (%s)", accNum, chainID)
+				var errMsg string
+				if sig.SkipSequenceCheck {
+					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)
+				}
+				return ctx, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, errMsg)
+
 			}
 		}
 	}

x/auth/ante/sigverify_test.go

@@ -6,6 +6,8 @@ import (
 	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/ed25519"
 
+	"github.com/cosmos/cosmos-sdk/client"
+	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
 	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	"github.com/cosmos/cosmos-sdk/simapp"
@@ -14,6 +16,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 )
 
 func (suite *AnteTestSuite) TestSetPubKey() {
@@ -179,6 +182,91 @@ func (suite *AnteTestSuite) TestSigVerification() {
 	}
 }
 
+// This test is exactly like the one above, but we set the codec explicitly to
+// Amino.
+// Once https://github.com/cosmos/cosmos-sdk/issues/6190 is in, we can remove
+// this, since it'll be handled by the test matrix.
+// In the meantime, we want to make double-sure amino compatibility works.
+// ref: https://github.com/cosmos/cosmos-sdk/issues/7229
+func (suite *AnteTestSuite) TestSigVerification_ExplicitAmino() {
+	suite.app, suite.ctx = createTestApp(true)
+	suite.ctx = suite.ctx.WithBlockHeight(1)
+
+	// Set up TxConfig.
+	aminoCdc := codec.New()
+	// We're using TestMsg amino encoding in some tests, so register it here.
+	txConfig := authtypes.StdTxConfig{Cdc: aminoCdc}
+
+	suite.clientCtx = client.Context{}.
+		WithTxConfig(txConfig)
+
+	suite.anteHandler = ante.NewAnteHandler(suite.app.AccountKeeper, suite.app.BankKeeper, ante.DefaultSigVerificationGasConsumer, txConfig.SignModeHandler())
+
+	suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()
+
+	// make block height non-zero to ensure account numbers part of signBytes
+	suite.ctx = suite.ctx.WithBlockHeight(1)
+
+	// keys and addresses
+	priv1, _, addr1 := testdata.KeyTestPubAddr()
+	priv2, _, addr2 := testdata.KeyTestPubAddr()
+	priv3, _, addr3 := testdata.KeyTestPubAddr()
+
+	addrs := []sdk.AccAddress{addr1, addr2, addr3}
+
+	msgs := make([]sdk.Msg, len(addrs))
+	// set accounts and create msg for each address
+	for i, addr := range addrs {
+		acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr)
+		suite.Require().NoError(acc.SetAccountNumber(uint64(i)))
+		suite.app.AccountKeeper.SetAccount(suite.ctx, acc)
+		msgs[i] = testdata.NewTestMsg(addr)
+	}
+
+	feeAmount := testdata.NewTestFeeAmount()
+	gasLimit := testdata.NewTestGasLimit()
+
+	spkd := ante.NewSetPubKeyDecorator(suite.app.AccountKeeper)
+	svd := ante.NewSigVerificationDecorator(suite.app.AccountKeeper, suite.clientCtx.TxConfig.SignModeHandler())
+	antehandler := sdk.ChainAnteDecorators(spkd, svd)
+
+	type testCase struct {
+		name      string
+		privs     []crypto.PrivKey
+		accNums   []uint64
+		accSeqs   []uint64
+		recheck   bool
+		shouldErr bool
+	}
+	testCases := []testCase{
+		{"no signers", []crypto.PrivKey{}, []uint64{}, []uint64{}, false, true},
+		{"not enough signers", []crypto.PrivKey{priv1, priv2}, []uint64{0, 1}, []uint64{0, 0}, false, true},
+		{"wrong order signers", []crypto.PrivKey{priv3, priv2, priv1}, []uint64{2, 1, 0}, []uint64{0, 0, 0}, false, true},
+		{"wrong accnums", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{7, 8, 9}, []uint64{0, 0, 0}, false, true},
+		{"wrong sequences", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{3, 4, 5}, false, true},
+		{"valid tx", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{0, 0, 0}, false, false},
+		{"no err on recheck", []crypto.PrivKey{}, []uint64{}, []uint64{}, true, false},
+	}
+	for i, tc := range testCases {
+		suite.ctx = suite.ctx.WithIsReCheckTx(tc.recheck)
+		suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() // Create new txBuilder for each test
+
+		suite.Require().NoError(suite.txBuilder.SetMsgs(msgs...))
+		suite.txBuilder.SetFeeAmount(feeAmount)
+		suite.txBuilder.SetGasLimit(gasLimit)
+
+		tx, err := suite.CreateTestTx(tc.privs, tc.accNums, tc.accSeqs, suite.ctx.ChainID())
+		suite.Require().NoError(err)
+
+		_, err = antehandler(suite.ctx, tx, false)
+		if tc.shouldErr {
+			suite.Require().NotNil(err, "TestCase %d: %s did not error as expected", i, tc.name)
+		} else {
+			suite.Require().Nil(err, "TestCase %d: %s errored unexpectedly. Err: %v", i, tc.name, err)
+		}
+	}
+}
+
 func (suite *AnteTestSuite) TestSigIntegration() {
 	// generate private keys
 	privs := []crypto.PrivKey{secp256k1.GenPrivKey(), secp256k1.GenPrivKey(), secp256k1.GenPrivKey()}

x/auth/types/stdtx.go

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