more audit todos and fixes (#2489)

consensus/consensus.go

@@ -79,8 +79,9 @@ type Consensus struct {
 	MinPeers   int
 	pubKeyLock sync.Mutex
 	// private/public keys of current node
-	priKey        *multibls.PrivateKey
-	PubKey        *multibls.PublicKey
+	priKey *multibls.PrivateKey
+	PubKey *multibls.PublicKey
+	// TODO(audit): SelfAddresses doesn't have the ECDSA address for external validators. Don't use it that way.
 	SelfAddresses map[string]common.Address
 	// the publickey of leader
 	LeaderPubKey *bls.PublicKey

consensus/leader.go

@@ -209,6 +209,7 @@ func (consensus *Consensus) onCommit(msg *msg_pb.Message) {
 	consensus.mutex.Lock()
 	defer consensus.mutex.Unlock()
 
+	// TODO(audit): refactor into a new func
 	if key := (bls.PublicKey{}); consensus.couldThisBeADoubleSigner(recvMsg) {
 		if alreadyCastBallot := consensus.Decider.ReadBallot(
 			quorum.Commit, recvMsg.SenderPubkey,

core/rawdb/accessors_offchain.go

@@ -106,6 +106,7 @@ func DeletePendingCrossLinks(db DatabaseDeleter) error {
 }
 
 // ReadPendingSlashingCandidates retrieves last pending slashing candidates
+// TODO(audit): the pending slashes written in DB is never used.
 func ReadPendingSlashingCandidates(db DatabaseReader) ([]byte, error) {
 	return db.Get(pendingSlashingKey)
 }

internal/chain/engine.go

@@ -290,6 +290,8 @@ func (e *engineImpl) Finalize(
 		if err := applySlashes(chain, header, state, doubleSigners); err != nil {
 			return nil, nil, err
 		}
+	} else if len(doubleSigners) > 0 {
+		return nil, nil, errors.New("slashes proposed in non-beacon chain or non-staking epoch")
 	}
 
 	// Finalize the state root
@@ -370,6 +372,7 @@ func applySlashes(
 	state *state.DB,
 	doubleSigners slash.Records,
 ) error {
+	// TODO(audit): should read from the epoch when the slash happened
 	superCommittee, err := chain.ReadShardState(chain.CurrentHeader().Epoch())
 
 	if err != nil {
@@ -379,6 +382,8 @@ func applySlashes(
 	staked := superCommittee.StakedValidators()
 	// Apply the slashes, invariant: assume been verified as legit slash by this point
 	var slashApplied *slash.Application
+	// TODO(audit): need to group doubleSigners by the target (block) and slash them separately
+	//              the rate of slash should be based on num_keys_signed_on_same_block/total_bls_key
 	rate := slash.Rate(len(doubleSigners), staked.CountStakedBLSKey)
 	utils.Logger().Info().
 		Str("rate", rate.String()).

node/worker/worker.go

@@ -359,7 +359,11 @@ func (w *Worker) verifySlashes(
 		},
 	)
 
-	workingState := w.GetCurrentState()
+	// Always base the state on current tip of the chain
+	workingState, err := w.chain.State()
+	if err != nil {
+		return successes, failures
+	}
 
 	for i := range d {
 		if err := slash.Verify(

shard/committee/assignment.go

@@ -171,6 +171,7 @@ func eposStakedCommittee(
 		if validator.EPOSStatus != effective.Active {
 			continue
 		}
+		// TODO(audit): remove the sanity check here; do the sanity check with maxBLSKey before validator change
 		if err := validator.SanityCheck(maxBLSKey); err != nil {
 			utils.Logger().Info().
 				Int("staked-candidates", len(candidates)).
@@ -178,10 +179,7 @@ func eposStakedCommittee(
 				Msg("validator sanity check failed")
 			continue
 		}
-		validatorStake := big.NewInt(0)
-		for _, delegation := range validator.Delegations {
-			validatorStake.Add(validatorStake, delegation.Amount)
-		}
+		totalStake := validator.TotalDelegation()
 
 		found := false
 		dupKey := shard.BlsPublicKey{}
@@ -202,28 +200,23 @@ func eposStakedCommittee(
 		}
 
 		essentials[validator.Address] = effective.SlotOrder{
-			validatorStake,
+			totalStake,
 			validator.SlotPubKeys,
 		}
 	}
 
-	staked := effective.Apply(essentials, stakedSlotsCount)
+	electedSlots := effective.Apply(essentials, stakedSlotsCount)
 	shardBig := big.NewInt(int64(shardCount))
 
-	if l := len(staked); l < stakedSlotsCount {
-		// WARN unlikely to happen in production but will happen as we are developing
-		stakedSlotsCount = l
-	}
-
-	totalStake := numeric.ZeroDec()
+	totalEffectiveStake := numeric.ZeroDec()
 
-	for i := 0; i < stakedSlotsCount; i++ {
-		shardID := int(new(big.Int).Mod(staked[i].BlsPublicKey.Big(), shardBig).Int64())
-		slot := staked[i]
-		totalStake = totalStake.Add(slot.Dec)
+	for i := 0; i < len(electedSlots); i++ {
+		slot := electedSlots[i]
+		shardID := int(new(big.Int).Mod(slot.BlsPublicKey.Big(), shardBig).Int64())
+		totalEffectiveStake = totalEffectiveStake.Add(slot.Dec)
 		shardState.Shards[shardID].Slots = append(shardState.Shards[shardID].Slots, shard.Slot{
 			slot.Address,
-			staked[i].BlsPublicKey,
+			slot.BlsPublicKey,
 			&slot.Dec,
 		})
 	}
@@ -231,7 +224,7 @@ func eposStakedCommittee(
 	if c := len(candidates); c != 0 {
 		utils.Logger().Info().
 			Int("staked-candidates", c).
-			Str("total-staked-by-validators", totalStake.String()).
+			Str("sum-all-effective-stake-by-validators", totalEffectiveStake.String()).
 			Msg("epos based super-committe")
 	}
 

staking/effective/calculate.go

@@ -68,7 +68,6 @@ func (s Slots) JSON() string {
 
 // Median ..
 func Median(stakes []SlotPurchase) numeric.Dec {
-
 	if len(stakes) == 0 {
 		utils.Logger().Error().Int("non-zero", len(stakes)).
 			Msg("Input to median has len 0, check caller")
@@ -76,7 +75,7 @@ func Median(stakes []SlotPurchase) numeric.Dec {
 
 	sort.SliceStable(
 		stakes,
-		func(i, j int) bool { return stakes[i].Dec.LT(stakes[j].Dec) },
+		func(i, j int) bool { return stakes[i].Dec.GT(stakes[j].Dec) },
 	)
 	const isEven = 0
 	switch l := len(stakes); l % 2 {
@@ -132,9 +131,6 @@ func Compute(
 			})
 		}
 	}
-	if len(eposedSlots) < len(shortHand) {
-		// WARN Should never happen
-	}
 
 	sort.SliceStable(
 		eposedSlots,

staking/slash/double-sign.go

@@ -186,9 +186,8 @@ func Verify(
 		)
 	}
 	currentEpoch := chain.CurrentBlock().Epoch()
-	// on beaconchain committee, the slash can't come from the future
-	if candidate.Evidence.ShardID == shard.BeaconChainShardID &&
-		candidate.Evidence.Epoch.Cmp(currentEpoch) == 1 {
+	// the slash can't come from the future (shard chain's epoch can't be larger than beacon chain's)
+	if candidate.Evidence.Epoch.Cmp(currentEpoch) == 1 {
 		return errors.Wrapf(
 			errSlashFromFutureEpoch, "current-epoch %v", currentEpoch,
 		)
@@ -232,6 +231,7 @@ func Verify(
 		}
 
 		blockNumBytes := make([]byte, 8)
+		// TODO(audit): add view ID into signature payload
 		binary.LittleEndian.PutUint64(blockNumBytes, ballot.Height)
 		commitPayload := append(blockNumBytes, ballot.BlockHeaderHash[:]...)
 		if !signature.VerifyHash(publicKey, commitPayload) {
@@ -362,7 +362,7 @@ func delegatorSlashApply(
 						}
 
 						if nowAmt.Cmp(common.Big0) == 0 {
-							// TODO need to remove the undelegate
+							// TODO(audit): need to remove the undelegate
 							utils.Logger().Info().
 								RawJSON("delegation-snapshot", []byte(delegationSnapshot.String())).
 								RawJSON("delegation-current", []byte(delegationNow.String())).