From de71fecc4e58d99474ff655789801e5edf3764b1 Mon Sep 17 00:00:00 2001
From: Xavier Lau <xavier@inv.cafe>
Date: Thu, 28 Sep 2023 06:08:05 -0500
Subject: [PATCH] Add `MaxTipAmount` for pallet-tips (#1709)

Last week we experienced a governance attack.
Surprisingly, there was no upper limit on the tip amount.

Due to the mechanism of pallet-fragment-election, the council members
will be refreshed immediately. Attacker is easy to control the council
and give a large tip amount.
---
 substrate/bin/node/runtime/src/lib.rs  |  1 +
 substrate/docs/Upgrading-2.0-to-3.0.md |  1 +
 substrate/frame/tips/src/lib.rs        | 14 +++++++++++++-
 substrate/frame/tips/src/tests.rs      | 19 +++++++++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs
index c90b9076de..2fcb20ad8d 100644
--- a/substrate/bin/node/runtime/src/lib.rs
+++ b/substrate/bin/node/runtime/src/lib.rs
@@ -1289,6 +1289,7 @@ impl pallet_tips::Config for Runtime {
 	type TipCountdown = TipCountdown;
 	type TipFindersFee = TipFindersFee;
 	type TipReportDepositBase = TipReportDepositBase;
+	type MaxTipAmount = ConstU128<{ 500 * DOLLARS }>;
 	type WeightInfo = pallet_tips::weights::SubstrateWeight<Runtime>;
 }
 
diff --git a/substrate/docs/Upgrading-2.0-to-3.0.md b/substrate/docs/Upgrading-2.0-to-3.0.md
index 58066ce074..3f2a3e7c5b 100644
--- a/substrate/docs/Upgrading-2.0-to-3.0.md
+++ b/substrate/docs/Upgrading-2.0-to-3.0.md
@@ -261,6 +261,7 @@ impl pallet_tips::Config for Runtime {
 	type TipCountdown = TipCountdown;
 	type TipFindersFee = TipFindersFee;
 	type TipReportDepositBase = TipReportDepositBase;
+	type MaxTipAmount = MaxTipAmount;
 	type WeightInfo = pallet_tips::weights::SubstrateWeight<Runtime>;
  }
 ```
diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs
index 6e8f72e054..8764486d5f 100644
--- a/substrate/frame/tips/src/lib.rs
+++ b/substrate/frame/tips/src/lib.rs
@@ -154,6 +154,10 @@ pub mod pallet {
 		#[pallet::constant]
 		type TipReportDepositBase: Get<BalanceOf<Self, I>>;
 
+		/// The maximum amount for a single tip.
+		#[pallet::constant]
+		type MaxTipAmount: Get<BalanceOf<Self, I>>;
+
 		/// Origin from which tippers must come.
 		///
 		/// `ContainsLengthBound::max_len` must be cost free (i.e. no storage read or heavy
@@ -208,6 +212,8 @@ pub mod pallet {
 		AlreadyKnown,
 		/// The tip hash is unknown.
 		UnknownTip,
+		/// The tip given was too generous.
+		MaxTipAmountExceeded,
 		/// The account attempting to retract the tip is not the finder of the tip.
 		NotFinder,
 		/// The tip cannot be claimed/closed because there are not enough tippers yet.
@@ -336,10 +342,13 @@ pub mod pallet {
 			let tipper = ensure_signed(origin)?;
 			let who = T::Lookup::lookup(who)?;
 			ensure!(T::Tippers::contains(&tipper), BadOrigin);
+
+			ensure!(T::MaxTipAmount::get() >= tip_value, Error::<T, I>::MaxTipAmountExceeded);
+
 			let reason_hash = T::Hashing::hash(&reason[..]);
 			ensure!(!Reasons::<T, I>::contains_key(&reason_hash), Error::<T, I>::AlreadyKnown);
-			let hash = T::Hashing::hash_of(&(&reason_hash, &who));
 
+			let hash = T::Hashing::hash_of(&(&reason_hash, &who));
 			Reasons::<T, I>::insert(&reason_hash, &reason);
 			Self::deposit_event(Event::NewTip { tip_hash: hash });
 			let tips = vec![(tipper.clone(), tip_value)];
@@ -387,7 +396,10 @@ pub mod pallet {
 			let tipper = ensure_signed(origin)?;
 			ensure!(T::Tippers::contains(&tipper), BadOrigin);
 
+			ensure!(T::MaxTipAmount::get() >= tip_value, Error::<T, I>::MaxTipAmountExceeded);
+
 			let mut tip = Tips::<T, I>::get(hash).ok_or(Error::<T, I>::UnknownTip)?;
+
 			if Self::insert_tip_and_check_closing(&mut tip, tipper, tip_value) {
 				Self::deposit_event(Event::TipClosing { tip_hash: hash });
 			}
diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs
index a700892d42..9cb90c3798 100644
--- a/substrate/frame/tips/src/tests.rs
+++ b/substrate/frame/tips/src/tests.rs
@@ -172,6 +172,7 @@ impl Config for Test {
 	type TipFindersFee = TipFindersFee;
 	type TipReportDepositBase = ConstU64<1>;
 	type DataDepositPerByte = ConstU64<1>;
+	type MaxTipAmount = ConstU64<10_000_000>;
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = ();
 }
@@ -183,6 +184,7 @@ impl Config<Instance1> for Test {
 	type TipFindersFee = TipFindersFee;
 	type TipReportDepositBase = ConstU64<1>;
 	type DataDepositPerByte = ConstU64<1>;
+	type MaxTipAmount = ConstU64<10_000_000>;
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = ();
 }
@@ -396,6 +398,23 @@ fn tip_median_calculation_works() {
 	});
 }
 
+#[test]
+fn tip_large_should_fail() {
+	new_test_ext().execute_with(|| {
+		Balances::make_free_balance_be(&Treasury::account_id(), 101);
+		assert_ok!(Tips::tip_new(RuntimeOrigin::signed(10), b"awesome.dot".to_vec(), 3, 0));
+		let h = tip_hash();
+		assert_noop!(
+			Tips::tip(
+				RuntimeOrigin::signed(12),
+				h,
+				<<Test as Config>::MaxTipAmount as Get<u64>>::get() + 1
+			),
+			Error::<Test>::MaxTipAmountExceeded
+		);
+	});
+}
+
 #[test]
 fn tip_changing_works() {
 	new_test_ext().execute_with(|| {