fix(auth): audit issues with unordered txs (#23392)

Co-authored-by: Alex | Interchain Labs <alex@interchainlabs.io>
Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
This commit is contained in:
Facundo Medica
2025-01-31 15:27:13 +00:00
committed by GitHub
co-authored by Alex | Interchain Labs Alexander Peters
parent 8eb6822d25
commit ddf9e18ee4
9 changed files with 296 additions and 35 deletions
+17
View File
@@ -5,6 +5,7 @@ import (
"fmt"
"math/rand"
"testing"
"time"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
@@ -55,6 +56,21 @@ type testTx struct {
address sdk.AccAddress
// useful for debugging
strAddress string
unordered bool
timeout *time.Time
}
// GetTimeoutTimeStamp implements types.TxWithUnordered.
func (tx testTx) GetTimeoutTimeStamp() time.Time {
if tx.timeout == nil {
return time.Time{}
}
return *tx.timeout
}
// GetUnordered implements types.TxWithUnordered.
func (tx testTx) GetUnordered() bool {
return tx.unordered
}
func (tx testTx) GetSigners() ([][]byte, error) { panic("not implemented") }
@@ -73,6 +89,7 @@ func (tx testTx) GetSignaturesV2() (res []txsigning.SignatureV2, err error) {
var (
_ sdk.Tx = (*testTx)(nil)
_ sdk.TxWithUnordered = (*testTx)(nil)
_ signing.SigVerifiableTx = (*testTx)(nil)
_ cryptotypes.PubKey = (*testPubKey)(nil)
)
+10 -10
View File
@@ -224,13 +224,13 @@ func (mp *PriorityNonceMempool[C]) Insert(ctx context.Context, tx sdk.Tx) error
priority := mp.cfg.TxPriority.GetTxPriority(ctx, tx)
nonce := sig.Sequence
// if it's an unordered tx, we use the gas instead of the nonce
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
gasLimit, err := unordered.GetGasLimit()
nonce = gasLimit
if err != nil {
return err
timestamp := unordered.GetTimeoutTimeStamp().Unix()
if timestamp < 0 {
return errors.New("invalid timestamp value")
}
nonce = uint64(timestamp)
}
key := txMeta[C]{nonce: nonce, priority: priority, sender: sender}
@@ -469,13 +469,13 @@ func (mp *PriorityNonceMempool[C]) Remove(tx sdk.Tx) error {
sender := sig.Signer.String()
nonce := sig.Sequence
// if it's an unordered tx, we use the gas instead of the nonce
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
gasLimit, err := unordered.GetGasLimit()
nonce = gasLimit
if err != nil {
return err
timestamp := unordered.GetTimeoutTimeStamp().Unix()
if timestamp < 0 {
return errors.New("invalid timestamp value")
}
nonce = uint64(timestamp)
}
scoreKey := txMeta[C]{nonce: nonce, sender: sender}
+37
View File
@@ -970,3 +970,40 @@ func TestNextSenderTx_TxReplacement(t *testing.T) {
iter := mp.Select(ctx, nil)
require.Equal(t, txs[3], iter.Tx())
}
func TestPriorityNonceMempool_UnorderedTx(t *testing.T) {
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 2)
sa := accounts[0].Address
sb := accounts[1].Address
mp := mempool.DefaultPriorityMempool()
now := time.Now()
oneHour := now.Add(1 * time.Hour)
thirtyMin := now.Add(30 * time.Minute)
twoHours := now.Add(2 * time.Hour)
fifteenMin := now.Add(15 * time.Minute)
txs := []testTx{
{id: 1, priority: 0, address: sa, timeout: &thirtyMin, unordered: true},
{id: 0, priority: 0, address: sa, timeout: &oneHour, unordered: true},
{id: 3, priority: 0, address: sb, timeout: &fifteenMin, unordered: true},
{id: 2, priority: 0, address: sb, timeout: &twoHours, unordered: true},
}
for _, tx := range txs {
c := ctx.WithPriority(tx.priority)
require.NoError(t, mp.Insert(c, tx))
}
require.Equal(t, 4, mp.CountTx())
orderedTxs := fetchTxs(mp.Select(ctx, nil), 100000)
require.Equal(t, len(txs), len(orderedTxs))
// check order
for i, tx := range orderedTxs {
require.Equal(t, txs[i].id, tx.(testTx).id)
}
}
+14 -14
View File
@@ -139,21 +139,21 @@ func (snm *SenderNonceMempool) Insert(_ context.Context, tx sdk.Tx) error {
sender := sdk.AccAddress(sig.PubKey.Address()).String()
nonce := sig.Sequence
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
timestamp := unordered.GetTimeoutTimeStamp().Unix()
if timestamp < 0 {
return errors.New("invalid timestamp value")
}
nonce = uint64(timestamp)
}
senderTxs, found := snm.senders[sender]
if !found {
senderTxs = skiplist.New(skiplist.Uint64)
snm.senders[sender] = senderTxs
}
// if it's an unordered tx, we use the gas instead of the nonce
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
gasLimit, err := unordered.GetGasLimit()
nonce = gasLimit
if err != nil {
return err
}
}
senderTxs.Set(nonce, tx)
key := txKey{nonce: nonce, address: sender}
@@ -236,13 +236,13 @@ func (snm *SenderNonceMempool) Remove(tx sdk.Tx) error {
sender := sdk.AccAddress(sig.PubKey.Address()).String()
nonce := sig.Sequence
// if it's an unordered tx, we use the gas instead of the nonce
// if it's an unordered tx, we use the timeout timestamp instead of the nonce
if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() {
gasLimit, err := unordered.GetGasLimit()
nonce = gasLimit
if err != nil {
return err
timestamp := unordered.GetTimeoutTimeStamp().Unix()
if timestamp < 0 {
return errors.New("invalid timestamp value")
}
nonce = uint64(timestamp)
}
senderTxs, found := snm.senders[sender]
+65
View File
@@ -4,6 +4,7 @@ import (
"fmt"
"math/rand"
"testing"
"time"
"github.com/stretchr/testify/require"
@@ -192,3 +193,67 @@ func (s *MempoolTestSuite) TestTxNotFoundOnSender() {
err = mp.Remove(tx)
require.Equal(t, mempool.ErrTxNotFound, err)
}
func (s *MempoolTestSuite) TestUnorderedTx() {
t := s.T()
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 2)
sa := accounts[0].Address
sb := accounts[1].Address
mp := mempool.NewSenderNonceMempool(mempool.SenderNonceMaxTxOpt(5000))
now := time.Now()
oneHour := now.Add(1 * time.Hour)
thirtyMin := now.Add(30 * time.Minute)
twoHours := now.Add(2 * time.Hour)
fifteenMin := now.Add(15 * time.Minute)
txs := []testTx{
{id: 0, address: sa, timeout: &oneHour, unordered: true},
{id: 1, address: sa, timeout: &thirtyMin, unordered: true},
{id: 2, address: sb, timeout: &twoHours, unordered: true},
{id: 3, address: sb, timeout: &fifteenMin, unordered: true},
}
for _, tx := range txs {
c := ctx.WithPriority(tx.priority)
require.NoError(t, mp.Insert(c, tx))
}
require.Equal(t, 4, mp.CountTx())
orderedTxs := fetchTxs(mp.Select(ctx, nil), 100000)
require.Equal(t, len(txs), len(orderedTxs))
// Because the sender is selected randomly it can be any of these options
acceptableOptions := [][]int{
{3, 1, 2, 0},
{3, 1, 0, 2},
{3, 2, 1, 0},
{1, 3, 0, 2},
{1, 3, 2, 0},
{1, 0, 3, 2},
}
orderedTxsIds := make([]int, len(orderedTxs))
for i, tx := range orderedTxs {
orderedTxsIds[i] = tx.(testTx).id
}
anyAcceptableOrder := false
for _, option := range acceptableOptions {
for i, tx := range orderedTxs {
if tx.(testTx).id != txs[option[i]].id {
break
}
if i == len(orderedTxs)-1 {
anyAcceptableOrder = true
}
}
}
require.True(t, anyAcceptableOrder, "expected any of %v but got %v", acceptableOptions, orderedTxsIds)
}