token swap type (replay protection)

Author: svyatonik

Cargo.lock

@@ -54,7 +54,7 @@ use bp_messages::{
 	DeliveredMessages, LaneId, MessageNonce,
 };
 use bp_runtime::{messages::DispatchFeePayment, InstanceId};
-use bp_token_swap::TokenSwap;
+use bp_token_swap::{TokenSwap, TokenSwapType};
 use codec::{Decode, Encode};
 use frame_support::{
 	fail,
@@ -84,6 +84,8 @@ pub enum TokenSwapState {
 
 pub use pallet::*;
 
+// comes from #[pallet::event]
+#[allow(clippy::unused_unit)]
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
@@ -144,6 +146,7 @@ pub mod pallet {
 	>;
 	/// Type of `TokenSwap` used by the pallet.
 	pub type TokenSwapOf<T, I> = TokenSwap<
+		BlockNumberFor<T>,
 		<<T as Config<I>>::ThisCurrency as Currency<<T as frame_system::Config>::AccountId>>::Balance,
 		<T as frame_system::Config>::AccountId,
 		<T as Config<I>>::BridgedBalance,
@@ -328,7 +331,16 @@ pub mod pallet {
 			let swap_state = PendingSwaps::<T, I>::get(swap_hash);
 			match swap_state {
 				Some(TokenSwapState::Started) => fail!(Error::<T, I>::SwapIsPending),
-				Some(TokenSwapState::Confirmed) => (),
+				Some(TokenSwapState::Confirmed) => {
+					let is_claim_allowed = match swap.swap_type {
+						TokenSwapType::TemporaryTargetAccountAtBridgedChain => true,
+						TokenSwapType::LockClaimUntilBlock(block_number, _) => {
+							block_number < frame_system::Pallet::<T>::block_number()
+						}
+					};
+
+					ensure!(is_claim_allowed, Error::<T, I>::SwapIsTemporaryLocked);
+				}
 				Some(TokenSwapState::Failed) => fail!(Error::<T, I>::SwapIsFailed),
 				None => fail!(Error::<T, I>::SwapIsInactive),
 			}
@@ -338,7 +350,7 @@ pub mod pallet {
 
 		/// Return previously reserved `source_balance_at_this_chain` back to the `source_account_at_this_chain`.
 		///
-		/// This should be called only when transferhas failed at Bridged chain and we have received
+		/// This should be called only when transfer has failed at Bridged chain and we have received
 		/// notification about thate.
 		#[pallet::weight(0)]
 		pub fn cancel_swap(origin: OriginFor<T>, swap: TokenSwapOf<T, I>) -> DispatchResultWithPostInfo {
@@ -349,13 +361,16 @@ pub mod pallet {
 				Error::<T, I>::MismatchedSwapSourceOrigin,
 			);
 
-			// ensure that the swap is failed
+			// ensure that the swap has failed
 			let swap_hash = swap.using_encoded(blake2_256).into();
 			let swap_state = PendingSwaps::<T, I>::get(swap_hash);
 			match swap_state {
 				Some(TokenSwapState::Started) => fail!(Error::<T, I>::SwapIsPending),
 				Some(TokenSwapState::Confirmed) => fail!(Error::<T, I>::SwapIsConfirmed),
-				Some(TokenSwapState::Failed) => (),
+				Some(TokenSwapState::Failed) => {
+					// we allow cancelling swap even before lock period is over - the `source_account_at_this_chain`
+					// has already paid for nothing and it is up to him to decide whether he want to try again
+				}
 				None => fail!(Error::<T, I>::SwapIsInactive),
 			}
 
@@ -394,6 +409,11 @@ pub mod pallet {
 		SwapIsPending,
 		/// Someone is trying to claim swap that has failed.
 		SwapIsFailed,
+		/// Claiming swap is not allowed.
+		///
+		/// Now the only possible case when you may get this error, is when you're trying to claim swap with
+		/// `TokenSwapType::LockClaimUntilBlock` before lock period is over.
+		SwapIsTemporaryLocked,
 		/// Someone is trying to cancel swap that has been confirmed.
 		SwapIsConfirmed,
 		/// Someone is trying to claim/cancel swap that is either not started or already claimed/cancelled.
@@ -496,12 +516,15 @@ mod tests {
 	use crate::mock::*;
 	use frame_support::{assert_noop, assert_ok};
 
+	const CAN_CLAIM_BLOCK_NUMBER: u64 = 11;
+
 	const BRIDGED_CHAIN_ACCOUNT_PUBLIC: BridgedAccountPublic = 1;
 	const BRIDGED_CHAIN_ACCOUNT_SIGNATURE: BridgedAccountSignature = 2;
 	const BRIDGED_CHAIN_ACCOUNT: BridgedAccountId = 3;
 
 	fn test_swap() -> TokenSwapOf<TestRuntime, ()> {
 		bp_token_swap::TokenSwap {
+			swap_type: TokenSwapType::LockClaimUntilBlock(CAN_CLAIM_BLOCK_NUMBER - 1, 0.into()),
 			source_balance_at_this_chain: 100,
 			source_account_at_this_chain: THIS_CHAIN_ACCOUNT,
 			target_balance_at_bridged_chain: 200,
@@ -723,6 +746,7 @@ mod tests {
 	#[test]
 	fn claim_swap_fails_if_currency_transfer_from_swap_account_fails() {
 		run_test(|| {
+			frame_system::Pallet::<TestRuntime>::set_block_number(CAN_CLAIM_BLOCK_NUMBER);
 			PendingSwaps::<TestRuntime, ()>::insert(test_swap_hash(), TokenSwapState::Confirmed);
 
 			assert_noop!(
@@ -735,13 +759,31 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn claim_swap_fails_before_lock_period_is_completed() {
+		run_test(|| {
+			start_test_swap();
+			receive_test_swap_confirmation(true);
+
+			frame_system::Pallet::<TestRuntime>::set_block_number(CAN_CLAIM_BLOCK_NUMBER - 1);
+
+			assert_noop!(
+				Pallet::<TestRuntime>::claim_swap(
+					Origin::signed(target_account_at_this_chain::<TestRuntime, ()>(&test_swap())),
+					test_swap(),
+				),
+				Error::<TestRuntime, ()>::SwapIsTemporaryLocked
+			);
+		});
+	}
+
 	#[test]
 	fn claim_swap_succeeds() {
 		run_test(|| {
 			start_test_swap();
 			receive_test_swap_confirmation(true);
 
-			frame_system::Pallet::<TestRuntime>::set_block_number(1);
+			frame_system::Pallet::<TestRuntime>::set_block_number(CAN_CLAIM_BLOCK_NUMBER);
 			frame_system::Pallet::<TestRuntime>::reset_events();
 
 			assert_ok!(Pallet::<TestRuntime>::claim_swap(

modules/token-swap/src/lib.rs

@@ -12,10 +12,12 @@ codec = { package = "parity-scale-codec", version = "2.0.0", default-features =
 # Substrate Dependencies
 
 frame-support = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false }
+sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false }
 
 [features]
 default = ["std"]
 std = [
 	"codec/std",
 	"frame-support/std",
+	"sp-core/std",
 ]

primitives/token-swap/Cargo.toml

@@ -16,15 +16,43 @@
 
 use codec::{Decode, Encode};
 use frame_support::RuntimeDebug;
+use sp_core::U256;
+
+/// Token swap type.
+///
+/// Different swap types give a different guarantees regarding possible swap
+/// replay protection.
+#[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, Eq)]
+pub enum TokenSwapType<ThisBlockNumber> {
+	/// The `target_account_at_bridged_chain` is temporary and only have funds for single swap.
+	///
+	/// ***WARNING**: if `target_account_at_bridged_chain` still exists after the swap has been
+	/// completed (either by claiming or cancelling), the `source_account_at_this_chain` will be able
+	/// to restart the swap again and repeat the swap until `target_account_at_bridged_chain` depletes.
+	TemporaryTargetAccountAtBridgedChain,
+	/// This swap type prevents `source_account_at_this_chain` from restarting the swap after it has
+	/// been completed. There are two consequences:
+	///
+	/// 1) the `source_account_at_this_chain` won't be able to call `start_swap` after given <ThisBlockNumber>;
+	/// 2) the `target_account_at_bridged_chain` won't be able to call `claim_swap` (over the bridge) before
+	///    block `<ThisBlockNumber + 1>`.
+	///
+	/// The second element is the nonce of the swap. You must care about its uniqueness if you're
+	/// planning to perform another swap with exactly the same parameters (i.e. same amount, same accounts,
+	/// same `ThisBlockNumber`) to avoid collisions.
+	LockClaimUntilBlock(ThisBlockNumber, U256),
+}
 
 /// An intention to swap `source_balance_at_this_chain` owned by `source_account_at_this_chain`
 /// to `target_balance_at_bridged_chain` owned by `target_account_at_bridged_chain`.
 ///
 /// **IMPORTANT NOTE**: this structure is always the same during single token swap. So even
-/// when chain changes, the meaning of This and Bridged chains is still the same. This chain
-/// is always the chain where swap has been started. And the Bridged chain is the other chain.
+/// when chain changes, the meaning of This and Bridged are still used to point to the same chains.
+/// This chain is always the chain where swap has been started. And the Bridged chain is the other chain.
 #[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, Eq)]
-pub struct TokenSwap<ThisBalance, ThisAccountId, BridgedBalance, BridgedAccountId> {
+pub struct TokenSwap<ThisBlockNumber, ThisBalance, ThisAccountId, BridgedBalance, BridgedAccountId> {
+	/// The type of the swap.
+	pub swap_type: TokenSwapType<ThisBlockNumber>,
 	/// This chain balance to be swapped with `target_balance_at_bridged_chain`.
 	pub source_balance_at_this_chain: ThisBalance,
 	/// Account id of the party acting at This chain and owning the `source_account_at_this_chain`.