some refactor and clean up

Signed-off-by: bytemare <[email protected]>

commitment.go

@@ -218,7 +218,7 @@ func (c *Configuration) ValidateCommitmentList(commitments CommitmentList) error
 		}
 
 		// List must be sorted, compare with the next commitment.
-		if uint64(i) < length-2 {
+		if uint64(i) <= length-2 {
 			if cmpID(commitment, commitments[i+1]) > 0 {
 				return fmt.Errorf("commitment list is not sorted by signer identifiers")
 			}
@@ -237,37 +237,6 @@ func (c *Configuration) ValidateCommitmentList(commitments CommitmentList) error
 	return nil
 }
 
-// Validate checks for the Commitment list's integrity.
-// - list is returned sorted
-// - no signer identifier in commitments is 0
-// - no
-func (c CommitmentList) Validate(g group.Group) error {
-	// set to detect duplication
-	set := make(map[uint64]struct{}, len(c))
-
-	for _, com := range c {
-
-		// Check for duplicate participant entries.
-		if _, exists := set[com.SignerID]; exists {
-			return fmt.Errorf("commitment list contains multiple commitments of participant %d", com.SignerID)
-		}
-
-		set[com.SignerID] = struct{}{}
-
-		// Check general validity of the commitment.
-		if err := com.Validate(g); err != nil {
-			return err
-		}
-	}
-
-	// Ensure the list is sorted
-	if !c.IsSorted() {
-		c.Sort()
-	}
-
-	return nil
-}
-
 func (c CommitmentList) Encode() []byte {
 	n := len(c)
 	if n == 0 {
@@ -359,7 +328,7 @@ func encodeCommitmentList(g group.Group, commitments []*commitmentWithEncodedID)
 	return encoded
 }
 
-// BindingFactors is a map of participant identifier to BindingFactors.
+// BindingFactors is a map of participant identifiers to BindingFactors.
 type BindingFactors map[uint64]*group.Scalar
 
 func (c CommitmentList) bindingFactors(publicKey *group.Element, message []byte) BindingFactors {

coordinator.go

@@ -34,15 +34,15 @@ type Signature struct {
 // The coordinator should verify this signature using the group public key before publishing or releasing the signature.
 // This aggregate signature will verify if and only if all signature shares are valid. If an invalid share is identified
 // a reasonable approach is to remove the signer from the set of allowed participants in future runs of FROST. If verify
-// is set to true, AggregateSignatures will automatically verify the signature shares and produced signatures, and will
+// is set to true, AggregateSignatures will automatically verify the signature shares and the output signature, and will
 // return an error with the first encountered invalid signature share.
 func (c *Configuration) AggregateSignatures(
 	message []byte,
 	sigShares []*SignatureShare,
 	commitments CommitmentList,
 	verify bool,
 ) (*Signature, error) {
-	groupCommitment, bindingFactors, participants, err := c.PrepareVerifySignatureShare(message, commitments)
+	groupCommitment, bindingFactors, participants, err := c.PrepareSignatureShareVerification(message, commitments)
 	if err != nil {
 		return nil, err
 	}
@@ -85,15 +85,15 @@ func (c *Configuration) VerifySignatureShare(
 	message []byte,
 	commitments CommitmentList,
 ) error {
-	groupCommitment, bindingFactors, participants, err := c.PrepareVerifySignatureShare(message, commitments)
+	groupCommitment, bindingFactors, participants, err := c.PrepareSignatureShareVerification(message, commitments)
 	if err != nil {
 		return err
 	}
 
 	return c.verifySignatureShare(sigShare, message, commitments, participants, groupCommitment, bindingFactors)
 }
 
-func (c *Configuration) PrepareVerifySignatureShare(message []byte,
+func (c *Configuration) PrepareSignatureShareVerification(message []byte,
 	commitments CommitmentList,
 ) (*group.Element, BindingFactors, []*group.Scalar, error) {
 	if !c.verified {
@@ -125,6 +125,46 @@ func (c *Configuration) getSignerPubKey(id uint64) *group.Element {
 	return nil
 }
 
+func (c *Configuration) validateSignatureShareLight(sigShare *SignatureShare) error {
+	if sigShare == nil {
+		return errors.New("nil signature share")
+	}
+
+	if sigShare.SignatureShare == nil || sigShare.SignatureShare.IsZero() {
+		return errors.New("invalid signature share (nil or zero)")
+	}
+
+	return nil
+}
+
+func (c *Configuration) validateSignatureShareExtensive(sigShare *SignatureShare) error {
+	if err := c.validateSignatureShareLight(sigShare); err != nil {
+		return err
+	}
+
+	if sigShare.SignerIdentifier == 0 {
+		return errors.New("signature share's signer identifier is 0 (invalid)")
+	}
+
+	if sigShare.SignerIdentifier > c.MaxSigners {
+		return fmt.Errorf(
+			"signature share has invalid ID %d, above authorized range [1:%d]",
+			sigShare.SignerIdentifier,
+			c.MaxSigners,
+		)
+	}
+
+	if sigShare.Group != c.group {
+		return fmt.Errorf("signature share has invalid group parameter, want %s got %s", c.group, sigShare.Group)
+	}
+
+	if c.getSignerPubKey(sigShare.SignerIdentifier) == nil {
+		return fmt.Errorf("no public key registered for signer %d", sigShare.SignerIdentifier)
+	}
+
+	return nil
+}
+
 func (c *Configuration) verifySignatureShare(
 	sigShare *SignatureShare,
 	message []byte,
@@ -133,9 +173,8 @@ func (c *Configuration) verifySignatureShare(
 	groupCommitment *group.Element,
 	bindingFactors BindingFactors,
 ) error {
-	// Due diligence check that no signer id == 0.
-	if sigShare.SignerIdentifier == 0 {
-		return errors.New("signer identifier is 0 (invalid)")
+	if err := c.validateSignatureShareExtensive(sigShare); err != nil {
+		return err
 	}
 
 	com := commitments.Get(sigShare.SignerIdentifier)
@@ -144,11 +183,7 @@ func (c *Configuration) verifySignatureShare(
 	}
 
 	pk := c.getSignerPubKey(sigShare.SignerIdentifier)
-	if pk == nil {
-		return fmt.Errorf("public key not registered for signer %d", sigShare.SignerIdentifier)
-	}
-
-	lambda := internal.Lambda2(c.group, sigShare.SignerIdentifier, participants)
+	lambda := internal.Lambda(c.group, sigShare.SignerIdentifier, participants)
 	lambdaChall := c.challenge(lambda, message, groupCommitment)
 
 	// Commitment KeyShare: r = g(h + b*f + l*s)

frost.go

@@ -132,8 +132,8 @@ var (
 )
 
 func (c *Configuration) verifySignerPublicKeys() error {
-	if uint64(len(c.SignerPublicKeys)) < c.Threshold ||
-		uint64(len(c.SignerPublicKeys)) > c.MaxSigners {
+	length := uint64(len(c.SignerPublicKeys))
+	if length < c.Threshold || length > c.MaxSigners {
 		return errInvalidNumberOfPublicKeys
 	}
 
@@ -216,6 +216,65 @@ func (c *Configuration) Init() error {
 	return nil
 }
 
+func (c *Configuration) ValidateKeyShare(keyShare *KeyShare) error {
+	if !c.verified {
+		if err := c.Init(); err != nil {
+			return err
+		}
+	}
+
+	if keyShare == nil {
+		return errors.New("provided key share is nil")
+	}
+
+	if keyShare.ID == 0 {
+		return errors.New("provided key share has invalid ID 0")
+	}
+
+	if keyShare.ID > c.MaxSigners {
+		return fmt.Errorf(
+			"provided key share has invalid ID %d, above authorized range [1:%d]",
+			keyShare.ID,
+			c.MaxSigners,
+		)
+	}
+
+	if keyShare.Group != c.group {
+		return fmt.Errorf("provided key share has invalid group parameter, want %s got %s", c.group, keyShare.Group)
+	}
+
+	if c.GroupPublicKey.Equal(keyShare.GroupPublicKey) != 1 {
+		return errors.New(
+			"the group's public key in the provided key share does not match the one in the configuration",
+		)
+	}
+
+	if keyShare.PublicKey == nil {
+		return errors.New("provided key share has nil public key")
+	}
+
+	if keyShare.Secret == nil || keyShare.Secret.IsZero() {
+		return errors.New("provided key share has invalid secret key")
+	}
+
+	if c.group.Base().Multiply(keyShare.Secret).Equal(keyShare.PublicKey) != 1 {
+		return errors.New("provided key share has non-matching secret and public keys")
+	}
+
+	pk := c.getSignerPubKey(keyShare.ID)
+	if pk == nil {
+		return errors.New("provided key share has no registered signer identifier in the configuration")
+	}
+
+	if pk.Equal(keyShare.PublicKey) != 1 {
+		return errors.New(
+			"provided key share has a different public key than the one registered for that signer in the configuration",
+		)
+	}
+
+	return nil
+}
+
 // Signer returns a new participant of the protocol instantiated from the Configuration and the signer's key share.
 func (c *Configuration) Signer(keyShare *KeyShare) (*Signer, error) {
 	if !c.verified {
@@ -224,6 +283,10 @@ func (c *Configuration) Signer(keyShare *KeyShare) (*Signer, error) {
 		}
 	}
 
+	if err := c.ValidateKeyShare(keyShare); err != nil {
+		return nil, err
+	}
+
 	return &Signer{
 		KeyShare:         keyShare,
 		LambdaRegistry:   make(internal.LambdaRegistry),

internal/lambda.go

@@ -17,21 +17,12 @@ import (
 	secretsharing "github.com/bytemare/secret-sharing"
 )
 
-func Lambda(g group.Group, id uint64, polynomial secretsharing.Polynomial) (*group.Scalar, error) {
-	l, err := polynomial.DeriveInterpolatingValue(g, g.NewScalar().SetUInt64(id))
-	if err != nil {
-		return nil, fmt.Errorf("anomaly in participant identifiers: %w", err)
-	}
-
-	return l, nil
-}
-
 // Lambda derives the interpolating value for id in the polynomial made by the participant identifiers.
 // This function assumes that:
 // - id is non-nil and != 0
 // - every scalar in participants is non-nil and != 0
 // - there are no duplicates in participants
-func Lambda2(g group.Group, id uint64, participants []*group.Scalar) *group.Scalar {
+func Lambda(g group.Group, id uint64, participants []*group.Scalar) *group.Scalar {
 	sid := g.NewScalar().SetUInt64(id)
 	numerator := g.NewScalar().One()
 	denominator := g.NewScalar().One()
@@ -68,7 +59,7 @@ func (l LambdaRegistry) New(g group.Group, id uint64, participants []uint64) *gr
 		}
 
 	*/
-	lambda := Lambda2(g, id, polynomial)
+	lambda := Lambda(g, id, polynomial)
 
 	l.Set(participants, lambda)
 

signer.go

@@ -81,8 +81,7 @@ func (s *Signer) Identifier() uint64 {
 func randomCommitmentID() uint64 {
 	buf := make([]byte, 8)
 
-	_, err := rand.Read(buf)
-	if err != nil {
+	if _, err := rand.Read(buf); err != nil {
 		panic(fmt.Errorf("FATAL: %w", err))
 	}
 
@@ -100,6 +99,8 @@ func (s *Signer) generateNonce(secret *group.Scalar, random []byte) *group.Scala
 func (s *Signer) genNonceID() uint64 {
 	cid := randomCommitmentID()
 
+	// In the extremely rare and unlikely case the CSPRNG returns an already registered ID, we try again 128 times
+	// before failing.
 	for range 128 {
 		if _, exists := s.NonceCommitments[cid]; !exists {
 			return cid
@@ -108,7 +109,7 @@ func (s *Signer) genNonceID() uint64 {
 		cid = randomCommitmentID()
 	}
 
-	panic("FATAL: CSPRNG could not generate a unique nonce over 128 iterations")
+	panic("FATAL: CSPRNG could not generate unique commitment identifiers over 128 iterations")
 }
 
 // Commit generates a signer's nonces and commitment, to be used in the second FROST round. The internal nonce must

tests/frost_error_test.go

@@ -385,7 +385,7 @@ func TestConfiguration_PrepareVerifySignatureShare_BadNonVerifiedConfiguration(t
 		SignerPublicKeys: publicKeyShares,
 	}
 
-	if _, _, _, err := configuration.PrepareVerifySignatureShare(nil, nil); err == nil ||
+	if _, _, _, err := configuration.PrepareSignatureShareVerification(nil, nil); err == nil ||
 		err.Error() != expectedErrorPrefix.Error() {
 		t.Fatalf("expected %q, got %q", expectedErrorPrefix, err)
 	}
@@ -405,7 +405,7 @@ func TestConfiguration_PrepareVerifySignatureShare_InvalidCommitments(t *testing
 		coms[i] = s.Commit()
 	}
 
-	if _, _, _, err := configuration.PrepareVerifySignatureShare(nil, coms[:1]); err == nil ||
+	if _, _, _, err := configuration.PrepareSignatureShareVerification(nil, coms[:1]); err == nil ||
 		!strings.HasPrefix(err.Error(), expectedErrorPrefix) {
 		t.Fatalf("expected %q, got %q", expectedErrorPrefix, err)
 	}

tests/misc_test.go

@@ -435,7 +435,7 @@ func TestLambda_BadID(t *testing.T) {
 	}
 
 	// todo : what happens if the participant list is not vetted?
-	fmt.Println(internal.Lambda2(g, 1, polynomial).Hex())
+	fmt.Println(internal.Lambda(g, 1, polynomial).Hex())
 }
 
 func TestLambdaRegistry(t *testing.T) {