fix: Potential Vulnerabilities Identified Through Static Code Analysis (

#1736)

* removed rand.Seed usage

* removed few type conflicts + returned back linter rule G115

* few more gosec G115 fixes

* few fixes and comments added

* minor changes

* added //nolint:gosec in time conversions which can't be resolved

* added nolint:gosec reason

* updated to golangcli-lint 1.61.0

* super minor refactoring uint64 for cfg.GetValidatorStats()

* refactoring

* added conversion wrapper to devide all existing G115 issues

* refactored many G115 issues

* minor fix

* fixed expected value returned from mock

* removed unused code

* renames

* cr: role to int32

* cr: remove len conversion, add duration conversion check, create var for cutoffround

* fix

* rename package from conversion to casts

* readability

* remove unnecessary loop gymnastics

* remove more loop gymnastics

* add nosec for trusted subnet input

* approve spec diffs

* approve spec diff

* approve genesis spec diff

* fix: duty count uint64

* fix test

* remove extra debug log

---------

Co-authored-by: moshe-blox <[email protected]>
Co-authored-by: y0sher <[email protected]>

.golangci.yaml

@@ -82,10 +82,6 @@ output:
 
 # all available settings of specific linters
 linters-settings:
-  gosec:
-    # TODO: fix all issues with int overflow and return this rule back
-    excludes:
-      - G115
   dogsled:
     # checks assignments with too many blank identifiers; default is 2
     max-blank-identifiers: 2

api/handling.go

@@ -18,12 +18,18 @@ type HandlerFunc func(http.ResponseWriter, *http.Request) error
 func Handler(h HandlerFunc) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		if err := h(w, r); err != nil {
-			//nolint:all
+			//nolint:errorlint
+			// errors.As would be incorrect here since a renderer.Renderer
+			// wrapped inside another error should error, not render.
 			switch e := err.(type) {
 			case render.Renderer:
-				render.Render(w, r, e)
+				if err := render.Render(w, r, e); err != nil {
+					http.Error(w, err.Error(), http.StatusInternalServerError)
+				}
 			default:
-				render.Render(w, r, Error(err))
+				if err := render.Render(w, r, Error(err)); err != nil {
+					http.Error(w, err.Error(), http.StatusInternalServerError)
+				}
 			}
 		}
 	}

beacon/goclient/goclient.go

