From 88706294d9b317446e83b5b1ca1cf392880d2546 Mon Sep 17 00:00:00 2001
From: Robert Raynor <35671663+mooselumph@users.noreply.github.com>
Date: Tue, 14 Nov 2023 15:37:46 -0800
Subject: [PATCH] Validate churner request (#30)

---
 churner/churner.go     |  6 ++++-
 churner/metrics.go     |  1 +
 churner/server.go      | 61 ++++++++++++++++++++++++++++++++----------
 churner/server_test.go |  2 +-
 4 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/churner/churner.go b/churner/churner.go
index 09075e4af..5eb57f2c0 100644
--- a/churner/churner.go
+++ b/churner/churner.go
@@ -180,7 +180,11 @@ func (c *churner) getOperatorsToChurn(ctx context.Context, quorumIDs []uint8, op
 			return nil, nil
 		}
 
-		if operatorSetParams.MaxOperatorCount != uint32(len(operatorStakes[i])) {
+		if operatorSetParams.MaxOperatorCount == 0 {
+			return nil, errors.New("maxOperatorCount is 0")
+		}
+
+		if uint32(len(operatorStakes[i])) < operatorSetParams.MaxOperatorCount {
 			// quorum is not full, so we can continue
 			continue
 		}
diff --git a/churner/metrics.go b/churner/metrics.go
index f7b2119cd..c1efc22ca 100644
--- a/churner/metrics.go
+++ b/churner/metrics.go
@@ -22,6 +22,7 @@ const (
 	FailReasonPrevApprovalNotExpired      FailReason = "prev_approval_not_expired"      // Expiry: previous approval hasn't expired
 	FailReasonInvalidSignature            FailReason = "invalid_signature"              // Invalid signature: operator's signature is wong
 	FailReasonProcessChurnRequestFailed   FailReason = "failed_process_churn_request"   // Failed to process churn request
+	FailReasonInvalidRequest              FailReason = "invalid_request"                // Invalid request: request is malformed
 )
 
 type MetricsConfig struct {
diff --git a/churner/server.go b/churner/server.go
index 85f9b196e..296e93ed0 100644
--- a/churner/server.go
+++ b/churner/server.go
@@ -51,6 +51,13 @@ func (s *Server) Start(metricsConfig MetricsConfig) error {
 }
 
 func (s *Server) Churn(ctx context.Context, req *pb.ChurnRequest) (*pb.ChurnReply, error) {
+
+	err := s.validateChurnRequest(ctx, req)
+	if err != nil {
+		s.metrics.IncrementFailedRequestNum("Churn", FailReasonInvalidRequest)
+		return nil, fmt.Errorf("invalid request: %w", err)
+	}
+
 	timer := prometheus.NewTimer(prometheus.ObserverFunc(func(f float64) {
 		s.metrics.ObserveLatency("Churn", f*1000) // make milliseconds
 	}))
@@ -64,20 +71,6 @@ func (s *Server) Churn(ctx context.Context, req *pb.ChurnRequest) (*pb.ChurnRepl
 		return nil, fmt.Errorf("previous approval not expired, retry in %d", s.latestExpiry-now.Unix())
 	}
 
-	for quorumID := range req.GetQuorumIds() {
-		if quorumID >= int(s.churner.QuorumCount) {
-			err := s.churner.UpdateQuorumCount(ctx)
-			if err != nil {
-				return nil, fmt.Errorf("failed to get onchain quorum count: %w", err)
-			}
-
-			if quorumID >= int(s.churner.QuorumCount) {
-				s.metrics.IncrementFailedRequestNum("Churn", FailReasonQuorumIdOutOfRange)
-				return nil, fmt.Errorf("Invalid request: the quorum_id must be in range [0, %d], but found %d", s.churner.QuorumCount-1, quorumID)
-			}
-		}
-	}
-
 	request := createChurnRequest(req)
 
 	operatorToRegisterAddress, err := s.churner.VerifyRequestSignature(ctx, request)
@@ -125,6 +118,46 @@ func (s *Server) checkShouldBeRateLimited(now time.Time, request ChurnRequest) e
 	return nil
 }
 
+func (s *Server) validateChurnRequest(ctx context.Context, req *pb.ChurnRequest) error {
+
+	if len(req.OperatorRequestSignature) != 64 {
+		return fmt.Errorf("invalid signature length")
+	}
+
+	if len(req.OperatorToRegisterPubkeyG1) != 64 {
+		return fmt.Errorf("invalid operatorToRegisterPubkeyG1 length")
+	}
+
+	if len(req.OperatorToRegisterPubkeyG2) != 128 {
+		return fmt.Errorf("invalid operatorToRegisterPubkeyG2 length")
+	}
+
+	if len(req.Salt) != 32 {
+		return fmt.Errorf("invalid salt length")
+	}
+
+	// TODO: ensure that all quorumIDs are valid
+	if len(req.QuorumIds) == 0 {
+		return fmt.Errorf("invalid quorumIds length")
+	}
+
+	for quorumID := range req.GetQuorumIds() {
+		if quorumID >= int(s.churner.QuorumCount) {
+			err := s.churner.UpdateQuorumCount(ctx)
+			if err != nil {
+				return fmt.Errorf("failed to get onchain quorum count: %w", err)
+			}
+
+			if quorumID >= int(s.churner.QuorumCount) {
+				return fmt.Errorf("invalid request: the quorum_id must be in range [0, %d], but found %d", s.churner.QuorumCount-1, quorumID)
+			}
+		}
+	}
+
+	return nil
+
+}
+
 func createChurnRequest(req *pb.ChurnRequest) *ChurnRequest {
 	signature := &core.Signature{G1Point: new(core.G1Point).Deserialize(req.GetOperatorRequestSignature())}
 
diff --git a/churner/server_test.go b/churner/server_test.go
index de2be6f6a..a6f5a9996 100644
--- a/churner/server_test.go
+++ b/churner/server_test.go
@@ -110,7 +110,7 @@ func TestChurnWithInvalidQuorum(t *testing.T) {
 	}, nil)
 
 	_, err := s.Churn(ctx, request)
-	assert.ErrorContains(t, err, "Invalid request: the quorum_id must be in range [0, 0], but found 1")
+	assert.ErrorContains(t, err, "invalid request: the quorum_id must be in range [0, 0], but found 1")
 }
 
 func setupMockTransactor() {