BLOCKCHAIN-235 - min & max limits for set fee

bin/node/runtime/src/lib.rs

@@ -1575,6 +1575,8 @@ parameter_types! {
 	);
 	pub const LLMMaxTotalLocked: Balance = 100_000 * GRAINS_IN_LLM;
 	pub const LLMMinimumTransfer: Balance = 10 * GRAINS_IN_LLM;
+	pub const BridgeMinimumFee: Balance = 1 * DOLLARS;
+	pub const BridgeMaximumFee: Balance = 10 * DOLLARS;
 }
 
 type EthLLDBridgeInstance = pallet_federated_bridge::Instance1;
@@ -1590,6 +1592,8 @@ impl pallet_federated_bridge::Config<EthLLDBridgeInstance> for Runtime {
 	type WithdrawalRateLimit = LLDRateLimit;
 	type MaxTotalLocked = LLDMaxTotalLocked;
 	type MinimumTransfer = LLDMinimumTransfer;
+	type MinimumFee = BridgeMinimumFee;
+	type MaximumFee = BridgeMaximumFee;
 	type WeightInfo = ();
 }
 
@@ -1606,6 +1610,8 @@ impl pallet_federated_bridge::Config<EthLLMBridgeInstance> for Runtime {
 	type WithdrawalRateLimit = LLMRateLimit;
 	type MaxTotalLocked = LLMMaxTotalLocked;
 	type MinimumTransfer = LLMMinimumTransfer;
+	type MinimumFee = BridgeMinimumFee;
+	type MaximumFee = BridgeMaximumFee;
 	type WeightInfo = ();
 }
 

eth-bridge/contracts/script/Deploy.s.sol

@@ -42,7 +42,9 @@ contract Deploy is Script {
                     60_000_000_000_000, // rate limit counter decay
                     300_000_000_000_000_000, // supply limit
                     30_000_000_000_000, // min transfer
-                    3_000_000_000_000_000_000 // max supply limit
+                    3_000_000_000_000_000_000, // max supply limit
+                    1_000_000 gwei, // min fee
+                    100_000_000 gwei // max fee
                 )
             )
         );
@@ -70,7 +72,9 @@ contract Deploy is Script {
                         20_000_000_000_000, // rate limit counter decay
                     100_000_000_000_000_000, // supply limit
                     10_000_000_000_000, // min transfer
-                    1_000_000_000_000_000_000 // max supply limit
+                    1_000_000_000_000_000_000, // max supply limit
+                    1_000_000 gwei, // min fee
+                    100_000_000 gwei // max fee
                 )
             )
         );

eth-bridge/contracts/script/UpgradeBridgesToV2.s.sol

@@ -16,14 +16,22 @@ contract UpgradeBridgesToV2 is Script {
             address(newBridgeImpl),
             abi.encodeCall(
                 Bridge.initializeV2,
-                (3_000_000_000_000_000_000) // max supply limit of 3M LLD, admin can lower it
+                (
+                    3_000_000_000_000_000_000, // max supply limit of 3M LLD, admin can lower it
+                    1_000_000 gwei, // min fee
+                    100_000_000 gwei // max fee
+                )
             )
         );
         llmProxy.upgradeToAndCall(
             address(newBridgeImpl),
             abi.encodeCall(
                 Bridge.initializeV2,
-                (1_000_000_000_000_000_000) // max supply limit of 1M LLM, admin can lower it
+                (
+                    1_000_000_000_000_000_000, // max supply limit of 3M LLD, admin can lower it
+                    1_000_000 gwei, // min fee
+                    100_000_000 gwei // max fee
+                )
             )
         );
     }

eth-bridge/contracts/src/Bridge.sol

@@ -94,8 +94,12 @@ contract Bridge is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Bri
     RateLimitParameters public rateLimit; // 2x uint256
     /// Minimum transfer - only applied for burns
     uint256 public minTransfer;
-    /// Initial supply limit - admin can't increase supply limit above this value
+    /// Maximum supply limit - admin can't increase supply limit above this value
     uint256 public maxSupplyLimit;
