Check non-nil validator before accessing withdrawal credentials (#14705)

* Check non-nil validator before accessing withdrawal credentials

* Updated changelog

Author: prestonvanloon

CHANGELOG.md

@@ -119,6 +119,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
 - P2P: Avoid infinite loop when looking for peers in small networks.
 - Fixed another rollback bug due to a context deadline.
 - Fix checkpoint sync bug on holesky. [pr](https://github.com/prysmaticlabs/prysm/pull/14689)
+- Added check to prevent nil pointer deference or out of bounds array access when validating the BLSToExecutionChange on an impossibly nil validator.
 - Fix proposer boost spec tests being flakey by adjusting start time from 3 to 2s into slot.
 - Fix segmentation fault in E2E when light-client feature flag is enabled. [PR](https://github.com/prysmaticlabs/prysm/pull/14699)
 - Fix `searchForPeers` infinite loop in small networks.

beacon-chain/core/blocks/exports_test.go

@@ -1,5 +1,5 @@
 package blocks
 
 var ProcessBLSToExecutionChange = processBLSToExecutionChange
-
+var ErrInvalidBLSPrefix = errInvalidBLSPrefix
 var VerifyBlobCommitmentCount = verifyBlobCommitmentCount

beacon-chain/core/blocks/withdrawals.go

@@ -100,8 +100,11 @@ func ValidateBLSToExecutionChange(st state.ReadOnlyBeaconState, signed *ethpb.Si
 	if err != nil {
 		return nil, err
 	}
+	if val == nil {
+		return nil, errors.Wrap(errInvalidWithdrawalCredentials, "validator is nil") // This should not be possible.
+	}
 	cred := val.WithdrawalCredentials
-	if cred[0] != params.BeaconConfig().BLSWithdrawalPrefixByte {
+	if len(cred) < 2 || cred[0] != params.BeaconConfig().BLSWithdrawalPrefixByte {
 		return nil, errInvalidBLSPrefix
 	}
 

beacon-chain/core/blocks/withdrawals_test.go

@@ -113,7 +113,42 @@ func TestProcessBLSToExecutionChange(t *testing.T) {
 		require.NoError(t, err)
 		require.DeepEqual(t, digest[:], val.WithdrawalCredentials)
 	})
+	t.Run("nil validator does not panic", func(t *testing.T) {
+		priv, err := bls.RandKey()
+		require.NoError(t, err)
+		pubkey := priv.PublicKey().Marshal()
 
+		message := &ethpb.BLSToExecutionChange{
+			ToExecutionAddress: []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13},
+			ValidatorIndex:     0,
+			FromBlsPubkey:      pubkey,
+		}
+
+		registry := []*ethpb.Validator{
+			nil,
+		}
+		st, err := state_native.InitializeFromProtoPhase0(&ethpb.BeaconState{
+			Validators: registry,
+			Fork: &ethpb.Fork{
+				CurrentVersion:  params.BeaconConfig().GenesisForkVersion,
+				PreviousVersion: params.BeaconConfig().GenesisForkVersion,
+			},
+			Slot: params.BeaconConfig().SlotsPerEpoch * 5,
+		})
+		require.NoError(t, err)
+
+		signature, err := signing.ComputeDomainAndSign(st, time.CurrentEpoch(st), message, params.BeaconConfig().DomainBLSToExecutionChange, priv)
+		require.NoError(t, err)
+
+		signed := &ethpb.SignedBLSToExecutionChange{
+			Message:   message,
+			Signature: signature,
+		}
+		_, err = blocks.ValidateBLSToExecutionChange(st, signed)
+		// The state should return an empty validator, even when the validator object in the registry is
+		// nil. This error should return when the withdrawal credentials are invalid or too short.
+		require.ErrorIs(t, err, blocks.ErrInvalidBLSPrefix)
+	})
 	t.Run("non-existent validator", func(t *testing.T) {
 		priv, err := bls.RandKey()
 		require.NoError(t, err)