Skip to content

Commit ee9959b

Browse files
mergify[bot]facundomedicajulienrbrt
authored
fix(auth): audit issues with unordered txs (backport #23392) (#23571)
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
1 parent 5a8777a commit ee9959b

10 files changed

Lines changed: 303 additions & 44 deletions

File tree

‎tests/systemtests/unordered_tx_test.go‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"testing"
88
"time"
99

10-
"github.com/stretchr/testify/assert"
1110
"github.com/stretchr/testify/require"
1211

1312
systest "cosmossdk.io/systemtests"
@@ -35,17 +34,14 @@ func TestUnorderedTXDuplicate(t *testing.T) {
3534
systest.Sut.StartChain(t)
3635

3736
timeoutTimestamp := time.Now().Add(time.Minute)
37+
3838
// send tokens
39-
rsp1 := cli.Run("tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from="+account1Addr, "--fees=1stake", fmt.Sprintf("--timeout-timestamp=%v", timeoutTimestamp.Unix()), "--unordered", "--sequence=1", "--note=1")
39+
args := []string{"tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from=" + account1Addr, "--fees=1stake", fmt.Sprintf("--timeout-timestamp=%v", timeoutTimestamp.Unix()), "--unordered", "--sequence=1"}
40+
rsp1 := cli.Run(args...)
4041
systest.RequireTxSuccess(t, rsp1)
4142

42-
assertDuplicateErr := func(xt assert.TestingT, gotErr error, gotOutputs ...interface{}) bool {
43-
require.Len(t, gotOutputs, 1)
44-
assert.Contains(t, gotOutputs[0], "is duplicated: invalid request")
45-
return false // always abort
46-
}
47-
rsp2 := cli.WithRunErrorMatcher(assertDuplicateErr).Run("tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from="+account1Addr, "--fees=1stake", fmt.Sprintf("--timeout-timestamp=%v", timeoutTimestamp.Unix()), "--unordered", "--sequence=1")
48-
systest.RequireTxFailure(t, rsp2)
43+
rsp2 := cli.RunCommandWithArgs(cli.WithTXFlags(args...)...)
44+
require.Contains(t, rsp2, "19") // tx already in mempool
4945

5046
require.Eventually(t, func() bool {
5147
return cli.QueryBalance(account2Addr, "stake") == 5000

‎types/mempool/mempool_test.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"math/rand"
66
"testing"
7+
"time"
78

89
"github.com/stretchr/testify/require"
910
"github.com/stretchr/testify/suite"
@@ -54,6 +55,21 @@ type testTx struct {
5455
address sdk.AccAddress
5556
// useful for debugging
5657
strAddress string
58+
unordered bool
59+
timeout *time.Time
60+
}
61+
62+
// GetTimeoutTimeStamp implements types.TxWithUnordered.
63+
func (tx testTx) GetTimeoutTimeStamp() time.Time {
64+
if tx.timeout == nil {
65+
return time.Time{}
66+
}
67+
return *tx.timeout
68+
}
69+
70+
// GetUnordered implements types.TxWithUnordered.
71+
func (tx testTx) GetUnordered() bool {
72+
return tx.unordered
5773
}
5874

5975
func (tx testTx) GetSigners() ([][]byte, error) { panic("not implemented") }
@@ -72,6 +88,7 @@ func (tx testTx) GetSignaturesV2() (res []txsigning.SignatureV2, err error) {
7288

7389
var (
7490
_ sdk.Tx = (*testTx)(nil)
91+
_ sdk.TxWithUnordered = (*testTx)(nil)
7592
_ signing.SigVerifiableTx = (*testTx)(nil)
7693
_ cryptotypes.PubKey = (*testPubKey)(nil)
7794
)

‎types/mempool/priority_nonce.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package mempool
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67
"math"
78
"sync"
@@ -223,13 +224,13 @@ func (mp *PriorityNonceMempool[C]) Insert(ctx context.Context, tx sdk.Tx) error
223224
priority := mp.cfg.TxPriority.GetTxPriority(ctx, tx)
224225
nonce := sig.Sequence
225226

226-
// if it's an unordered tx, we use the gas instead of the nonce
227+
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
227228
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
228-
gasLimit, err := unordered.GetGasLimit()
229-
nonce = gasLimit
230-
if err != nil {
231-
return err
229+
timestamp := unordered.GetTimeoutTimeStamp().Unix()
230+
if timestamp < 0 {
231+
return errors.New("invalid timestamp value")
232232
}
233+
nonce = uint64(timestamp)
233234
}
234235

235236
key := txMeta[C]{nonce: nonce, priority: priority, sender: sender}
@@ -468,13 +469,13 @@ func (mp *PriorityNonceMempool[C]) Remove(tx sdk.Tx) error {
468469
sender := sig.Signer.String()
469470
nonce := sig.Sequence
470471

471-
// if it's an unordered tx, we use the gas instead of the nonce
472+
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
472473
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
473-
gasLimit, err := unordered.GetGasLimit()
474-
nonce = gasLimit
475-
if err != nil {
476-
return err
474+
timestamp := unordered.GetTimeoutTimeStamp().Unix()
475+
if timestamp < 0 {
476+
return errors.New("invalid timestamp value")
477477
}
478+
nonce = uint64(timestamp)
478479
}
479480

480481
scoreKey := txMeta[C]{nonce: nonce, sender: sender}

‎types/mempool/priority_nonce_test.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -970,3 +970,40 @@ func TestNextSenderTx_TxReplacement(t *testing.T) {
970970
iter := mp.Select(ctx, nil)
971971
require.Equal(t, txs[3], iter.Tx())
972972
}
973+
974+
func TestPriorityNonceMempool_UnorderedTx(t *testing.T) {
975+
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
976+
accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 2)
977+
sa := accounts[0].Address
978+
sb := accounts[1].Address
979+
980+
mp := mempool.DefaultPriorityMempool()
981+
982+
now := time.Now()
983+
oneHour := now.Add(1 * time.Hour)
984+
thirtyMin := now.Add(30 * time.Minute)
985+
twoHours := now.Add(2 * time.Hour)
986+
fifteenMin := now.Add(15 * time.Minute)
987+
988+
txs := []testTx{
989+
{id: 1, priority: 0, address: sa, timeout: &thirtyMin, unordered: true},
990+
{id: 0, priority: 0, address: sa, timeout: &oneHour, unordered: true},
991+
{id: 3, priority: 0, address: sb, timeout: &fifteenMin, unordered: true},
992+
{id: 2, priority: 0, address: sb, timeout: &twoHours, unordered: true},
993+
}
994+
995+
for _, tx := range txs {
996+
c := ctx.WithPriority(tx.priority)
997+
require.NoError(t, mp.Insert(c, tx))
998+
}
999+
1000+
require.Equal(t, 4, mp.CountTx())
1001+
1002+
orderedTxs := fetchTxs(mp.Select(ctx, nil), 100000)
1003+
require.Equal(t, len(txs), len(orderedTxs))
1004+
1005+
// check order
1006+
for i, tx := range orderedTxs {
1007+
require.Equal(t, txs[i].id, tx.(testTx).id)
1008+
}
1009+
}

‎types/mempool/sender_nonce.go‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
crand "crypto/rand" // #nosec // crypto/rand is used for seed generation
66
"encoding/binary"
7+
"errors"
78
"fmt"
89
"math/rand" // #nosec // math/rand is used for random selection and seeded from crypto/rand
910
"sync"
@@ -139,21 +140,21 @@ func (snm *SenderNonceMempool) Insert(_ context.Context, tx sdk.Tx) error {
139140
sender := sdk.AccAddress(sig.PubKey.Address()).String()
140141
nonce := sig.Sequence
141142

143+
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
144+
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
145+
timestamp := unordered.GetTimeoutTimeStamp().Unix()
146+
if timestamp < 0 {
147+
return errors.New("invalid timestamp value")
148+
}
149+
nonce = uint64(timestamp)
150+
}
151+
142152
senderTxs, found := snm.senders[sender]
143153
if !found {
144154
senderTxs = skiplist.New(skiplist.Uint64)
145155
snm.senders[sender] = senderTxs
146156
}
147157

148-
// if it's an unordered tx, we use the gas instead of the nonce
149-
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
150-
gasLimit, err := unordered.GetGasLimit()
151-
nonce = gasLimit
152-
if err != nil {
153-
return err
154-
}
155-
}
156-
157158
senderTxs.Set(nonce, tx)
158159

159160
key := txKey{nonce: nonce, address: sender}
@@ -236,13 +237,13 @@ func (snm *SenderNonceMempool) Remove(tx sdk.Tx) error {
236237
sender := sdk.AccAddress(sig.PubKey.Address()).String()
237238
nonce := sig.Sequence
238239

239-
// if it's an unordered tx, we use the gas instead of the nonce
240+
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
240241
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
241-
gasLimit, err := unordered.GetGasLimit()
242-
nonce = gasLimit
243-
if err != nil {
244-
return err
242+
timestamp := unordered.GetTimeoutTimeStamp().Unix()
243+
if timestamp < 0 {
244+
return errors.New("invalid timestamp value")
245245
}
246+
nonce = uint64(timestamp)
246247
}
247248

248249
senderTxs, found := snm.senders[sender]

‎types/mempool/sender_nonce_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"math/rand"
66
"testing"
7+
"time"
78

89
"github.com/stretchr/testify/require"
910

@@ -192,3 +193,67 @@ func (s *MempoolTestSuite) TestTxNotFoundOnSender() {
192193
err = mp.Remove(tx)
193194
require.Equal(t, mempool.ErrTxNotFound, err)
194195
}
196+
197+
func (s *MempoolTestSuite) TestUnorderedTx() {
198+
t := s.T()
199+
200+
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
201+
accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 2)
202+
sa := accounts[0].Address
203+
sb := accounts[1].Address
204+
205+
mp := mempool.NewSenderNonceMempool(mempool.SenderNonceMaxTxOpt(5000))
206+
207+
now := time.Now()
208+
oneHour := now.Add(1 * time.Hour)
209+
thirtyMin := now.Add(30 * time.Minute)
210+
twoHours := now.Add(2 * time.Hour)
211+
fifteenMin := now.Add(15 * time.Minute)
212+
213+
txs := []testTx{
214+
{id: 0, address: sa, timeout: &oneHour, unordered: true},
215+
{id: 1, address: sa, timeout: &thirtyMin, unordered: true},
216+
{id: 2, address: sb, timeout: &twoHours, unordered: true},
217+
{id: 3, address: sb, timeout: &fifteenMin, unordered: true},
218+
}
219+
220+
for _, tx := range txs {
221+
c := ctx.WithPriority(tx.priority)
222+
require.NoError(t, mp.Insert(c, tx))
223+
}
224+
225+
require.Equal(t, 4, mp.CountTx())
226+
227+
orderedTxs := fetchTxs(mp.Select(ctx, nil), 100000)
228+
require.Equal(t, len(txs), len(orderedTxs))
229+
230+
// Because the sender is selected randomly it can be any of these options
231+
acceptableOptions := [][]int{
232+
{3, 1, 2, 0},
233+
{3, 1, 0, 2},
234+
{3, 2, 1, 0},
235+
{1, 3, 0, 2},
236+
{1, 3, 2, 0},
237+
{1, 0, 3, 2},
238+
}
239+
240+
orderedTxsIds := make([]int, len(orderedTxs))
241+
for i, tx := range orderedTxs {
242+
orderedTxsIds[i] = tx.(testTx).id
243+
}
244+
245+
anyAcceptableOrder := false
246+
for _, option := range acceptableOptions {
247+
for i, tx := range orderedTxs {
248+
if tx.(testTx).id != txs[option[i]].id {
249+
break
250+
}
251+
252+
if i == len(orderedTxs)-1 {
253+
anyAcceptableOrder = true
254+
}
255+
}
256+
}
257+
258+
require.True(t, anyAcceptableOrder, "expected any of %v but got %v", acceptableOptions, orderedTxsIds)
259+
}

‎x/auth/ante/ante_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"strings"
88
"testing"
9+
"time"
910

1011
"github.com/stretchr/testify/require"
1112
"go.uber.org/mock/gomock"
@@ -1384,3 +1385,34 @@ func TestAnteHandlerReCheck(t *testing.T) {
13841385
_, err = suite.anteHandler(suite.ctx, tx, false)
13851386
require.NotNil(t, err, "antehandler on recheck did not fail once feePayer no longer has sufficient funds")
13861387
}
1388+
1389+
func TestAnteHandlerUnorderedTx(t *testing.T) {
1390+
suite := SetupTestSuite(t, false)
1391+
accs := suite.CreateTestAccounts(1)
1392+
msg := testdata.NewTestMsg(accs[0].acc.GetAddress())
1393+
1394+
// First send a normal sequential tx with sequence 0
1395+
suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), accs[0].acc.GetAddress(), authtypes.FeeCollectorName, testdata.NewTestFeeAmount()).Return(nil).AnyTimes()
1396+
1397+
privs, accNums, accSeqs := []cryptotypes.PrivKey{accs[0].priv}, []uint64{1000}, []uint64{0}
1398+
_, err := suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false)
1399+
require.NoError(t, err)
1400+
1401+
// we try to send another tx with the same sequence, it will fail
1402+
_, err = suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false)
1403+
require.Error(t, err)
1404+
1405+
// now we'll still use the same sequence but because it's unordered, it will be ignored and accepted anyway
1406+
msgs := []sdk.Msg{msg}
1407+
require.NoError(t, suite.txBuilder.SetMsgs(msgs...))
1408+
suite.txBuilder.SetFeeAmount(testdata.NewTestFeeAmount())
1409+
suite.txBuilder.SetGasLimit(testdata.NewTestGasLimit())
1410+
1411+
tx, txErr := suite.CreateTestUnorderedTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), apisigning.SignMode_SIGN_MODE_DIRECT, true, time.Now().Add(time.Minute))
1412+
require.NoError(t, txErr)
1413+
txBytes, err := suite.clientCtx.TxConfig.TxEncoder()(tx)
1414+
bytesCtx := suite.ctx.WithTxBytes(txBytes)
1415+
require.NoError(t, err)
1416+
_, err = suite.anteHandler(bytesCtx, tx, false)
1417+
require.NoError(t, err)
1418+
}

