Fix Off by one error for the RefreshVote proposal (#489)

* add InvalidNonce error

* inc the nonce by one

* add nonce to the emitted events

* use next nonce when we rotate

pallets/dkg-metadata/src/lib.rs

@@ -539,6 +539,8 @@ pub mod pallet {
 		UsedSignature,
 		/// Invalid public key signature submission
 		InvalidSignature,
+		/// Invalid Nonece used, must be greater than [`refresh_nonce`].
+		InvalidNonce,
 		/// Invalid misbehaviour reports
 		InvalidMisbehaviourReports,
 		/// DKG Refresh is already in progress.
@@ -562,6 +564,7 @@ pub mod pallet {
 			pub_key_sig: Vec<u8>,
 			compressed_pub_key: Vec<u8>,
 			uncompressed_pub_key: Vec<u8>,
+			nonce: u32,
 		},
 		/// Current Public Key Changed.
 		PublicKeyChanged { compressed_pub_key: Vec<u8>, uncompressed_pub_key: Vec<u8> },
@@ -570,6 +573,7 @@ pub mod pallet {
 			pub_key_sig: Vec<u8>,
 			compressed_pub_key: Vec<u8>,
 			uncompressed_pub_key: Vec<u8>,
+			nonce: u32,
 		},
 		/// Misbehaviour reports submitted
 		MisbehaviourReportsSubmitted {
@@ -857,19 +861,20 @@ pub mod pallet {
 				Error::<T>::AlreadySubmittedSignature
 			);
 			let used_signatures = Self::used_signatures();
+			let last_used_nonce = Self::refresh_nonce();
+			let next_nonce = last_used_nonce.saturating_add(1);
 			// Deconstruct the signature and nonce for easier access
 			let (nonce, signature) = (signature_proposal.nonce, signature_proposal.signature);
-			ensure!(
-				nonce == Self::refresh_nonce().into() || !signature.is_empty(),
-				Error::<T>::InvalidSignature
-			);
+			ensure!(u32::from(nonce) == next_nonce, Error::<T>::InvalidNonce);
+			ensure!(!signature.is_empty(), Error::<T>::InvalidSignature);
 			ensure!(!used_signatures.contains(&signature), Error::<T>::UsedSignature);
 
 			let (_, next_pub_key) = Self::next_dkg_public_key().unwrap();
 			let uncompressed_pub_key =
 				Self::decompress_public_key(next_pub_key.clone()).unwrap_or_default();
 			let data = RefreshProposal {
-				nonce: Self::refresh_nonce().into(),
+				// We ensure that the nonce is valid above, so this is safe.
+				nonce,
 				pub_key: uncompressed_pub_key.clone(),
 			};
 			// Verify signature against the `RefreshProposal`
@@ -880,8 +885,10 @@ pub mod pallet {
 						"Invalid signature for RefreshProposal
 						**********************************************************
 						signature: {:?}
+						nonce: {}
 						**********************************************************",
 						hex::encode(signature.clone()),
+						u32::from(nonce),
 					);
 					Error::<T>::InvalidSignature
 				})?;
@@ -892,6 +899,7 @@ pub mod pallet {
 				uncompressed_pub_key,
 				compressed_pub_key: next_pub_key,
 				pub_key_sig: signature,
+				nonce: u32::from(nonce),
 			});
 
 			// now increment the block number at which we expect next unsigned transaction.
@@ -1342,9 +1350,8 @@ impl<T: Config> Pallet<T> {
 
 	pub fn do_refresh(pub_key: (u64, Vec<u8>)) {
 		let uncompressed_pub_key = Self::decompress_public_key(pub_key.1).unwrap_or_default();
-		// the nonce will be auto incremented once we rotate the keys successfully.
 		let data = dkg_runtime_primitives::RefreshProposal {
-			nonce: Self::refresh_nonce().into(),
+			nonce: Self::refresh_nonce().saturating_add(1).into(),
 			pub_key: uncompressed_pub_key,
 		};
 		match T::ProposalHandler::handle_unsigned_refresh_proposal(data) {
@@ -1530,7 +1537,8 @@ impl<T: Config> Pallet<T> {
 				val.push(pub_key_signature.clone());
 			});
 			// and increment the nonce
-			let next_nonce = Self::refresh_nonce().saturating_add(1);
+			let current_nonce = Self::refresh_nonce();
+			let next_nonce = current_nonce.saturating_add(1);
 			RefreshNonce::<T>::put(next_nonce);
 			let uncompressed_pub_key =
 				Self::decompress_public_key(next_pub_key.1.clone()).unwrap_or_default();
@@ -1545,6 +1553,7 @@ impl<T: Config> Pallet<T> {
 				uncompressed_pub_key,
 				compressed_pub_key,
 				pub_key_sig: next_pub_key_signature,
+				nonce: next_nonce,
 			});
 		}
 	}

pallets/dkg-metadata/src/tests.rs

@@ -14,13 +14,18 @@
 
 use std::vec;
 
-use frame_support::{assert_ok, traits::OnInitialize};
+use frame_support::{assert_ok, traits::Hooks, weights::Weight};
+use sp_core::ByteArray;
+use sp_runtime::traits::Bounded;
 
 use crate::mock::*;
 
 fn init_block(block: u64) {
 	System::set_block_number(block);
 	Session::on_initialize(block);
+	DKGMetadata::on_initialize(block);
+	DKGMetadata::on_idle(block, Weight::max_value());
+	DKGMetadata::on_finalize(block);
 }
 
 #[test]
@@ -119,3 +124,24 @@ fn trigger_emergency_keygen_works() {
 		assert!(!DKGMetadata::should_execute_emergency_keygen());
 	});
 }
+
+#[test]
+fn refresh_nonce_should_increment_by_one() {
+	new_test_ext(vec![1, 2, 3, 4]).execute_with(|| {
+		init_block(1);
+		let refresh_nonce = DKGMetadata::refresh_nonce();
+		assert_eq!(refresh_nonce, 0);
+
+		crate::pallet::NextDKGPublicKey::<Test>::put((1, mock_dkg_id(1).to_raw_vec()));
+		crate::pallet::NextPublicKeySignature::<Test>::put(vec![1u8; 64]);
+		init_block(2);
+		let refresh_nonce = DKGMetadata::refresh_nonce();
+		assert_eq!(refresh_nonce, 1);
+
+		crate::pallet::NextDKGPublicKey::<Test>::put((2, mock_dkg_id(2).to_raw_vec()));
+		crate::pallet::NextPublicKeySignature::<Test>::put(vec![2u8; 64]);
+		init_block(3);
+		let refresh_nonce = DKGMetadata::refresh_nonce();
+		assert_eq!(refresh_nonce, 2);
+	});
+}