fix: bind setup participants for keygen, keyrefresh and quorumchange too (F-2026-18199)

Author: Aman035

universalClient/tss/dkls/utils.go

@@ -2,6 +2,9 @@ package dkls
 
 import (
 	"crypto/sha256"
+	"fmt"
+
+	session "go-wrapper/go-dkls/sessions"
 )
 
 // deriveKeyID derives a key ID bytes from a string key ID.
@@ -22,3 +25,28 @@ func encodeParticipantIDs(participants []string) []byte {
 	}
 	return ids
 }
+
+// SetupParticipants returns the participant list embedded in a DKLS setup blob,
+// in index order. The setup is what actually drives the session, so callers must
+// confirm it matches the participants they validated. Otherwise a coordinator
+// can present one list for validation and run the session over another.
+//
+// Party names decode by index and come back empty past the end, which is how the
+// list terminates.
+func SetupParticipants(setupData []byte) ([]string, error) {
+	if len(setupData) == 0 {
+		return nil, fmt.Errorf("setupData is required")
+	}
+	var participants []string
+	for i := 0; ; i++ {
+		name, err := session.DklsDecodePartyName(setupData, i)
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode party name at index %d: %w", i, err)
+		}
+		if len(name) == 0 {
+			break
+		}
+		participants = append(participants, string(name))
+	}
+	return participants, nil
+}

universalClient/tss/sessionmanager/sessionmanager.go

@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"math/big"
+	"slices"
 	"sync"
 	"time"
 
@@ -212,6 +213,17 @@ func (sm *SessionManager) handleSetupMessage(ctx context.Context, senderPeerID s
 		}
 	}
 
+	// 6d. Same split for the participant list: we validate msg.Participants above
+	// but the session runs on the list embedded in Payload, so an unbound setup
+	// could run over a different set than the one we approved.
+	if err := verifySetupBindsParticipants(msg.Payload, msg.Participants); err != nil {
+		sm.logger.Error().Err(err).
+			Str("event_id", msg.EventID).
+			Str("coordinator", senderPeerID).
+			Msg("setup message participants do not match the validated list - rejecting")
+		return err
+	}
+
 	// 7. Create session based on protocol type
 	session, err := sm.createSession(ctx, event, msg)
 	if err != nil {
@@ -1010,6 +1022,28 @@ func verifySetupBindsHash(setupData, verifiedHash []byte) error {
 	return nil
 }
 
+// verifySetupBindsParticipants requires the DKLS setup blob to embed exactly the
+// participant list the caller already validated, in the same order. Index order
+// is part of the protocol, so a reorder is as consequential as a substitution.
+//
+// Note this cannot cover the threshold: the setup embeds one, the wrapper exposes
+// no decoder for it, and the threshold argument the session constructors take is
+// unused. So the coordinator's embedded threshold is authoritative and unchecked.
+func verifySetupBindsParticipants(setupData []byte, validated []string) error {
+	if len(validated) == 0 {
+		return fmt.Errorf("no validated participants to bind setup message to")
+	}
+	embedded, err := dkls.SetupParticipants(setupData)
+	if err != nil {
+		return fmt.Errorf("cannot decode setup message participants: %w", err)
+	}
+	if !slices.Equal(embedded, validated) {
+		return fmt.Errorf("setup message participants %v do not match validated participants %v",
+			embedded, validated)
+	}
+	return nil
+}
+
 // maxNonceGap bounds how far above the ceiling base a coordinator may assign.
 // An honest coordinator starts at the pending nonce and increments at most
 // coordinator.PerChainCap times per poll, so pending+PerChainCap is the true

universalClient/tss/sessionmanager/sessionmanager_test.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"math/big"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 	"unsafe"
@@ -469,18 +470,17 @@ func TestSessionManager_Integration(t *testing.T) {
 		Type:         "setup",
 		EventID:      event.EventID,
 		Participants: []string{"validator1", "validator2", "validator3"},
-		Payload:      []byte("invalid setup data"), // Will fail when creating session
+		Payload:      []byte("invalid setup data"), // rejected before a session is created
 	}
 
-	// This will fail at session creation or GetLatestBlockNum, but validation should pass
+	// Rejected at the setup-binding check (the payload is not a decodable DKLS
+	// setup), or earlier at GetLatestBlockNum. Either way validation must not
+	// let an unbound payload reach session creation.
 	err := sm.HandleIncomingMessage(ctx, "peer1", &msg)
-	// We expect an error because we can't create a real DKLS session with invalid data
-	// or because GetLatestBlockNum fails
 	assert.Error(t, err)
-	// Error should be about session creation, DKLS library, or no endpoints
 	assert.True(t,
-		containsAny(err.Error(), []string{"failed to create session", "DKLS", "dkls", "session", "no endpoints"}),
-		"error should be about session creation or endpoints, got: %s", err.Error())
+		containsAny(err.Error(), []string{"failed to create session", "DKLS", "dkls", "session", "setup message", "no endpoints"}),
+		"error should be about setup binding, session creation or endpoints, got: %s", err.Error())
 }
 
 func TestVerifySigningRequest_OutboundDisabled(t *testing.T) {
@@ -1496,3 +1496,51 @@ func TestVerifySetupBindsHash(t *testing.T) {
 		assert.Contains(t, err.Error(), "no verified signing hash")
 	})
 }
+
+// Keygen, keyrefresh and quorumchange have the same split as the sign path: we
+// validate msg.Participants, but the session runs on the list embedded in
+// Payload. The threshold cannot be bound this way, see verifySetupBindsParticipants.
+func TestVerifySetupBindsParticipants(t *testing.T) {
+	validated := []string{"validator1", "validator2", "validator3"}
+	encode := func(ids []string) []byte {
+		return []byte(strings.Join(ids, "\x00"))
+	}
+
+	legitSetup, err := session.DklsKeygenSetupMsgNew(2, nil, encode(validated))
+	require.NoError(t, err)
+
+	t.Run("accepts setup with the validated participants", func(t *testing.T) {
+		require.NoError(t, verifySetupBindsParticipants(legitSetup, validated))
+	})
+
+	t.Run("rejects setup with a substituted participant", func(t *testing.T) {
+		swapped, err := session.DklsKeygenSetupMsgNew(2, nil,
+			encode([]string{"validator1", "validator2", "attacker"}))
+		require.NoError(t, err)
+		err = verifySetupBindsParticipants(swapped, validated)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "do not match validated participants")
+	})
+
+	t.Run("rejects setup with a dropped participant", func(t *testing.T) {
+		fewer, err := session.DklsKeygenSetupMsgNew(2, nil,
+			encode([]string{"validator1", "validator2"}))
+		require.NoError(t, err)
+		require.Error(t, verifySetupBindsParticipants(fewer, validated))
+	})
+
+	// Index order is part of the protocol, so a reorder is as consequential as
+	// a substitution.
+	t.Run("rejects reordered participants", func(t *testing.T) {
+		reordered, err := session.DklsKeygenSetupMsgNew(2, nil,
+			encode([]string{"validator3", "validator2", "validator1"}))
+		require.NoError(t, err)
+		require.Error(t, verifySetupBindsParticipants(reordered, validated))
+	})
+
+	t.Run("rejects undecodable setup and missing validated list", func(t *testing.T) {
+		require.Error(t, verifySetupBindsParticipants([]byte("not-a-setup"), validated))
+		require.Error(t, verifySetupBindsParticipants(nil, validated))
+		require.Error(t, verifySetupBindsParticipants(legitSetup, nil))
+	})
+}