From 3347cbee70c86c3f6687b28e9195da063a2698bb Mon Sep 17 00:00:00 2001 From: aman035 Date: Wed, 19 Aug 2026 18:52:29 +0530 Subject: [PATCH] fix: check outbound nonce against the key that signed, not the current TSS (F-2026-18827) --- universalClient/tss/tss.go | 9 -- .../tss/txbroadcaster/broadcaster.go | 3 - .../tss/txbroadcaster/broadcaster_test.go | 127 +++++++++++------- universalClient/tss/txbroadcaster/evm.go | 16 +-- universalClient/tss/txflow/parse.go | 34 +++++ universalClient/tss/txflow/parse_test.go | 102 ++++++++++++++ universalClient/tss/txresolver/evm.go | 25 ++-- universalClient/tss/txresolver/resolver.go | 3 - .../tss/txresolver/resolver_test.go | 116 ++++++++++------ 9 files changed, 307 insertions(+), 128 deletions(-) create mode 100644 universalClient/tss/txflow/parse_test.go diff --git a/universalClient/tss/tss.go b/universalClient/tss/tss.go index 215f5dfb..525c2d8d 100644 --- a/universalClient/tss/tss.go +++ b/universalClient/tss/tss.go @@ -239,20 +239,12 @@ func NewNode(ctx context.Context, cfg Config) (*Node, error) { registeredPeers: make(map[string]bool), } - getTSSAddress := func(ctx context.Context) (string, error) { - if node.coordinator == nil { - return "", fmt.Errorf("coordinator not initialized") - } - return node.coordinator.GetTSSAddress(ctx) - } - node.txResolver = txresolver.NewResolver(txresolver.Config{ EventStore: evtStore, Chains: cfg.Chains, PushSigner: cfg.PushSigner, CheckInterval: sessionExpiryCheckInterval, Logger: logger, - GetTSSAddress: getTSSAddress, }) node.txBroadcaster = txbroadcaster.NewBroadcaster(txbroadcaster.Config{ @@ -260,7 +252,6 @@ func NewNode(ctx context.Context, cfg Config) (*Node, error) { Chains: cfg.Chains, CheckInterval: sessionExpiryCheckInterval, Logger: logger, - GetTSSAddress: getTSSAddress, }) node.expirySweeper = expirysweeper.NewSweeper(expirysweeper.Config{ diff --git a/universalClient/tss/txbroadcaster/broadcaster.go b/universalClient/tss/txbroadcaster/broadcaster.go index b5b99c4c..759758d9 100644 --- a/universalClient/tss/txbroadcaster/broadcaster.go +++ b/universalClient/tss/txbroadcaster/broadcaster.go @@ -18,7 +18,6 @@ type Config struct { Chains *chains.Chains CheckInterval time.Duration Logger zerolog.Logger - GetTSSAddress func(ctx context.Context) (string, error) } type Broadcaster struct { @@ -26,7 +25,6 @@ type Broadcaster struct { chains *chains.Chains checkInterval time.Duration logger zerolog.Logger - getTSSAddress func(ctx context.Context) (string, error) } func NewBroadcaster(cfg Config) *Broadcaster { @@ -39,7 +37,6 @@ func NewBroadcaster(cfg Config) *Broadcaster { chains: cfg.Chains, checkInterval: interval, logger: cfg.Logger.With().Str("component", "txbroadcaster").Logger(), - getTSSAddress: cfg.GetTSSAddress, } } diff --git a/universalClient/tss/txbroadcaster/broadcaster_test.go b/universalClient/tss/txbroadcaster/broadcaster_test.go index 8ce0abca..727a4688 100644 --- a/universalClient/tss/txbroadcaster/broadcaster_test.go +++ b/universalClient/tss/txbroadcaster/broadcaster_test.go @@ -11,6 +11,7 @@ import ( "time" "unsafe" + "github.com/ethereum/go-ethereum/crypto" "github.com/rs/zerolog" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -25,6 +26,7 @@ import ( "github.com/pushchain/push-chain-node/universalClient/chains/common" "github.com/pushchain/push-chain-node/universalClient/config" "github.com/pushchain/push-chain-node/universalClient/store" + "github.com/pushchain/push-chain-node/universalClient/tss/coordinator" "github.com/pushchain/push-chain-node/universalClient/tss/eventstore" "github.com/pushchain/push-chain-node/universalClient/tss/txflow" ) @@ -117,10 +119,27 @@ func newTestChains(t *testing.T, chainID string, vmType uregistrytypes.VmType, c return c } +// testBroadcastSigningKeyHex signs the outbound fixtures. The broadcaster derives +// the nonce domain from the signature, so it has to be a real one. +const testBroadcastSigningKeyHex = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318" + +// testBroadcastSigner is the address recovered from those fixtures, i.e. the +// nonce domain the broadcaster must query. +var testBroadcastSigner = func() string { + key, _ := crypto.HexToECDSA(testBroadcastSigningKeyHex) + addr, _ := coordinator.DeriveEVMAddressFromPubkey(hex.EncodeToString(crypto.CompressPubkey(&key.PublicKey))) + return addr +}() + func makeSignedOutboundData(t *testing.T, destChain string, nonce uint64) []byte { t.Helper() - sig := hex.EncodeToString(make([]byte, 64)) - hash := hex.EncodeToString(make([]byte, 32)) + key, err := crypto.HexToECDSA(testBroadcastSigningKeyHex) + require.NoError(t, err) + hashBytes := crypto.Keccak256([]byte("test outbound signing hash")) + sigBytes, err := crypto.Sign(hashBytes, key) + require.NoError(t, err) + sig := hex.EncodeToString(sigBytes) + hash := hex.EncodeToString(hashBytes) data := txflow.SignedOutboundData{ OutboundCreatedEvent: uexecutortypes.OutboundCreatedEvent{ TxID: "tx-123", @@ -197,14 +216,12 @@ func getEvent(t *testing.T, db *gorm.DB, eventID string) store.Event { return ev } -func newBroadcaster(evtStore *eventstore.Store, ch *chains.Chains, tssAddr string) *Broadcaster { - getTSSAddr := func(ctx context.Context) (string, error) { return tssAddr, nil } +func newBroadcaster(evtStore *eventstore.Store, ch *chains.Chains) *Broadcaster { return NewBroadcaster(Config{ EventStore: evtStore, Chains: ch, CheckInterval: 0, // uses default, doesn't matter for direct calls Logger: zerolog.Nop(), - GetTSSAddress: getTSSAddr, }) } @@ -222,9 +239,9 @@ func TestEVM_BroadcastError_NonceConsumed_MarksBroadcasted(t *testing.T) { // VerifyBroadcastedTx=not found → fall through to the nonce-consumed check. builder.On("VerifyBroadcastedTx", mock.Anything, "0xabc"). Return(false, uint64(0), uint64(0), uint8(0), nil) - builder.On("GetNextNonce", mock.Anything, "0xTSS", true).Return(uint64(10), nil) + builder.On("GetNextNonce", mock.Anything, testBroadcastSigner, true).Return(uint64(10), nil) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -248,7 +265,7 @@ func TestEVM_BroadcastError_TxOnChain_MarksBroadcasted(t *testing.T) { builder.On("VerifyBroadcastedTx", mock.Anything, "0xabc"). Return(true, uint64(100), uint64(3), uint8(1), nil) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -269,7 +286,7 @@ func TestEVM_BroadcastSuccess_MarksBroadcasted(t *testing.T) { builder.On("BroadcastOutboundSigningRequest", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("0xabc123", nil) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -291,7 +308,7 @@ func TestEVM_BroadcastAssemblyFails_StaysSigned(t *testing.T) { builder.On("BroadcastOutboundSigningRequest", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("", fmt.Errorf("connection refused")) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -312,44 +329,61 @@ func TestEVM_BroadcastFails_WithTxHash_NonceNotConsumed_StaysSigned(t *testing.T Return("0xabc", fmt.Errorf("gas too low")) builder.On("VerifyBroadcastedTx", mock.Anything, "0xabc"). Return(false, uint64(0), uint64(0), uint8(0), nil) - builder.On("GetNextNonce", mock.Anything, "0xTSS", true).Return(uint64(5), nil) + builder.On("GetNextNonce", mock.Anything, testBroadcastSigner, true).Return(uint64(5), nil) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") require.Equal(t, store.StatusSigned, ev.Status) // stays SIGNED } -func TestEVM_GetTSSAddressNil_UsesEmptyAddress(t *testing.T) { - // getTSSAddress is nil → empty string passed to GetNextNonce on broadcast error. +// An unrecoverable signer leaves no nonce domain to query. The broadcaster must +// defer rather than fall back to another address, which previously meant asking +// for the nonce of the empty string. +func TestEVM_SignerUnrecoverable_StaysSigned(t *testing.T) { evtStore, db := setupTestDB(t) builder := &mockTxBuilder{} client := &mockChainClient{builder: builder} ch := newTestChains(t, "eip155:1", uregistrytypes.VmType_EVM, client) - insertSignedEvent(t, db, "ev-1", "eip155:1", 5) + insertSignedEventUnsigned(t, db, "ev-1", "eip155:1", 5) builder.On("BroadcastOutboundSigningRequest", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("0xabc", fmt.Errorf("already known")) builder.On("VerifyBroadcastedTx", mock.Anything, "0xabc"). Return(false, uint64(0), uint64(0), uint8(0), nil) - // Expect empty address since GetTSSAddress is nil. - builder.On("GetNextNonce", mock.Anything, "", true).Return(uint64(10), nil) - b := NewBroadcaster(Config{ - EventStore: evtStore, - Chains: ch, - Logger: zerolog.Nop(), - GetTSSAddress: nil, // explicitly nil - }) + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) - ev := getEvent(t, db, "ev-1") - require.Equal(t, store.StatusBroadcasted, ev.Status) - builder.AssertCalled(t, "GetNextNonce", mock.Anything, "", true) + require.Equal(t, store.StatusSigned, getEvent(t, db, "ev-1").Status) + builder.AssertNotCalled(t, "GetNextNonce", mock.Anything, mock.Anything, mock.Anything) } +// insertSignedEventUnsigned inserts a SIGNED outbound whose signature cannot be +// recovered, standing in for a legacy or malformed payload. +func insertSignedEventUnsigned(t *testing.T, db *gorm.DB, eventID, destChain string, nonce uint64) { + t.Helper() + data := txflow.SignedOutboundData{ + OutboundCreatedEvent: uexecutortypes.OutboundCreatedEvent{ + TxID: "tx-123", UniversalTxId: "utx-456", DestinationChain: destChain, + Recipient: "0xRecipient", Amount: "1000000", + }, + SigningData: &txflow.SigningData{ + Signature: hex.EncodeToString(make([]byte, 64)), + SigningHash: hex.EncodeToString(make([]byte, 32)), + Nonce: nonce, + }, + } + b, err := json.Marshal(data) + require.NoError(t, err) + require.NoError(t, db.Create(&store.Event{ + EventID: eventID, BlockHeight: 100, ExpiryBlockHeight: 99999, + Type: "SIGN_OUTBOUND", ConfirmationType: "STANDARD", + Status: store.StatusSigned, EventData: b, + }).Error) +} func TestSVM_DeadlineZero_ClusterConfirmsExpiry_MarksBroadcasted(t *testing.T) { // Legacy event without a signing deadline. `now > 0` enters the deadline // branch and any fresh cluster time (>> 0) trips the expiry case → @@ -362,7 +396,7 @@ func TestSVM_DeadlineZero_ClusterConfirmsExpiry_MarksBroadcasted(t *testing.T) { insertSignedEvent(t, db, "ev-1", "solana:mainnet", 0) builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(false, time.Now().Unix(), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -385,7 +419,7 @@ func TestSVM_BroadcastSuccess_MarksBroadcasted(t *testing.T) { builder.On("BroadcastOutboundSigningRequest", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("solTxSig123", nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -407,7 +441,7 @@ func TestSVM_BroadcastFails_PDAExists_MarksBroadcasted(t *testing.T) { Return("", fmt.Errorf("tx simulation failed: account already exists")) builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(true, int64(0), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -429,7 +463,7 @@ func TestSVM_BroadcastFails_BeforeDeadline_StaysSigned(t *testing.T) { Return("", fmt.Errorf("simulation failed: invalid instruction")) builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(false, int64(0), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -448,7 +482,7 @@ func TestSVM_BroadcastFails_PastDeadline_MarksBroadcastedForRevert(t *testing.T) // PDA absent, cluster time = now (fresh) and well past deadline → cluster-confirmed expiry. builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(false, time.Now().Unix(), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -468,7 +502,7 @@ func TestSVM_PastLocalDeadline_ExecutedByPeer_MarksBroadcasted(t *testing.T) { insertSignedSVMEventWithDeadline(t, db, "ev-1", "solana:mainnet", 0, time.Now().Unix()-3600) builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(true, time.Now().Unix(), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -494,7 +528,7 @@ func TestSVM_PastLocalDeadline_ClusterSaysStillInWindow_FallsThroughToBroadcast( builder.On("BroadcastOutboundSigningRequest", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("tx-hash-ok", nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -513,7 +547,7 @@ func TestSVM_PastLocalDeadline_RPCError_StaysSigned(t *testing.T) { insertSignedSVMEventWithDeadline(t, db, "ev-1", "solana:mainnet", 0, time.Now().Unix()-3600) builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(false, int64(0), fmt.Errorf("RPC down")) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -535,7 +569,7 @@ func TestSVM_BroadcastFails_PDACheckFails_StaysSigned(t *testing.T) { Return("", fmt.Errorf("RPC timeout")) builder.On("IsAlreadyExecuted", mock.Anything, "tx-123").Return(false, int64(0), fmt.Errorf("RPC down")) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "ev-1") @@ -548,7 +582,7 @@ func TestProcessSigned_NoEvents_DoesNothing(t *testing.T) { client := &mockChainClient{builder: builder} ch := newTestChains(t, "eip155:1", uregistrytypes.VmType_EVM, client) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) // no panic, no calls builder.AssertNotCalled(t, "GetNextNonce", mock.Anything, mock.Anything, mock.Anything) @@ -556,7 +590,7 @@ func TestProcessSigned_NoEvents_DoesNothing(t *testing.T) { func TestProcessSigned_NilChains_DoesNothing(t *testing.T) { evtStore, _ := setupTestDB(t) - b := newBroadcaster(evtStore, nil, "") + b := newBroadcaster(evtStore, nil) b.processSigned(context.Background()) // should not panic } @@ -573,7 +607,7 @@ func TestProcessSigned_MultipleEvents(t *testing.T) { builder.On("BroadcastOutboundSigningRequest", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("0xabc", nil) - b := newBroadcaster(evtStore, ch, "0xTSS") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev1 := getEvent(t, db, "ev-1") @@ -586,7 +620,7 @@ func TestMarkBroadcasted_FormatsCAIPTxHash(t *testing.T) { evtStore, db := setupTestDB(t) insertSignedEvent(t, db, "ev-1", "eip155:1", 5) - b := newBroadcaster(evtStore, nil, "") + b := newBroadcaster(evtStore, nil) ev := getEvent(t, db, "ev-1") b.markBroadcasted(&ev, "eip155:1", "0xdeadbeef") @@ -599,7 +633,7 @@ func TestMarkBroadcasted_EmptyTxHash(t *testing.T) { evtStore, db := setupTestDB(t) insertSignedEvent(t, db, "ev-1", "solana:mainnet", 3) - b := newBroadcaster(evtStore, nil, "") + b := newBroadcaster(evtStore, nil) ev := getEvent(t, db, "ev-1") b.markBroadcasted(&ev, "solana:mainnet", "") @@ -681,7 +715,7 @@ func TestFundMigrationEVM_BroadcastSuccess(t *testing.T) { mock.Anything). Return("0xmigrate123", nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "fm-1") @@ -721,7 +755,7 @@ func TestFundMigrationEVM_TSSFundMigrationAmountThreaded(t *testing.T) { mock.Anything). Return("0xmigrate777", nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "fm-transfer") @@ -743,7 +777,7 @@ func TestFundMigrationEVM_BroadcastFails_NonceConsumed(t *testing.T) { Return(false, uint64(0), uint64(0), uint8(0), nil) builder.On("GetNextNonce", mock.Anything, mock.Anything, true).Return(uint64(10), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "fm-1") @@ -752,7 +786,7 @@ func TestFundMigrationEVM_BroadcastFails_NonceConsumed(t *testing.T) { func TestMarkBroadcasted_NonExistentEvent(t *testing.T) { evtStore, _ := setupTestDB(t) - b := newBroadcaster(evtStore, nil, "") + b := newBroadcaster(evtStore, nil) ev := &store.Event{EventID: "does-not-exist"} b.markBroadcasted(ev, "eip155:1", "0xdeadbeef") @@ -763,7 +797,7 @@ func TestMarkBroadcasted_SetsAllFields(t *testing.T) { evtStore, db := setupTestDB(t) insertSignedEvent(t, db, "ev-fields", "eip155:1", 5) - b := newBroadcaster(evtStore, nil, "") + b := newBroadcaster(evtStore, nil) ev := getEvent(t, db, "ev-fields") b.markBroadcasted(&ev, "eip155:42", "0xcafe") @@ -816,10 +850,9 @@ func TestFundMigrationEVM_BroadcastFails_NonceNotConsumed_StaysSigned(t *testing Return(false, uint64(0), uint64(0), uint8(0), nil) builder.On("GetNextNonce", mock.Anything, mock.Anything, true).Return(uint64(3), nil) - b := newBroadcaster(evtStore, ch, "") + b := newBroadcaster(evtStore, ch) b.processSigned(context.Background()) ev := getEvent(t, db, "fm-1") require.Equal(t, store.StatusSigned, ev.Status) // stays SIGNED for retry } - diff --git a/universalClient/tss/txbroadcaster/evm.go b/universalClient/tss/txbroadcaster/evm.go index da0df315..e0bb380f 100644 --- a/universalClient/tss/txbroadcaster/evm.go +++ b/universalClient/tss/txbroadcaster/evm.go @@ -68,17 +68,15 @@ func (b *Broadcaster) broadcastOutboundEVM(ctx context.Context, event *store.Eve return } - tssAddress := "" - if b.getTSSAddress != nil { - var addrErr error - tssAddress, addrErr = b.getTSSAddress(ctx) - if addrErr != nil { - log.Warn().Err(addrErr).Msg("failed to get TSS address for nonce check, will retry next tick") - return - } + // Nonce check must use the key that signed this tx, not the live TSS: after a + // rotation they are different EOAs with unrelated nonce sequences. + signer, signedNonce, ok := txflow.RecoverOutboundSigner(event) + if !ok { + log.Warn().Msg("could not recover signing key for nonce check, will retry next tick") + return } - b.checkNonceAndMarkBroadcasted(ctx, event, builder, chainID, txHash, tssAddress, data.SigningData.Nonce, broadcastErr) + b.checkNonceAndMarkBroadcasted(ctx, event, builder, chainID, txHash, signer, signedNonce, broadcastErr) } // broadcastFundMigrationEVM broadcasts a signed EVM fund migration transaction. diff --git a/universalClient/tss/txflow/parse.go b/universalClient/tss/txflow/parse.go index 6bb71857..2d0f9f5b 100644 --- a/universalClient/tss/txflow/parse.go +++ b/universalClient/tss/txflow/parse.go @@ -5,6 +5,8 @@ import ( "encoding/json" "fmt" + "github.com/ethereum/go-ethereum/crypto" + "github.com/pushchain/push-chain-node/universalClient/chains/common" "github.com/pushchain/push-chain-node/universalClient/store" "github.com/pushchain/push-chain-node/universalClient/tss/coordinator" @@ -53,6 +55,38 @@ func ReadSigningDeadline(event *store.Event) int64 { // ReadFundMigrationSigner derives the sender EVM address (old TSS) and reads // the signed nonce from a fund migration event payload. Returns ok=false on // missing/invalid fields — caller defers in that case. +// RecoverOutboundSigner returns the EVM address that actually signed a SIGNED or +// BROADCASTED outbound, recovered from the persisted signature and signing hash. +// +// Nonces are per-EOA, so a nonce check is only meaningful against the key that +// signed. Outbound SigningData carries no key id, and after a TSS rotation the +// current key is a different EOA with an unrelated nonce sequence — comparing a +// K1-signed nonce against K2's would report "consumed" while K1's nonce is still +// free. Recovering from the signature binds the check to the right key without +// persisting anything new, so events signed before this existed are covered too. +// +// Returns ok=false when the signer cannot be established, which callers must +// treat as "defer", never as evidence the transaction did not execute. +func RecoverOutboundSigner(event *store.Event) (signer string, nonce uint64, ok bool) { + var data SignedOutboundData + if err := json.Unmarshal(event.EventData, &data); err != nil || data.SigningData == nil { + return "", 0, false + } + req, signature, err := DecodeSigningData(data.SigningData) + if err != nil || len(signature) != 65 || len(req.SigningHash) != 32 { + return "", 0, false + } + pub, err := crypto.SigToPub(req.SigningHash, signature) + if err != nil || pub == nil { + return "", 0, false + } + addr, err := coordinator.DeriveEVMAddressFromPubkey(hex.EncodeToString(crypto.CompressPubkey(pub))) + if err != nil { + return "", 0, false + } + return addr, data.SigningData.Nonce, true +} + func ReadFundMigrationSigner(event *store.Event) (signer string, nonce uint64, ok bool) { var data SignedFundMigrationData if err := json.Unmarshal(event.EventData, &data); err != nil || data.SigningData == nil || data.OldTssPubkey == "" { diff --git a/universalClient/tss/txflow/parse_test.go b/universalClient/tss/txflow/parse_test.go new file mode 100644 index 00000000..1062b0a3 --- /dev/null +++ b/universalClient/tss/txflow/parse_test.go @@ -0,0 +1,102 @@ +package txflow + +import ( + "encoding/hex" + "encoding/json" + "testing" + + "github.com/ethereum/go-ethereum/crypto" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/pushchain/push-chain-node/universalClient/store" + "github.com/pushchain/push-chain-node/universalClient/tss/coordinator" +) + +const ( + keyAHex = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318" + keyBHex = "8a1f9a8f9c8b7d6e5f4a3b2c1d0e9f8a7b6c5d4e3f2a1b0c9d8e7f6a5b4c3d2e" +) + +func signerAddr(t *testing.T, keyHex string) string { + t.Helper() + key, err := crypto.HexToECDSA(keyHex) + require.NoError(t, err) + addr, err := coordinator.DeriveEVMAddressFromPubkey(hex.EncodeToString(crypto.CompressPubkey(&key.PublicKey))) + require.NoError(t, err) + return addr +} + +// outboundEvent builds a SIGNED outbound payload signed by keyHex, matching what +// sessionManager persists. +func outboundEvent(t *testing.T, keyHex string, nonce uint64) *store.Event { + t.Helper() + key, err := crypto.HexToECDSA(keyHex) + require.NoError(t, err) + hash := crypto.Keccak256([]byte("signing hash")) + sig, err := crypto.Sign(hash, key) + require.NoError(t, err) + + b, err := json.Marshal(map[string]any{ + "tx_id": "tx-1", "utx_id": "utx-1", "destination_chain": "eip155:1", + "signing_data": map[string]any{ + "nonce": nonce, + "signature": hex.EncodeToString(sig), + "signing_hash": hex.EncodeToString(hash), + }, + }) + require.NoError(t, err) + return &store.Event{EventData: b} +} + +func TestRecoverOutboundSigner(t *testing.T) { + t.Run("recovers the address that signed", func(t *testing.T) { + signer, nonce, ok := RecoverOutboundSigner(outboundEvent(t, keyAHex, 5)) + require.True(t, ok) + assert.Equal(t, signerAddr(t, keyAHex), signer) + assert.Equal(t, uint64(5), nonce) + }) + + // The point of the change: two keys are two EOAs with unrelated nonce + // sequences, so the recovered signer has to follow the key that signed rather + // than whichever key is current. + t.Run("different keys recover to different addresses", func(t *testing.T) { + a, _, okA := RecoverOutboundSigner(outboundEvent(t, keyAHex, 5)) + b, _, okB := RecoverOutboundSigner(outboundEvent(t, keyBHex, 5)) + require.True(t, okA) + require.True(t, okB) + assert.NotEqual(t, a, b) + assert.Equal(t, signerAddr(t, keyBHex), b) + }) + + // Every failure has to report ok=false. A wrong address would be worse than + // no address: it produces a confident answer about the wrong nonce domain. + t.Run("unusable payloads report failure", func(t *testing.T) { + cases := map[string]*store.Event{ + "not json": {EventData: []byte("{")}, + "no signing data": {EventData: []byte(`{"tx_id":"tx-1"}`)}, + "short signature": {EventData: []byte(`{"signing_data":{"nonce":5,"signature":"deadbeef","signing_hash":"` + + hex.EncodeToString(crypto.Keccak256([]byte("h"))) + `"}}`)}, + "bad hex": {EventData: []byte(`{"signing_data":{"nonce":5,"signature":"zz","signing_hash":"zz"}}`)}, + "short hash": {EventData: []byte(`{"signing_data":{"nonce":5,"signature":"` + + hex.EncodeToString(make([]byte, 65)) + `","signing_hash":"00"}}`)}, + "unrecoverable signature": {EventData: []byte(`{"signing_data":{"nonce":5,"signature":"` + + hex.EncodeToString(make([]byte, 65)) + `","signing_hash":"` + + hex.EncodeToString(crypto.Keccak256([]byte("h"))) + `"}}`)}, + } + for name, ev := range cases { + t.Run(name, func(t *testing.T) { + _, _, ok := RecoverOutboundSigner(ev) + assert.False(t, ok) + }) + } + }) + + // Same signature, different persisted nonce: the nonce is read from the + // payload, the domain from the signature. They are independent. + t.Run("nonce comes from the payload", func(t *testing.T) { + _, nonce, ok := RecoverOutboundSigner(outboundEvent(t, keyAHex, 99)) + require.True(t, ok) + assert.Equal(t, uint64(99), nonce) + }) +} diff --git a/universalClient/tss/txresolver/evm.go b/universalClient/tss/txresolver/evm.go index 9167a516..83773b91 100644 --- a/universalClient/tss/txresolver/evm.go +++ b/universalClient/tss/txresolver/evm.go @@ -21,9 +21,13 @@ import ( // - Tx not found, nonce check unavailable → stay BROADCASTED (retry) // // The nonce IS the give-up signal; there is no max-retry counter. The two -// flows differ only in (a) which vote function records success/failure and -// (b) where the signer address comes from — current TSS for outbound, OLD TSS -// (derived from the event's old pubkey) for fund migration. +// flows differ only in which vote function records success/failure. +// +// Both check the nonce against the key that actually signed, never the current +// TSS: nonces are per-EOA, so after a rotation the live key is a different EOA +// whose sequence says nothing about an outbound signed under the previous one. +// Outbound recovers that signer from the signature, fund migration derives it +// from the event's old pubkey. // // Shared types (SignedOutboundData / SigningData) and helpers (DecodeSigningData, // ReadSignedNonce, ReadFundMigrationSigner, CheckNonce, NonceVerdict) live in @@ -164,21 +168,12 @@ func (r *Resolver) resolveFundMigrationEVM(ctx context.Context, event *store.Eve func (r *Resolver) outboundSigner(ctx context.Context, event *store.Event) (string, uint64, bool) { log := r.logger.With().Str("event_id", event.EventID).Logger() - signedNonce, ok := txflow.ReadSignedNonce(event) + signer, signedNonce, ok := txflow.RecoverOutboundSigner(event) if !ok { - log.Warn().Msg("EVM tx not found and signed nonce unavailable, staying BROADCASTED") - return "", 0, false - } - if r.getTSSAddress == nil { - log.Warn().Msg("EVM tx not found and no TSS-address resolver configured, staying BROADCASTED") - return "", 0, false - } - addr, err := r.getTSSAddress(ctx) - if err != nil { - log.Debug().Err(err).Msg("could not fetch TSS address, will retry next tick") + log.Warn().Msg("EVM tx not found and signing key unrecoverable, staying BROADCASTED") return "", 0, false } - return addr, signedNonce, true + return signer, signedNonce, true } // rewindToSigned moves a BROADCASTED event back to SIGNED so the broadcaster diff --git a/universalClient/tss/txresolver/resolver.go b/universalClient/tss/txresolver/resolver.go index 03fe4dcd..60284cb8 100644 --- a/universalClient/tss/txresolver/resolver.go +++ b/universalClient/tss/txresolver/resolver.go @@ -25,7 +25,6 @@ type Config struct { PushSigner *pushsigner.Signer CheckInterval time.Duration Logger zerolog.Logger - GetTSSAddress func(ctx context.Context) (string, error) } type Resolver struct { @@ -34,7 +33,6 @@ type Resolver struct { pushSigner *pushsigner.Signer checkInterval time.Duration logger zerolog.Logger - getTSSAddress func(ctx context.Context) (string, error) } func NewResolver(cfg Config) *Resolver { @@ -48,7 +46,6 @@ func NewResolver(cfg Config) *Resolver { pushSigner: cfg.PushSigner, checkInterval: interval, logger: cfg.Logger.With().Str("component", "txresolver").Logger(), - getTSSAddress: cfg.GetTSSAddress, } } diff --git a/universalClient/tss/txresolver/resolver_test.go b/universalClient/tss/txresolver/resolver_test.go index 752b03c2..5937a4f4 100644 --- a/universalClient/tss/txresolver/resolver_test.go +++ b/universalClient/tss/txresolver/resolver_test.go @@ -2,12 +2,14 @@ package txresolver import ( "context" + "encoding/hex" "encoding/json" "reflect" "testing" "time" "unsafe" + "github.com/ethereum/go-ethereum/crypto" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -23,6 +25,7 @@ import ( "github.com/pushchain/push-chain-node/universalClient/chains/common" "github.com/pushchain/push-chain-node/universalClient/config" "github.com/pushchain/push-chain-node/universalClient/store" + "github.com/pushchain/push-chain-node/universalClient/tss/coordinator" "github.com/pushchain/push-chain-node/universalClient/tss/eventstore" ) @@ -178,19 +181,6 @@ func newResolver(evtStore *eventstore.Store, ch *chains.Chains) *Resolver { }) } -// newResolverWithTSSAddress builds a Resolver that returns a fixed TSS address -// from GetTSSAddress — needed by tests that exercise the EVM nonce-based -// retry/revert path. -func newResolverWithTSSAddress(evtStore *eventstore.Store, ch *chains.Chains, addr string) *Resolver { - return NewResolver(Config{ - EventStore: evtStore, - Chains: ch, - CheckInterval: 0, - Logger: zerolog.Nop(), - GetTSSAddress: func(ctx context.Context) (string, error) { return addr, nil }, - }) -} - func TestParseCAIPTxHash(t *testing.T) { t.Run("valid CAIP tx hash", func(t *testing.T) { chainID, txHash, err := parseCAIPTxHash("eip155:1:0xabc123") @@ -930,14 +920,39 @@ func makeOutboundEventDataWithNonce(txID, utxID, destChain string, nonce uint64) "tx_id": txID, "utx_id": utxID, "destination_chain": destChain, - "signing_data": map[string]any{ - "nonce": nonce, - }, + "signing_data": testOutboundSigningData(testSigningKeyHex, nonce), }) return b } -const testEVMTSSAddr = "0x4D353565442Eb33b66ef88E14336F3F4Bf3a02FB" +// The resolver recovers the nonce domain from the signature, so payloads have to +// carry a real one. Two fixed keys stand in for a TSS key and its rotation +// successor; signing with one and checking the other's nonce is the bug. +const ( + testSigningKeyHex = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318" + testRotatedSigningKeyHex = "8a1f9a8f9c8b7d6e5f4a3b2c1d0e9f8a7b6c5d4e3f2a1b0c9d8e7f6a5b4c3d2e" +) + +func testOutboundSigningData(keyHex string, nonce uint64) map[string]any { + key, _ := crypto.HexToECDSA(keyHex) + hash := crypto.Keccak256([]byte("test outbound signing hash")) + sig, _ := crypto.Sign(hash, key) + return map[string]any{ + "nonce": nonce, + "signature": hex.EncodeToString(sig), + "signing_hash": hex.EncodeToString(hash), + } +} + +func testSignerAddr(keyHex string) string { + key, _ := crypto.HexToECDSA(keyHex) + addr, _ := coordinator.DeriveEVMAddressFromPubkey(hex.EncodeToString(crypto.CompressPubkey(&key.PublicKey))) + return addr +} + +// The address that signed the payloads above, i.e. the nonce domain the resolver +// must query. Not a configured value any more — it comes from the signature. +var testEVMTSSAddr = testSignerAddr(testSigningKeyHex) func TestResolveOutboundEVM_NotFound_NonceConsumed_Reverts(t *testing.T) { // Tx not found AND signed nonce < finalized nonce → another tx consumed @@ -957,7 +972,7 @@ func TestResolveOutboundEVM_NotFound_NonceConsumed_Reverts(t *testing.T) { // Finalized nonce = 7 → our nonce 5 is past finalized → consumed. builder.On("GetNextNonce", mock.Anything, testEVMTSSAddr, true).Return(uint64(7), nil) - resolver := newResolverWithTSSAddress(evtStore, ch, testEVMTSSAddr) + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) ev := getEvent(t, db, "ev-consumed-1") @@ -983,7 +998,7 @@ func TestResolveOutboundEVM_ReceiptError_NonceConsumed_DoesNotVoteFailure(t *tes // Finalized nonce 7 > signed nonce 5, so the nonce check would say "consumed". builder.On("GetNextNonce", mock.Anything, testEVMTSSAddr, true).Return(uint64(7), nil) - resolver := newResolverWithTSSAddress(evtStore, ch, testEVMTSSAddr) + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) require.Equal(t, store.StatusBroadcasted, getEvent(t, db, "ev-18826").Status) @@ -1015,7 +1030,7 @@ func TestResolveOutboundEVM_NotFound_NonceUnconsumed_RewindsToSigned(t *testing. // Finalized nonce = 5 → our nonce 5 not yet finalized. builder.On("GetNextNonce", mock.Anything, testEVMTSSAddr, true).Return(uint64(5), nil) - resolver := newResolverWithTSSAddress(evtStore, ch, testEVMTSSAddr) + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) ev := getEvent(t, db, "ev-unconsumed-1") @@ -1036,7 +1051,7 @@ func TestResolveOutboundEVM_NotFound_NonceRPCError_StaysBroadcasted(t *testing.T Return(false, uint64(0), uint64(0), uint8(0), nil) builder.On("GetNextNonce", mock.Anything, testEVMTSSAddr, true).Return(uint64(0), assert.AnError) - resolver := newResolverWithTSSAddress(evtStore, ch, testEVMTSSAddr) + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) ev := getEvent(t, db, "ev-rpc-err-1") @@ -1058,7 +1073,7 @@ func TestResolveOutboundEVM_NotFound_SignedNonceMissing_StaysBroadcasted(t *test builder.On("VerifyBroadcastedTx", mock.Anything, "0xmissing"). Return(false, uint64(0), uint64(0), uint8(0), nil) - resolver := newResolverWithTSSAddress(evtStore, ch, testEVMTSSAddr) + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) ev := getEvent(t, db, "ev-no-nonce") @@ -1066,52 +1081,69 @@ func TestResolveOutboundEVM_NotFound_SignedNonceMissing_StaysBroadcasted(t *test builder.AssertNotCalled(t, "GetNextNonce", mock.Anything, mock.Anything, mock.Anything) } -func TestResolveOutboundEVM_NotFound_TSSAddressFetchError_StaysBroadcasted(t *testing.T) { - // Tx not found and GetTSSAddress callback errors → defer (retry next tick). +// The nonce domain is derived from the signature, so if the signer cannot be +// recovered there is no domain to check. That must defer, never fall through to +// some other key's sequence. +func TestResolveOutboundEVM_NotFound_SignerUnrecoverable_StaysBroadcasted(t *testing.T) { evtStore, db := setupTestDB(t) builder := &mockTxBuilder{} client := &mockChainClient{builder: builder} ch := newTestChains(t, "eip155:1", uregistrytypes.VmType_EVM, client) - eventData := makeOutboundEventDataWithNonce("tx-100", "utx-200", "eip155:1", 5) - insertBroadcastedEvent(t, db, "ev-tss-err", "eip155:1", "eip155:1:0xmissing", eventData) + eventData, _ := json.Marshal(map[string]any{ + "tx_id": "tx-100", "utx_id": "utx-200", "destination_chain": "eip155:1", + "signing_data": map[string]any{ + "nonce": 5, + "signature": "deadbeef", // not 65 bytes + "signing_hash": hex.EncodeToString(crypto.Keccak256([]byte("h"))), + }, + }) + insertBroadcastedEvent(t, db, "ev-nosigner", "eip155:1", "eip155:1:0xmissing", eventData) builder.On("VerifyBroadcastedTx", mock.Anything, "0xmissing"). Return(false, uint64(0), uint64(0), uint8(0), nil) - resolver := NewResolver(Config{ - EventStore: evtStore, - Chains: ch, - CheckInterval: 0, - Logger: zerolog.Nop(), - GetTSSAddress: func(ctx context.Context) (string, error) { return "", assert.AnError }, - }) + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) - ev := getEvent(t, db, "ev-tss-err") - require.Equal(t, store.StatusBroadcasted, ev.Status) + require.Equal(t, store.StatusBroadcasted, getEvent(t, db, "ev-nosigner").Status) builder.AssertNotCalled(t, "GetNextNonce", mock.Anything, mock.Anything, mock.Anything) } -func TestResolveOutboundEVM_NotFound_NoTSSAddressResolver_StaysBroadcasted(t *testing.T) { - // Tx not found and GetTSSAddress is nil → can't run nonce check → defer. +// F-2026-18827. An outbound signed under K1 is still BROADCASTED when the TSS +// rotates to K2. K1 and K2 are separate EOAs with unrelated nonce sequences, so +// checking K2's would report the nonce consumed and fail-vote a transaction K1 +// can still land, while the refund path remints. The check must follow the key +// that signed. +func TestResolveOutboundEVM_NotFound_AfterRotation_ChecksSigningKeyNonce(t *testing.T) { evtStore, db := setupTestDB(t) builder := &mockTxBuilder{} client := &mockChainClient{builder: builder} ch := newTestChains(t, "eip155:1", uregistrytypes.VmType_EVM, client) + k1 := testSignerAddr(testSigningKeyHex) + k2 := testSignerAddr(testRotatedSigningKeyHex) + require.NotEqual(t, k1, k2) + + // Signed under K1 at nonce 5, still unresolved. eventData := makeOutboundEventDataWithNonce("tx-100", "utx-200", "eip155:1", 5) - insertBroadcastedEvent(t, db, "ev-no-tss-1", "eip155:1", "eip155:1:0xmissing", eventData) + insertBroadcastedEvent(t, db, "ev-rotated", "eip155:1", "eip155:1:0xmissing", eventData) builder.On("VerifyBroadcastedTx", mock.Anything, "0xmissing"). Return(false, uint64(0), uint64(0), uint8(0), nil) + // K1 nonce 5 is still free, so this tx can still mine. + builder.On("GetNextNonce", mock.Anything, k1, true).Return(uint64(5), nil) + // K2 has moved well past 5. Reading this domain is the bug. + builder.On("GetNextNonce", mock.Anything, k2, true).Return(uint64(42), nil) - resolver := newResolver(evtStore, ch) // no GetTSSAddress configured + // The live TSS is K2, the rotation successor. + resolver := newResolver(evtStore, ch) resolver.processBroadcasted(context.Background()) - ev := getEvent(t, db, "ev-no-tss-1") - require.Equal(t, store.StatusBroadcasted, ev.Status) - builder.AssertNotCalled(t, "GetNextNonce", mock.Anything, mock.Anything, mock.Anything) + builder.AssertCalled(t, "GetNextNonce", mock.Anything, k1, true) + builder.AssertNotCalled(t, "GetNextNonce", mock.Anything, k2, true) + require.Equal(t, store.StatusSigned, getEvent(t, db, "ev-rotated").Status, + "K1 nonce still free means rebroadcast, not a failure vote") } func TestResolveOutboundEVM_VerifyError_StaysBroadcasted(t *testing.T) {