‎x/auth/ante/sigverify.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -324,18 +324,24 @@ func (svd SigVerificationDecorator) consumeSignatureGas(
324324
// verifySig will verify the signature of the provided signer account.
325325
func (svd SigVerificationDecorator) verifySig(ctx context.Context, tx sdk.Tx, acc sdk.AccountI, sig signing.SignatureV2, newlyCreated bool) error {
326326
execMode := svd.ak.GetEnvironment().TransactionService.ExecMode(ctx)
327-
if execMode == transaction.ExecModeCheck {
328-
if sig.Sequence < acc.GetSequence() {
327+
unorderedTx, ok := tx.(sdk.TxWithUnordered)
328+
isUnordered := ok && unorderedTx.GetUnordered()
329+
330+
// only check sequence if the tx is not unordered
331+
if !isUnordered {
332+
if execMode == transaction.ExecModeCheck {
333+
if sig.Sequence < acc.GetSequence() {
334+
return errorsmod.Wrapf(
335+
sdkerrors.ErrWrongSequence,
336+
"account sequence mismatch: expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence,
337+
)
338+
}
339+
} else if sig.Sequence != acc.GetSequence() {
329340
return errorsmod.Wrapf(
330341
sdkerrors.ErrWrongSequence,
331-
"account sequence mismatch, expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence,
342+
"account sequence mismatch: expected %d, got %d", acc.GetSequence(), sig.Sequence,
332343
)
333344
}
334-
} else if sig.Sequence != acc.GetSequence() {
335-
return errorsmod.Wrapf(
336-
sdkerrors.ErrWrongSequence,
337-
"account sequence mismatch: expected %d, got %d", acc.GetSequence(), sig.Sequence,
338-
)
339345
}
340346

341347
// we're in simulation mode, or in ReCheckTx, or context is not

0 commit comments

Comments
 (0)
⚡