Sanity check for Babe's configuration (#11385)

* Prevent div by zero in native babe code
* Additional sanity check for babe config
* Further sanity checks and postpone threshold computation
* Apply suggestions from code review
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/consensus/babe/src/authorship.rs

@@ -41,6 +41,14 @@ pub(super) fn calculate_primary_threshold(
 	use num_rational::BigRational;
 	use num_traits::{cast::ToPrimitive, identities::One};
 
+	// Prevent div by zero and out of bounds access.
+	// While Babe's pallet implementation that ships with FRAME performs a sanity check over
+	// configuration parameters, this is not sufficient to guarantee that `c.1` is non-zero
+	// (i.e. third party implementations are possible).
+	if c.1 == 0 || authority_index >= authorities.len() {
+		return 0
+	}
+
 	let c = c.0 as f64 / c.1 as f64;
 
 	let theta = authorities[authority_index].1 as f64 /
@@ -235,12 +243,6 @@ fn claim_primary_slot(
 	for (authority_id, authority_index) in keys {
 		let transcript = make_transcript(randomness, slot, *epoch_index);
 		let transcript_data = make_transcript_data(randomness, slot, *epoch_index);
-		// Compute the threshold we will use.
-		//
-		// We already checked that authorities contains `key.public()`, so it can't
-		// be empty.  Therefore, this division in `calculate_threshold` is safe.
-		let threshold = calculate_primary_threshold(c, authorities, *authority_index);
-
 		let result = SyncCryptoStore::sr25519_vrf_sign(
 			&**keystore,
 			AuthorityId::ID,
@@ -253,6 +255,8 @@ fn claim_primary_slot(
 				Ok(inout) => inout,
 				Err(_) => continue,
 			};
+
+			let threshold = calculate_primary_threshold(c, authorities, *authority_index);
 			if check_primary_threshold(&inout, threshold) {
 				let pre_digest = PreDigest::Primary(PrimaryPreDigest {
 					slot,

substrate/frame/babe/src/lib.rs

@@ -24,6 +24,7 @@
 use codec::{Decode, Encode};
 use frame_support::{
 	dispatch::DispatchResultWithPostInfo,
+	ensure,
 	traits::{
 		ConstU32, DisabledValidators, FindAuthor, Get, KeyOwnerProofSystem, OnTimestampSet,
 		OneSessionHandler,
@@ -42,8 +43,8 @@ use sp_std::prelude::*;
 
 use sp_consensus_babe::{
 	digests::{NextConfigDescriptor, NextEpochDescriptor, PreDigest},
-	BabeAuthorityWeight, BabeEpochConfiguration, ConsensusLog, Epoch, EquivocationProof, Slot,
-	BABE_ENGINE_ID,
+	AllowedSlots, BabeAuthorityWeight, BabeEpochConfiguration, ConsensusLog, Epoch,
+	EquivocationProof, Slot, BABE_ENGINE_ID,
 };
 use sp_consensus_vrf::schnorrkel;
 
@@ -185,6 +186,8 @@ pub mod pallet {
 		InvalidKeyOwnershipProof,
 		/// A given equivocation report is valid but already previously reported.
 		DuplicateOffenceReport,
+		/// Submitted configuration is invalid.
+		InvalidConfiguration,
 	}
 
 	/// Current epoch index.
@@ -447,6 +450,14 @@ pub mod pallet {
 			config: NextConfigDescriptor,
 		) -> DispatchResult {
 			ensure_root(origin)?;
+			match config {
+				NextConfigDescriptor::V1 { c, allowed_slots } => {
+					ensure!(
+						(c.0 != 0 || allowed_slots != AllowedSlots::PrimarySlots) && c.1 != 0,
+						Error::<T>::InvalidConfiguration
+					);
+				},
+			}
 			PendingEpochConfigChange::<T>::put(config);
 			Ok(())
 		}