diff --git a/client/testutil/suite.go b/client/testutil/suite.go index 3f6e138e5a..b7fe6dc18b 100644 --- a/client/testutil/suite.go +++ b/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() diff --git a/types/tx/signing/signature.go b/types/tx/signing/signature.go index dd080bcb8e..f8f9a8d3bf 100644 --- a/types/tx/signing/signature.go +++ b/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. diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index e1d11ebc9b..7d1fea1ebe 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -207,11 +207,17 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul } // Check account sequence number. - if sig.Sequence != acc.GetSequence() { - return ctx, sdkerrors.Wrapf( - sdkerrors.ErrWrongSequence, - "account sequence mismatch, expected %d, got %d", acc.GetSequence(), sig.Sequence, - ) + // 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 @@ -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) + } } } diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index d757f720cf..3b8b5e02b3 100644 --- a/x/auth/ante/sigverify_test.go +++ b/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()} diff --git a/x/auth/types/stdtx.go b/x/auth/types/stdtx.go index 5f52b492cf..1ba33bc0e5 100644 --- a/x/auth/types/stdtx.go +++ b/x/auth/types/stdtx.go @@ -387,8 +387,9 @@ func StdSignatureToSignatureV2(cdc *codec.LegacyAmino, sig StdSignature) (signin } return signing.SignatureV2{ - PubKey: pk, - Data: data, + PubKey: pk, + Data: data, + SkipSequenceCheck: true, }, nil }