diff --git a/types/tx/signing/signature.go b/types/tx/signing/signature.go index f8f9a8d3bf3a..9479134cd5da 100644 --- a/types/tx/signing/signature.go +++ b/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. diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 914ee20b07eb..05bc583b10aa 100644 --- a/x/auth/ante/sigverify.go +++ b/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) diff --git a/x/auth/client/rest/rest_test.go b/x/auth/client/rest/rest_test.go index a25a34e596c2..dd407fbef879 100644 --- a/x/auth/client/rest/rest_test.go +++ b/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] diff --git a/x/auth/testutil/suite.go b/x/auth/testutil/suite.go index b7fe6dc18be5..3f6e138e5af8 100644 --- a/x/auth/testutil/suite.go +++ b/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() diff --git a/x/auth/types/stdtx.go b/x/auth/types/stdtx.go index 004406966091..cdd9b01b4154 100644 --- a/x/auth/types/stdtx.go +++ b/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 }