fix(blobs): Validate sidecar KzgCommitments and disallow sidecar omission

beacon/blockchain/errors.go

@@ -25,6 +25,8 @@ import "github.com/berachain/beacon-kit/errors"
 var (
 	// ErrNilBlk is an error for when the beacon block is nil.
 	ErrNilBlk = errors.New("nil beacon block")
+	// ErrNilBlob is an error for when the BlobSidecars is nil.
+	ErrNilBlob = errors.New("nil blob")
 	// ErrDataNotAvailable indicates that the required data is not available.
 	ErrDataNotAvailable = errors.New("data not available")
 )

beacon/blockchain/process_proposal.go

@@ -22,6 +22,7 @@ package blockchain
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	payloadtime "github.com/berachain/beacon-kit/beacon/payload-time"
@@ -61,6 +62,11 @@ func (s *Service[
 	)
 	if err != nil {
 		return createProcessProposalResponse(errors.WrapNonFatal(err))
+	} else if blk.IsNil() {
+		s.logger.Warn(
+			"Aborting block verification - beacon block not found in proposal",
+		)
+		return createProcessProposalResponse(errors.WrapNonFatal(ErrNilBlk))
 	}
 
 	// Decode the blob sidecars.
@@ -71,59 +77,47 @@ func (s *Service[
 	)
 	if err != nil {
 		return createProcessProposalResponse(errors.WrapNonFatal(err))
+	} else if sidecars.IsNil() {
+		s.logger.Warn(
+			"Aborting block verification - blob sidecars not found in proposal",
+		)
+		return createProcessProposalResponse(errors.WrapNonFatal(ErrNilBlob))
 	}
 
-	// In theory, swapping the order of verification between the sidecars
-	// and the incoming block should not introduce any inconsistencies
-	// in the state on which the sidecar verification depends on (notably
-	// the currently active fork). ProcessProposal should only
-	// keep the state changes as candidates (which is what we do in
-	// VerifyIncomingBlock).
+	// Make sure we have the right number of BlobSidecars
+	numCommitments := len(blk.GetBody().GetBlobKzgCommitments())
+	if numCommitments != sidecars.Len() {
+		err = fmt.Errorf("expected %d sidecars, got %d",
+			numCommitments, sidecars.Len(),
+		)
+		return createProcessProposalResponse(errors.WrapNonFatal(err))
+	}
 
-	// Process the blob sidecars, if any
-	if !sidecars.IsNil() && sidecars.Len() > 0 {
+	if numCommitments > 0 {
+		// Process the blob sidecars
+		//
+		// In theory, swapping the order of verification between the sidecars
+		// and the incoming block should not introduce any inconsistencies
+		// in the state on which the sidecar verification depends on (notably
+		// the currently active fork). ProcessProposal should only
+		// keep the state changes as candidates (which is what we do in
+		// VerifyIncomingBlock).
 		var consensusSidecars *types.ConsensusSidecars[BlobSidecarsT]
 		consensusSidecars = consensusSidecars.New(
 			sidecars,
 			blk.GetHeader(),
 		)
-
-		s.logger.Info("Received incoming blob sidecars")
-
-		// TODO: Clean this up once we remove generics.
-		cs := convertConsensusSidecars[
-			ConsensusSidecarsT,
-			BlobSidecarsT,
-		](consensusSidecars)
-
-		// Get the sidecar verification function from the state processor
-		//nolint:govet	// err shadowing
-		sidecarVerifierFn, err := s.stateProcessor.GetSidecarVerifierFn(
-			s.storageBackend.StateFromContext(ctx),
+		err = s.VerifyIncomingBlobSidecars(
+			ctx,
+			consensusSidecars,
 		)
 		if err != nil {
 			s.logger.Error(
-				"an error incurred while calculating the sidecar verifier",
-				"reason", err,
+				"failed to verify incoming blob sidecars",
+				"error", err,
 			)
 			return createProcessProposalResponse(errors.WrapNonFatal(err))
 		}
-
-		// Verify the blobs and ensure they match the local state.
-		err = s.blobProcessor.VerifySidecars(cs, sidecarVerifierFn)
-		if err != nil {
-			s.logger.Error(
-				"rejecting incoming blob sidecars",
-				"reason", err,
-			)
-			return createProcessProposalResponse(errors.WrapNonFatal(err))
-		}
-
-		s.logger.Info(
-			"Blob sidecars verification succeeded - accepting incoming blob sidecars",
-			"num_blobs",
-			sidecars.Len(),
-		)
 	}
 
 	// Process the block
@@ -147,6 +141,55 @@ func (s *Service[
 	return createProcessProposalResponse(nil)
 }
 
+// VerifyIncomingBlobSidecars verifies the BlobSidecars of an incoming
+// proposal and logs the process.
+func (s *Service[
+	_, _, _, _, _, _,
+	_, ConsensusSidecarsT, BlobSidecarsT,
+]) VerifyIncomingBlobSidecars(
+	ctx context.Context,
+	cSidecars *types.ConsensusSidecars[BlobSidecarsT],
+) error {
+	sidecars := cSidecars.GetSidecars()
+
+	s.logger.Info("Received incoming blob sidecars")
+
+	// TODO: Clean this up once we remove generics.
+	cs := convertConsensusSidecars[
+		ConsensusSidecarsT,
+		BlobSidecarsT,
+	](cSidecars)
+
+	// Get the sidecar verification function from the state processor
+	sidecarVerifierFn, err := s.stateProcessor.GetSidecarVerifierFn(
+		s.storageBackend.StateFromContext(ctx),
+	)
+	if err != nil {
+		s.logger.Error(
+			"an error incurred while calculating the sidecar verifier",
+			"reason", err,
+		)
+		return err
+	}
+
+	// Verify the blobs and ensure they match the local state.
+	err = s.blobProcessor.VerifySidecars(cs, sidecarVerifierFn)
+	if err != nil {
+		s.logger.Error(
+			"rejecting incoming blob sidecars",
+			"reason", err,
+		)
+		return err
+	}
+
+	s.logger.Info(
+		"Blob sidecars verification succeeded - accepting incoming blob sidecars",
+		"num_blobs",
+		sidecars.Len(),
+	)
+	return nil
+}
+
 // VerifyIncomingBlock verifies the state root of an incoming block
 // and logs the process.
 func (s *Service[
@@ -167,14 +210,6 @@ func (s *Service[
 	// ideally via some broader sync service.
 	s.forceStartupSyncOnce.Do(func() { s.forceStartupHead(ctx, preState) })
 
-	// If the block is nil or a nil pointer, exit early.
-	if beaconBlk.IsNil() {
-		s.logger.Warn(
-			"Aborting block verification - beacon block not found in proposal",
-		)
-		return errors.WrapNonFatal(ErrNilBlk)
-	}
-
 	s.logger.Info(
 		"Received incoming beacon block",
 		"state_root", beaconBlk.GetStateRoot(), "slot", beaconBlk.GetSlot(),

consensus/types/consensus_sidecars.go

@@ -25,8 +25,7 @@ import (
 )
 
 type ConsensusSidecars[SidecarsT any] struct {
-	sidecars SidecarsT
-
+	sidecars  SidecarsT
 	blkHeader *ctypes.BeaconBlockHeader
 }
 
@@ -35,11 +34,10 @@ func (s *ConsensusSidecars[SidecarsT]) New(
 	sidecars SidecarsT,
 	blkHeader *ctypes.BeaconBlockHeader,
 ) *ConsensusSidecars[SidecarsT] {
-	s = &ConsensusSidecars[SidecarsT]{
+	return &ConsensusSidecars[SidecarsT]{
 		sidecars:  sidecars,
 		blkHeader: blkHeader,
 	}
-	return s
 }
 
 func (s *ConsensusSidecars[SidecarsT]) GetSidecars() SidecarsT {

da/blob/verifier.go

@@ -28,7 +28,9 @@ import (
 	"github.com/berachain/beacon-kit/chain-spec/chain"
 	ctypes "github.com/berachain/beacon-kit/consensus-types/types"
 	"github.com/berachain/beacon-kit/da/kzg"
+	"github.com/berachain/beacon-kit/errors"
 	"github.com/berachain/beacon-kit/primitives/crypto"
+	"github.com/berachain/beacon-kit/primitives/eip4844"
 	"github.com/berachain/beacon-kit/primitives/math"
 	"golang.org/x/sync/errgroup"
 )
@@ -80,10 +82,24 @@ func (bv *verifier[_, BlobSidecarsT]) verifySidecars(
 
 	g, _ := errgroup.WithContext(context.Background())
 
-	// Verifying that sidecars block headers match with header of the
-	// corresponding block concurrently.
+	// create lookup table to check for duplicate commitments
+	duplicateCommitment := make(map[eip4844.KZGCommitment]struct{})
+
+	// Validate sidecar fields against data from the BeaconBlock.
 	for i, s := range sidecars.GetSidecars() {
+		// Check if sidecar's kzgCommitment is duplicate. Along with the
+		// length check and the inclusion proof, this fully verifies that
+		// the KzgCommitments in the BlobSidecar are the exact same as the
+		// ones in the BeaconBlockBody without having to explicitly compare.
+		if _, exists := duplicateCommitment[s.GetKzgCommitment()]; exists {
+			return errors.New(
+				"found duplicate KzgCommitments in BlobSidecars",
+			)
+		}
+		duplicateCommitment[s.GetKzgCommitment()] = struct{}{}
+
 		g.Go(func() error {
+			// Verify the signature.
 			var sigHeader = s.GetSignedBeaconBlockHeader()
 
 			// Check BlobSidecar.Header equality with BeaconBlockHeader

da/types/sidecars.go

@@ -33,7 +33,7 @@ type BlobSidecars struct {
 	Sidecars []*BlobSidecar
 }
 
-// NewBlobSidecars creates a new BlobSidecars object.
+// Empty creates a new empty BlobSidecars object.
 func (bs *BlobSidecars) Empty() *BlobSidecars {
 	return &BlobSidecars{}
 }

da/types/sidecars_test.go

@@ -152,3 +152,10 @@ func TestValidateBlockRoots(t *testing.T) {
 		"Validating sidecar with invalid roots should produce an error",
 	)
 }
+
+func TestZeroSidecarsInBlobSidecarsIsNotNil(t *testing.T) {
+	// This test exists to ensure that proposing a BlobSidecars with 0
+	// Sidecars is not considered IsNil().
+	sidecars := &types.BlobSidecars{Sidecars: make([]*types.BlobSidecar, 0)}
+	require.False(t, sidecars.IsNil())
+}