diff --git a/substrate/client/consensus/babe/src/authorship.rs b/substrate/client/consensus/babe/src/authorship.rs index 7a9b09495c..43df26a9a2 100644 --- a/substrate/client/consensus/babe/src/authorship.rs +++ b/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, diff --git a/substrate/frame/babe/src/lib.rs b/substrate/frame/babe/src/lib.rs index e578f0695a..1effc2c198 100644 --- a/substrate/frame/babe/src/lib.rs +++ b/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::::InvalidConfiguration + ); + }, + } PendingEpochConfigChange::::put(config); Ok(()) }