@@ -23,6 +23,7 @@ import (
 	operatordatastore "github.com/ssvlabs/ssv/operator/datastore"
 	"github.com/ssvlabs/ssv/operator/slotticker"
 	beaconprotocol "github.com/ssvlabs/ssv/protocol/v2/blockchain/beacon"
+	"github.com/ssvlabs/ssv/utils/casts"
 )
 
 const (
@@ -263,8 +264,8 @@ func (gc *GoClient) GetBeaconNetwork() spectypes.BeaconNetwork {
 // SlotStartTime returns the start time in terms of its unix epoch
 // value.
 func (gc *GoClient) slotStartTime(slot phase0.Slot) time.Time {
-	duration := time.Second * time.Duration(uint64(slot)*uint64(gc.network.SlotDurationSec().Seconds()))
-	startTime := time.Unix(int64(gc.network.MinGenesisTime()), 0).Add(duration)
+	duration := time.Second * casts.DurationFromUint64(uint64(slot)*uint64(gc.network.SlotDurationSec().Seconds()))
+	startTime := time.Unix(gc.network.MinGenesisTime(), 0).Add(duration)
 	return startTime
 }
 

cli/operator/node.go

@@ -438,6 +438,7 @@ func validateConfig(nodeStorage operatorstorage.Storage, networkName string, usi
 			return fmt.Errorf("incompatible config change: %w", err)
 		}
 	} else {
+
 		if err := nodeStorage.SaveConfig(nil, currentConfig); err != nil {
 			return fmt.Errorf("failed to store config: %w", err)
 		}

cli/threshold.go

@@ -35,8 +35,8 @@ var createThresholdCmd = &cobra.Command{
 			logger.Fatal("failed to get keys count flag value", zap.Error(err))
 		}
 
-		if !ssvtypes.ValidCommitteeSize(int(keysCount)) {
-			logger.Fatal("invalid keys count", zap.Int("keysCount", int(keysCount)))
+		if !ssvtypes.ValidCommitteeSize(keysCount) {
+			logger.Fatal("invalid keys count", zap.Uint64("keysCount", keysCount))
 		}
 
 		baseKey := &bls.SecretKey{}
@@ -47,7 +47,7 @@ var createThresholdCmd = &cobra.Command{
 		// https://github.com/ethereum/eth2-ssv/issues/22
 		// currently support 4, 7, 10, 13 nodes threshold 3f+1. need to align based open the issue to
 		// support k(2f+1) and n (3f+1) and allow to pass it as flag
-		quorum, _ := ssvtypes.ComputeQuorumAndPartialQuorum(int(keysCount))
+		quorum, _ := ssvtypes.ComputeQuorumAndPartialQuorum(keysCount)
 		privKeys, err := threshold.Create(baseKey.Serialize(), quorum, keysCount)
 		if err != nil {
 			logger.Fatal("failed to turn a private key into a threshold key", zap.Error(err))

eth/eventhandler/validation.go

@@ -25,7 +25,7 @@ func (eh *EventHandler) validateOperators(txn basedb.Txn, operators []uint64) er
 		return fmt.Errorf("no operators")
 	}
 
-	if !ssvtypes.ValidCommitteeSize(len(operators)) {
+	if !ssvtypes.ValidCommitteeSize(uint64(len(operators))) {
 		return fmt.Errorf("given operator count (%d) cannot build a 3f+1 quorum", operatorCount)
 	}
 

exporter/api/query_handlers.go

@@ -12,6 +12,7 @@ import (
 	"github.com/ssvlabs/ssv/ibft/storage"
 	"github.com/ssvlabs/ssv/logging/fields"
 	"github.com/ssvlabs/ssv/protocol/v2/message"
+	"github.com/ssvlabs/ssv/utils/casts"
 )
 
 const (
@@ -72,7 +73,7 @@ func HandleParticipantsQuery(logger *zap.Logger, qbftStorage *storage.QBFTStores
 		nm.Msg = res
 		return
 	}
-	runnerRole := convert.RunnerRole(beaconRole)
+	runnerRole := casts.BeaconRoleToConvertRole(beaconRole)
 	roleStorage := qbftStorage.Get(runnerRole)
 	if roleStorage == nil {
 		logger.Warn("role storage doesn't exist", fields.ExporterRole(runnerRole))

exporter/convert/message.go

@@ -3,6 +3,7 @@ package convert
 import (
 	"encoding/binary"
 	"encoding/hex"
+	"math"
 
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 )
@@ -29,13 +30,24 @@ func (msg MessageID) GetDutyExecutorID() []byte {
 
 func (msg MessageID) GetRoleType() RunnerRole {
 	roleByts := msg[roleTypeStartPos : roleTypeStartPos+roleTypeSize]
-	return RunnerRole(binary.LittleEndian.Uint32(roleByts))
+	roleValue := binary.LittleEndian.Uint32(roleByts)
+
+	// Sanitize RoleValue
+	if roleValue > math.MaxInt32 {
+		return spectypes.RoleUnknown
+	}
+
+	return RunnerRole(roleValue)
 }
 
 func NewMsgID(domain spectypes.DomainType, dutyExecutorID []byte, role RunnerRole) MessageID {
+	// Sanitize role. If bad role, return an empty MessageID
+	roleValue := int32(role)
+	if roleValue < 0 {
+		return MessageID{}
+	}
 	roleByts := make([]byte, 4)
-	binary.LittleEndian.PutUint32(roleByts, uint32(role))
-
+	binary.LittleEndian.PutUint32(roleByts, uint32(roleValue))
 	return newMessageID(domain[:], roleByts, dutyExecutorID)
 }
 

integration/qbft/tests/test_duty.go

@@ -28,6 +28,7 @@ func createDuty(pk []byte, slot phase0.Slot, idx phase0.ValidatorIndex, role spe
 	var beaconRole spectypes.BeaconRole
 	switch role {
 	case spectypes.RoleCommittee:
+		// #nosec G115
 		return spectestingutils.TestingCommitteeAttesterDuty(slot, []int{int(idx)})
 	case spectypes.RoleAggregator:
 		testingDuty = spectestingutils.TestingAggregatorDuty

message/validation/common_checks.go

@@ -77,7 +77,7 @@ func (mv *messageValidator) validateDutyCount(
 	return nil
 }
 
-func (mv *messageValidator) dutyLimit(msgID spectypes.MessageID, slot phase0.Slot, validatorIndices []phase0.ValidatorIndex) (int, bool) {
+func (mv *messageValidator) dutyLimit(msgID spectypes.MessageID, slot phase0.Slot, validatorIndices []phase0.ValidatorIndex) (uint64, bool) {
 	switch msgID.GetRoleType() {
 	case spectypes.RoleVoluntaryExit:
 		pk := phase0.BLSPubKey{}
@@ -89,8 +89,8 @@ func (mv *messageValidator) dutyLimit(msgID spectypes.MessageID, slot phase0.Slo
 		return 2, true
 
 	case spectypes.RoleCommittee:
-		validatorIndexCount := len(validatorIndices)
-		slotsPerEpoch := int(mv.netCfg.Beacon.SlotsPerEpoch())
+		validatorIndexCount := uint64(len(validatorIndices))
+		slotsPerEpoch := mv.netCfg.Beacon.SlotsPerEpoch()
 
 		// Skip duty search if validators * 2 exceeds slots per epoch,
 		// as the maximum duties per epoch is capped at the number of slots.

message/validation/consensus_state.go

@@ -36,8 +36,8 @@ type OperatorState struct {
 	state           []*SignerState // the slice index is slot % storedSlotCount
 	maxSlot         phase0.Slot
 	maxEpoch        phase0.Epoch
-	lastEpochDuties int
-	prevEpochDuties int
+	lastEpochDuties uint64
+	prevEpochDuties uint64
 }
 
 func newOperatorState(size phase0.Slot) *OperatorState {
@@ -50,7 +50,7 @@ func (os *OperatorState) Get(slot phase0.Slot) *SignerState {
 	os.mu.RLock()
 	defer os.mu.RUnlock()
 
-	s := os.state[int(slot)%len(os.state)]
+	s := os.state[(uint64(slot) % uint64(len(os.state)))]
 	if s == nil || s.Slot != slot {
 		return nil
 	}
@@ -62,7 +62,7 @@ func (os *OperatorState) Set(slot phase0.Slot, epoch phase0.Epoch, state *Signer
 	os.mu.Lock()
 	defer os.mu.Unlock()
 
-	os.state[int(slot)%len(os.state)] = state
+	os.state[uint64(slot)%uint64(len(os.state))] = state
 	if slot > os.maxSlot {
 		os.maxSlot = slot
 	}
@@ -82,7 +82,7 @@ func (os *OperatorState) MaxSlot() phase0.Slot {
 	return os.maxSlot
 }
 
-func (os *OperatorState) DutyCount(epoch phase0.Epoch) int {
+func (os *OperatorState) DutyCount(epoch phase0.Epoch) uint64 {
 	os.mu.RLock()
 	defer os.mu.RUnlock()
 

message/validation/consensus_state_test.go

@@ -62,18 +62,18 @@ func TestOperatorState(t *testing.T) {
 
 		os.Set(slot, epoch, signerState)
 
-		require.Equal(t, os.DutyCount(epoch), 1)
-		require.Equal(t, os.DutyCount(epoch-1), 0)
+		require.Equal(t, os.DutyCount(epoch), uint64(1))
+		require.Equal(t, os.DutyCount(epoch-1), uint64(0))
 
 		slot2 := phase0.Slot(6)
 		epoch2 := phase0.Epoch(2)
 		signerState2 := &SignerState{Slot: slot2}
 
 		os.Set(slot2, epoch2, signerState2)
 
-		require.Equal(t, os.DutyCount(epoch2), 1)
-		require.Equal(t, os.DutyCount(epoch), 1)
-		require.Equal(t, os.DutyCount(epoch-1), 0)
+		require.Equal(t, os.DutyCount(epoch2), uint64(1))
+		require.Equal(t, os.DutyCount(epoch), uint64(1))
+		require.Equal(t, os.DutyCount(epoch-1), uint64(0))
 	})
 
 	t.Run("TestIncrementLastEpochDuties", func(t *testing.T) {
@@ -85,12 +85,12 @@ func TestOperatorState(t *testing.T) {
 		signerState := &SignerState{Slot: slot}
 
 		os.Set(slot, epoch, signerState)
-		require.Equal(t, os.DutyCount(epoch), 1)
+		require.Equal(t, os.DutyCount(epoch), uint64(1))
 
 		slot2 := phase0.Slot(6)
 		signerState2 := &SignerState{Slot: slot2}
 		os.Set(slot2, epoch, signerState2)
 
-		require.Equal(t, os.DutyCount(epoch), 2)
+		require.Equal(t, os.DutyCount(epoch), uint64(2))
 	})
 }

message/validation/consensus_validation.go

@@ -18,6 +18,7 @@ import (
 	"github.com/ssvlabs/ssv/protocol/v2/message"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer"
 	ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types"
+	"github.com/ssvlabs/ssv/utils/casts"
 )
 
 func (mv *messageValidator) validateConsensusMessage(
@@ -81,7 +82,7 @@ func (mv *messageValidator) validateConsensusMessageSemantics(
 	committee []spectypes.OperatorID,
 ) error {
 	signers := signedSSVMessage.OperatorIDs
-	quorumSize, _ := ssvtypes.ComputeQuorumAndPartialQuorum(len(committee))
+	quorumSize, _ := ssvtypes.ComputeQuorumAndPartialQuorum(uint64(len(committee)))
 	msgType := consensusMessage.MsgType
 
 	if len(signers) > 1 {
@@ -376,14 +377,22 @@ func (mv *messageValidator) maxRound(role spectypes.RunnerRole) (specqbft.Round,
 	}
 }
 
-func (mv *messageValidator) currentEstimatedRound(sinceSlotStart time.Duration) specqbft.Round {
-	if currentQuickRound := specqbft.FirstRound + specqbft.Round(sinceSlotStart/roundtimer.QuickTimeout); currentQuickRound <= roundtimer.QuickTimeoutThreshold {
-		return currentQuickRound
+func (mv *messageValidator) currentEstimatedRound(sinceSlotStart time.Duration) (specqbft.Round, error) {
+	delta, err := casts.DurationToUint64(sinceSlotStart / roundtimer.QuickTimeout)
+	if err != nil {
+		return 0, fmt.Errorf("failed to convert time duration to uint64: %w", err)
+	}
+	if currentQuickRound := specqbft.FirstRound + specqbft.Round(delta); currentQuickRound <= roundtimer.QuickTimeoutThreshold {
+		return currentQuickRound, nil
 	}
 
 	sinceFirstSlowRound := sinceSlotStart - (time.Duration(roundtimer.QuickTimeoutThreshold) * roundtimer.QuickTimeout)
-	estimatedRound := roundtimer.QuickTimeoutThreshold + specqbft.FirstRound + specqbft.Round(sinceFirstSlowRound/roundtimer.SlowTimeout)
-	return estimatedRound
+	delta, err = casts.DurationToUint64(sinceFirstSlowRound / roundtimer.SlowTimeout)
+	if err != nil {
+		return 0, fmt.Errorf("failed to convert time duration to uint64: %w", err)
+	}
+	estimatedRound := roundtimer.QuickTimeoutThreshold + specqbft.FirstRound + specqbft.Round(delta)
+	return estimatedRound, nil
 }
 
 func (mv *messageValidator) validConsensusMsgType(msgType specqbft.MessageType) bool {
@@ -407,7 +416,11 @@ func (mv *messageValidator) roundBelongsToAllowedSpread(
 	estimatedRound := specqbft.FirstRound
 	if receivedAt.After(slotStartTime) {
 		sinceSlotStart = receivedAt.Sub(slotStartTime)
-		estimatedRound = mv.currentEstimatedRound(sinceSlotStart)
+		currentEstimatedRound, err := mv.currentEstimatedRound(sinceSlotStart)
+		if err != nil {
+			return err
+		}
+		estimatedRound = currentEstimatedRound
 	}
 
 	// TODO: lowestAllowed is not supported yet because first round is non-deterministic now
@@ -427,12 +440,12 @@ func (mv *messageValidator) roundBelongsToAllowedSpread(
 }
 
 func (mv *messageValidator) roundRobinProposer(height specqbft.Height, round specqbft.Round, committee []spectypes.OperatorID) types.OperatorID {
-	firstRoundIndex := 0
+	firstRoundIndex := uint64(0)
 	if height != specqbft.FirstHeight {
-		firstRoundIndex += int(height) % len(committee)
+		firstRoundIndex += uint64(height) % uint64(len(committee))
 	}
 
-	index := (firstRoundIndex + int(round) - int(specqbft.FirstRound)) % len(committee)
+	index := (firstRoundIndex + uint64(round) - uint64(specqbft.FirstRound)) % uint64(len(committee))
 	return committee[index]
 }
 

message/validation/consensus_validation_test.go

@@ -98,7 +98,8 @@ func TestMessageValidator_currentEstimatedRound(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			mv := &messageValidator{}
-			got := mv.currentEstimatedRound(tc.sinceSlotStart)
+			got, err := mv.currentEstimatedRound(tc.sinceSlotStart)
+			require.NoError(t, err)
 			require.Equal(t, tc.want, got)
 		})
 	}