+    /// Minimum fee that admin can set
+    uint256 public minFee;
+    /// Maximum fee that admin can set
+    uint256 public maxFee;
 
     constructor() {
         _disableInitializers();
@@ -111,6 +115,8 @@ contract Bridge is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Bri
     /// @param supplyLimit_ initial `supplyLimit`
     /// @param minTransfer_ initial `minTransfer`
     /// @param maxSupplyLimit_ maximum `supplyLimit` that can be set by admin
+    /// @param minFee_ minimum `fee` that can be set by admin
+    /// @param maxFee_ maximum `fee` that can be set by admin
     function initialize(
         WrappedToken token_,
         uint32 votesRequired_,
@@ -120,10 +126,12 @@ contract Bridge is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Bri
         uint256 decayRate,
         uint256 supplyLimit_,
         uint256 minTransfer_,
-        uint256 maxSupplyLimit_
+        uint256 maxSupplyLimit_,
+        uint256 minFee_,
+        uint256 maxFee_
     ) external {
         _initializeV1(token_, votesRequired_, mintDelay_, fee_, counterLimit, decayRate, supplyLimit_, minTransfer_);
-        initializeV2(maxSupplyLimit_);
+        initializeV2(maxSupplyLimit_, minFee_, maxFee_);
     }
 
     function _initializeV1(
@@ -154,8 +162,10 @@ contract Bridge is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Bri
 
     /// Reinitializer from v1 to v2. Should be used in the same tx as upgrade
     /// @param maxSupplyLimit_ maximum `supplyLimit` that can be set by admin
-    function initializeV2(uint256 maxSupplyLimit_) public reinitializer(2) {
+    function initializeV2(uint256 maxSupplyLimit_, uint256 minFee_, uint256 maxFee_) public reinitializer(2) {
         maxSupplyLimit = maxSupplyLimit_;
+        minFee = minFee_;
+        maxFee = maxFee_;
     }
 
     /// Adding special users. See role docs on info who can grant each role
@@ -319,7 +329,11 @@ contract Bridge is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Bri
     /// Set the minting fee
     /// @param fee_ New minting fee
     /// @dev Only addresses with ADMIN_ROLE can call this
+    /// @dev Will revert with InvalidArgument if outside [minFee,maxFee] range
     function setFee(uint256 fee_) public onlyRole(ADMIN_ROLE) {
+        if (fee_ > maxFee || fee_ < minFee) {
+            revert InvalidArgument();
+        }
         fee = fee_;
     }
 

eth-bridge/contracts/test/Bridge.t.sol

@@ -42,7 +42,7 @@ contract BridgeTest is Test, BridgeEvents {
                     address(bridgeImpl),
                     abi.encodeCall(
                         Bridge.initialize,
-                        (token, 2, 10, 4, 1000, 10, 1_000_000, 0, 1_000_000)
+                        (token, 2, 10, 4, 1000, 10, 1_000_000, 0, 1_000_000, 2, 100)
                     )
                 )
             )
@@ -405,12 +405,20 @@ contract BridgeTest is Test, BridgeEvents {
 
     function testSetFeeWorks() public {
         vm.startPrank(dave);
-        bridge.setFee(1);
-        assertEq(bridge.fee(), 1);
+        bridge.setFee(2);
+        assertEq(bridge.fee(), 2);
         bridge.setFee(10);
         assertEq(bridge.fee(), 10);
     }
 
+    function testSetFeeRespectsMinMax() public {
+        vm.startPrank(dave);
+        vm.expectRevert(InvalidArgument.selector);
+        bridge.setFee(1);
+        vm.expectRevert(InvalidArgument.selector);
+        bridge.setFee(101);
+    }
+
     function testSetFeeRequiresAdmin() public {
         vm.prank(alice);
         vm.expectRevert(

eth-bridge/contracts/test/VoteFeeCompare.t.sol

@@ -53,7 +53,9 @@ contract VoteFeeCompare is Test, BridgeEvents {
                             10,
                             650,
                             0,
-                            650
+                            650,
+                            2,
+                            100
                         )
                     )
                 )

frame/federated-bridge/README.md

@@ -97,6 +97,8 @@ Following security features are implemented substrate-side:
 * `WithdrawalDelay` - number of blocks between transfer approval and actually allowing it
 * `WithdrawalRateLimit` - rate limit parameters
 * `MinimumTransfer` - minimum amount that can be deposited in single call
+* `MinimumFee` - minimum fee that can be set by admin
+* `MaximumFee` - maximum fee that can be set by admin
 * `ForceOrigin` - origin that's authorized to set admin and super admin
 
 

frame/federated-bridge/src/benchmarking.rs

@@ -11,19 +11,17 @@ THE SOFTWARE IS PROVIDED “AS IS”, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR I
 
 use super::*;
 use crate::{
-	Pallet as Bridge,
-	StatusOf,
-	IncomingReceiptStatus,
-	Fee,
+	Admin, BridgeState, Fee, IncomingReceiptStatus, Pallet as Bridge, StatusOf, SuperAdmin,
 	VotesRequired,
-	BridgeState,
-	Admin, SuperAdmin,
 };
-use sp_runtime::traits::Bounded;
 use frame_benchmarking::{account, benchmarks_instance_pallet, impl_benchmark_test_suite};
+use frame_support::traits::{
+	fungible::{Inspect, Mutate},
+	Currency,
+};
 use frame_system::RawOrigin;
+use sp_runtime::traits::Bounded;
 use sp_std::prelude::*;
-use frame_support::traits::{Currency, fungible::{Mutate, Inspect}};
 
 const SEED: u32 = 0;
 
@@ -38,7 +36,9 @@ fn activate<T: Config<I>, I: 'static>() {
 	Bridge::<T, I>::set_state(RawOrigin::Signed(admin).into(), BridgeState::Active).unwrap();
 }
 
-fn receipt<T: Config<I>, I: 'static>(i: u8) -> (ReceiptId, IncomingReceipt<T::AccountId, BalanceOfToken<T, I>>) {
+fn receipt<T: Config<I>, I: 'static>(
+	i: u8,
+) -> (ReceiptId, IncomingReceipt<T::AccountId, BalanceOfToken<T, I>>) {
 	(
 		[i; 32],
 		IncomingReceipt {
@@ -84,17 +84,18 @@ fn add_votes<T: Config<I>, I: 'static>(r: u32) -> Result<(), &'static str> {
 	let (receipt_id, receipt_data) = receipt::<T, I>(0);
 	for i in 1..=r {
 		let relay: T::AccountId = account("relay", i, SEED);
-		Bridge::<T, I>::vote_withdraw(RawOrigin::Signed(relay).into(),
+		Bridge::<T, I>::vote_withdraw(
+			RawOrigin::Signed(relay).into(),
 			receipt_id.clone(),
 			receipt_data.clone(),
-		).unwrap();
+		)
+		.unwrap();
 	}
 
 	assert_eq!(Voting::<T, I>::get(receipt_id).len(), r as usize);
 	Ok(())
 }
 
-
 benchmarks_instance_pallet! {
 	deposit {
 		activate::<T, I>();
@@ -186,7 +187,7 @@ benchmarks_instance_pallet! {
 		let r in 1 .. T::MaxRelays::get() - 1 => add_relays::<T, I>(r)?;
 		let admin: T::AccountId = account("admin", 0, SEED);
 		let relay: T::AccountId = account("relay", r+1, SEED);
-        let origin = RawOrigin::Signed(admin.clone());
+		let origin = RawOrigin::Signed(admin.clone());
 	}: _(origin, relay.clone())
 	verify {
 		assert!(Relays::<T, I>::get().contains(&relay));
@@ -196,7 +197,7 @@ benchmarks_instance_pallet! {
 		let r in 1 .. T::MaxWatchers::get() => add_watchers::<T, I>(r)?;
 		let admin: T::AccountId = account("admin", 0, SEED);
 		let watcher: T::AccountId = account("watcher", 1, SEED);
-        let origin = RawOrigin::Signed(admin.clone());
+		let origin = RawOrigin::Signed(admin.clone());
 		assert!(Watchers::<T, I>::get().contains(&watcher));
 	}: _(origin, watcher.clone())
 	verify {
@@ -207,7 +208,7 @@ benchmarks_instance_pallet! {
 		let r in 1 .. T::MaxRelays::get() => add_relays::<T, I>(r)?;
 		let admin: T::AccountId = account("admin", 0, SEED);
 		let relay: T::AccountId = account("relay", 1, SEED);
-        let origin = RawOrigin::Signed(admin.clone());
+		let origin = RawOrigin::Signed(admin.clone());
 		assert!(Relays::<T, I>::get().contains(&relay));
 	}: _(origin, relay.clone())
 	verify {
@@ -218,7 +219,7 @@ benchmarks_instance_pallet! {
 		let r in 1 .. T::MaxWatchers::get() - 1 => add_watchers::<T, I>(r)?;
 		let admin: T::AccountId = account("admin", 0, SEED);
 		let watcher: T::AccountId = account("watcher", r+1, SEED);
-        let origin = RawOrigin::Signed(admin.clone());
+		let origin = RawOrigin::Signed(admin.clone());
 	}: _(origin, watcher.clone())
 	verify {
 		assert!(Watchers::<T, I>::get().contains(&watcher));
@@ -236,7 +237,7 @@ benchmarks_instance_pallet! {
 	emergency_stop {
 		let r in 1 .. T::MaxWatchers::get() => add_watchers::<T, I>(r)?;
 		let watcher: T::AccountId = account("watcher", 1, SEED);
-        let origin = RawOrigin::Signed(watcher.clone());
+		let origin = RawOrigin::Signed(watcher.clone());
 	}: _(origin)
 	verify {
 		assert_eq!(State::<T, I>::get(), BridgeState::Stopped);

frame/federated-bridge/src/lib.rs

@@ -97,6 +97,8 @@
 //! * `WithdrawalDelay` - number of blocks between transfer approval and actually allowing it
 //! * `WithdrawalRateLimit` - rate limit parameters
 //! * `MinimumTransfer` - minimum amount that can be deposited in single call
+//! * `MinimumFee` - minimum fee that can be set by admin
+//! * `MaximumFee` - maximum fee that can be set by admin
 //! * `ForceOrigin` - origin that's authorized to set admin and super admin
 //!
 //!
@@ -137,9 +139,9 @@ pub use pallet::*;
 #[cfg(feature = "std")]
 use serde::{Deserialize, Serialize};
 
+mod benchmarking;
 mod mock;
 mod tests;
-mod benchmarking;
 pub mod weights;
 pub use weights::WeightInfo;
 
@@ -197,7 +199,10 @@ pub mod pallet {
 	use frame_support::{
 		sp_runtime::{traits::AccountIdConversion, Saturating},
 		traits::{
-			tokens::{Preservation, fungible::{Inspect, Mutate}},
+			tokens::{
+				fungible::{Inspect, Mutate},
+				Preservation,
+			},
 			ExistenceRequirement,
 		},
 	};
@@ -261,6 +266,14 @@ pub mod pallet {
 		/// enforced on deposits.
 		type MinimumTransfer: Get<BalanceOfToken<Self, I>>;
 
+		#[pallet::constant]
+		/// Minimum fee that admin can set.
+		type MinimumFee: Get<BalanceOf<Self, I>>;
+
+		#[pallet::constant]
+		/// Maximum fee that admin can set.
+		type MaximumFee: Get<BalanceOf<Self, I>>;
+
 		/// Origin that's authorized to set Admin and SuperAdmin
 		type ForceOrigin: EnsureOrigin<Self::RuntimeOrigin>;
 
@@ -300,6 +313,8 @@ pub mod pallet {
 		TooMuchLocked,
 		/// Amount smaller than MinimumTransfer
 		TooSmallAmount,
+		/// Invalid value
+		InvalidValue,
 	}
 
 	#[pallet::event]
@@ -517,8 +532,8 @@ pub mod pallet {
 			ensure!(state == BridgeState::Active, Error::<T, I>::BridgeStopped);
 			ensure!(relays.contains(&relay), Error::<T, I>::Unauthorized);
 			ensure!(
-				status == IncomingReceiptStatus::Voting ||
-					matches!(status, IncomingReceiptStatus::Approved(_)),
+				status == IncomingReceiptStatus::Voting
+					|| matches!(status, IncomingReceiptStatus::Approved(_)),
 				Error::<T, I>::AlreadyProcessed
 			);
 
@@ -528,7 +543,7 @@ pub mod pallet {
 					// someone lied, stop the bridge
 					Self::do_set_state(BridgeState::Stopped);
 					// we return Ok, as we DONT want to revert stopping bridge
-					return Ok(())
+					return Ok(());
 				}
 			} else {
 				// first vote
@@ -622,11 +637,17 @@ pub mod pallet {
 		///
 		/// Can be called by Admin and SuperAdmin.
 		///
+		/// Reverts with InvalidValue if outside the [MinimumFee, MaximumFee] range.
+		///
 		/// Should be set high enough to cover running costs for all relays.
 		#[pallet::call_index(3)]
 		#[pallet::weight(T::WeightInfo::set_fee())]
 		pub fn set_fee(origin: OriginFor<T>, amount: BalanceOf<T, I>) -> DispatchResult {
 			Self::ensure_admin(origin)?;
+
+			ensure!(amount >= T::MinimumFee::get(), Error::<T, I>::InvalidValue);
+			ensure!(amount <= T::MaximumFee::get(), Error::<T, I>::InvalidValue);
+
 			Fee::<T, I>::set(amount);
 			Ok(())
 		}
@@ -828,8 +849,8 @@ pub mod pallet {
 		fn ensure_admin(origin: OriginFor<T>) -> DispatchResult {
 			let caller = ensure_signed(origin)?;
 			ensure!(
-				Some(caller.clone()) == Admin::<T, I>::get() ||
-					Some(caller) == SuperAdmin::<T, I>::get(),
+				Some(caller.clone()) == Admin::<T, I>::get()
+					|| Some(caller) == SuperAdmin::<T, I>::get(),
 				Error::<T, I>::Unauthorized
 			);
 			Ok(())