Skip to content

Commit

Permalink
fix: sequence should be higher or equal than expected during checktx …
Browse files Browse the repository at this point in the history
…and equal during deliver tx (#22299)

Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>
  • Loading branch information
3 people authored Oct 17, 2024
1 parent 29ea9c8 commit c1707b8
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 22 deletions.
14 changes: 0 additions & 14 deletions tests/systemtests/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,6 @@ func TestAuthSignAndBroadcastTxCmd(t *testing.T) {
sendAmount := transferAmount + newAmount
fees := feeAmount * 2

// TODO: remove below block code once v2 supports multi messages
// ref: https://github.com/cosmos/cosmos-sdk/issues/22215
if isV2() {
sendAmount = transferAmount
fees = feeAmount
}

testSignTxBroadcast(t, cli, signBatchCmd, "sign-batch tx", val1Addr, val2Addr, sendAmount, fees)
}

Expand Down Expand Up @@ -304,13 +297,6 @@ func TestAuthMultisigTxCmds(t *testing.T) {
sendAmount := transferAmount * 2
fees := feeAmount * 2

// TODO: remove below block code once v2 supports multi messages
// ref: https://github.com/cosmos/cosmos-sdk/issues/22215
if isV2() {
sendAmount = transferAmount
fees = feeAmount
}

testMultisigTxBroadcast(t, cli, multiSigTxInput{
"multisign-batch",
multiAddr,
Expand Down
11 changes: 10 additions & 1 deletion x/accounts/defaults/base/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"cosmossdk.io/collections"
"cosmossdk.io/core/address"
"cosmossdk.io/core/header"
"cosmossdk.io/core/transaction"
"cosmossdk.io/x/accounts/accountstd"
v1 "cosmossdk.io/x/accounts/defaults/base/v1"
aa_interface_v1 "cosmossdk.io/x/accounts/interfaces/account_abstraction/v1"
Expand Down Expand Up @@ -43,6 +44,7 @@ func NewAccount(name string, handlerMap *signing.HandlerMap, options ...Option)
Sequence: collections.NewSequence(deps.SchemaBuilder, SequencePrefix, "sequence"),
addrCodec: deps.AddressCodec,
hs: deps.Environment.HeaderService,
ts: deps.Environment.TransactionService,
supportedPubKeys: map[string]pubKeyImpl{},
signingHandlers: handlerMap,
}
Expand All @@ -65,6 +67,7 @@ type Account struct {

addrCodec address.Codec
hs header.Service
ts transaction.Service

supportedPubKeys map[string]pubKeyImpl

Expand Down Expand Up @@ -106,7 +109,13 @@ func (a Account) Authenticate(ctx context.Context, msg *aa_interface_v1.MsgAuthe
}

gotSeq := msg.Tx.AuthInfo.SignerInfos[msg.SignerIndex].Sequence
if gotSeq != signerData.Sequence {

execMode := a.ts.ExecMode(ctx)
if execMode == transaction.ExecModeCheck {
if gotSeq < signerData.Sequence {
return nil, fmt.Errorf("sequence number must be higher than: %d, got: %d", signerData.Sequence, gotSeq)
}
} else if gotSeq != signerData.Sequence {
return nil, fmt.Errorf("unexpected sequence number, wanted: %d, got: %d", signerData.Sequence, gotSeq)
}

Expand Down
12 changes: 10 additions & 2 deletions x/accounts/defaults/base/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ func newMockContext(t *testing.T) (context.Context, store.KVStoreService) {
)
}

type transactionService struct {
}

func (t transactionService) ExecMode(ctx context.Context) transaction.ExecMode {
return transaction.ExecModeFinalize
}

func makeMockDependencies(storeservice store.KVStoreService) accountstd.Dependencies {
sb := collections.NewSchemaBuilder(storeservice)

Expand All @@ -74,8 +81,9 @@ func makeMockDependencies(storeservice store.KVStoreService) accountstd.Dependen
AddressCodec: addressCodec{},
LegacyStateCodec: mockStateCodec{},
Environment: appmodulev2.Environment{
EventService: eventService{},
HeaderService: headerService{},
EventService: eventService{},
HeaderService: headerService{},
TransactionService: transactionService{},
},
}
}
Expand Down
4 changes: 4 additions & 0 deletions x/auth/ante/ante_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,8 @@ func TestAnteHandlerBadSignBytes(t *testing.T) {
suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
require.NoError(t, accs[0].acc.SetSequence(2)) // wrong accSeq

suite.ctx = suite.ctx.WithExecMode(sdk.ExecModeFinalize)

return TestCaseArgs{
chainID: suite.ctx.ChainID(),
feeAmount: feeAmount,
Expand Down Expand Up @@ -1081,6 +1083,8 @@ func TestAnteHandlerSetPubKey(t *testing.T) {
accs := suite.CreateTestAccounts(2)
suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

suite.ctx = suite.ctx.WithExecMode(sdk.ExecModeFinalize)

// Make sure public key has not been set from previous test.
acc1 := suite.accountKeeper.GetAccount(suite.ctx, accs[1].acc.GetAddress())
require.Nil(t, acc1.GetPubKey())
Expand Down
16 changes: 12 additions & 4 deletions x/auth/ante/sigverify.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,17 +306,25 @@ func (svd SigVerificationDecorator) consumeSignatureGas(

// verifySig will verify the signature of the provided signer account.
func (svd SigVerificationDecorator) verifySig(ctx context.Context, tx sdk.Tx, acc sdk.AccountI, sig signing.SignatureV2, newlyCreated bool) error {
if sig.Sequence != acc.GetSequence() {
execMode := svd.ak.GetEnvironment().TransactionService.ExecMode(ctx)
if execMode == transaction.ExecModeCheck {
if sig.Sequence < acc.GetSequence() {
return errorsmod.Wrapf(
sdkerrors.ErrWrongSequence,
"account sequence mismatch, expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence,
)
}
} else if sig.Sequence != acc.GetSequence() {
return errorsmod.Wrapf(
sdkerrors.ErrWrongSequence,
"account sequence mismatch, expected %d, got %d", acc.GetSequence(), sig.Sequence,
"account sequence mismatch: expected %d, got %d", acc.GetSequence(), sig.Sequence,
)
}

// we're in simulation mode, or in ReCheckTx, or context is not
// on sig verify tx, then we do not need to verify the signatures
// in the tx.
if svd.ak.GetEnvironment().TransactionService.ExecMode(ctx) == transaction.ExecModeSimulate ||
if execMode == transaction.ExecModeSimulate ||
isRecheckTx(ctx, svd.ak.GetEnvironment().TransactionService) ||
!isSigverifyTx(ctx) {
return nil
Expand Down Expand Up @@ -352,7 +360,7 @@ func (svd SigVerificationDecorator) verifySig(ctx context.Context, tx sdk.Tx, ac
Address: acc.GetAddress().String(),
ChainID: chainID,
AccountNumber: accNum,
Sequence: acc.GetSequence(),
Sequence: sig.Sequence,
PubKey: &anypb.Any{
TypeUrl: anyPk.TypeUrl,
Value: anyPk.Value,
Expand Down
2 changes: 1 addition & 1 deletion x/auth/ante/sigverify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func TestSigVerification(t *testing.T) {
if tc.recheck {
ctx = ctx.WithExecMode(sdk.ExecModeReCheck)
} else {
ctx = ctx.WithExecMode(sdk.ExecModeCheck)
ctx = ctx.WithExecMode(sdk.ExecModeFinalize)
}

suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() // Create new txBuilder for each test
Expand Down

0 comments on commit c1707b8

Please sign in to comment.