diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 50f136d4e..ae7b3349f 100644 --- a/bin/node/runtime/src/lib.rs +++ b/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 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 for Runtime { type WithdrawalRateLimit = LLMRateLimit; type MaxTotalLocked = LLMMaxTotalLocked; type MinimumTransfer = LLMMinimumTransfer; + type MinimumFee = BridgeMinimumFee; + type MaximumFee = BridgeMaximumFee; type WeightInfo = (); } diff --git a/eth-bridge/contracts/script/Deploy.s.sol b/eth-bridge/contracts/script/Deploy.s.sol index 0749019be..20553bfbe 100644 --- a/eth-bridge/contracts/script/Deploy.s.sol +++ b/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 ) ) ); diff --git a/eth-bridge/contracts/script/UpgradeBridgesToV2.s.sol b/eth-bridge/contracts/script/UpgradeBridgesToV2.s.sol index 78a58fbff..b577a30c5 100644 --- a/eth-bridge/contracts/script/UpgradeBridgesToV2.s.sol +++ b/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 + ) ) ); } diff --git a/eth-bridge/contracts/src/Bridge.sol b/eth-bridge/contracts/src/Bridge.sol index 7a6df45cf..08efb683c 100644 --- a/eth-bridge/contracts/src/Bridge.sol +++ b/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_; } diff --git a/eth-bridge/contracts/test/Bridge.t.sol b/eth-bridge/contracts/test/Bridge.t.sol index ed1471280..24eed7cc8 100644 --- a/eth-bridge/contracts/test/Bridge.t.sol +++ b/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( diff --git a/eth-bridge/contracts/test/VoteFeeCompare.t.sol b/eth-bridge/contracts/test/VoteFeeCompare.t.sol index cb2668df8..2e352376e 100644 --- a/eth-bridge/contracts/test/VoteFeeCompare.t.sol +++ b/eth-bridge/contracts/test/VoteFeeCompare.t.sol @@ -53,7 +53,9 @@ contract VoteFeeCompare is Test, BridgeEvents { 10, 650, 0, - 650 + 650, + 2, + 100 ) ) ) diff --git a/frame/federated-bridge/README.md b/frame/federated-bridge/README.md index 31420229d..12be4658f 100644 --- a/frame/federated-bridge/README.md +++ b/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 diff --git a/frame/federated-bridge/src/benchmarking.rs b/frame/federated-bridge/src/benchmarking.rs index 0c73898b6..05684d53c 100644 --- a/frame/federated-bridge/src/benchmarking.rs +++ b/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, I: 'static>() { Bridge::::set_state(RawOrigin::Signed(admin).into(), BridgeState::Active).unwrap(); } -fn receipt, I: 'static>(i: u8) -> (ReceiptId, IncomingReceipt>) { +fn receipt, I: 'static>( + i: u8, +) -> (ReceiptId, IncomingReceipt>) { ( [i; 32], IncomingReceipt { @@ -84,17 +84,18 @@ fn add_votes, I: 'static>(r: u32) -> Result<(), &'static str> { let (receipt_id, receipt_data) = receipt::(0); for i in 1..=r { let relay: T::AccountId = account("relay", i, SEED); - Bridge::::vote_withdraw(RawOrigin::Signed(relay).into(), + Bridge::::vote_withdraw( + RawOrigin::Signed(relay).into(), receipt_id.clone(), receipt_data.clone(), - ).unwrap(); + ) + .unwrap(); } assert_eq!(Voting::::get(receipt_id).len(), r as usize); Ok(()) } - benchmarks_instance_pallet! { deposit { activate::(); @@ -186,7 +187,7 @@ benchmarks_instance_pallet! { let r in 1 .. T::MaxRelays::get() - 1 => add_relays::(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::::get().contains(&relay)); @@ -196,7 +197,7 @@ benchmarks_instance_pallet! { let r in 1 .. T::MaxWatchers::get() => add_watchers::(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::::get().contains(&watcher)); }: _(origin, watcher.clone()) verify { @@ -207,7 +208,7 @@ benchmarks_instance_pallet! { let r in 1 .. T::MaxRelays::get() => add_relays::(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::::get().contains(&relay)); }: _(origin, relay.clone()) verify { @@ -218,7 +219,7 @@ benchmarks_instance_pallet! { let r in 1 .. T::MaxWatchers::get() - 1 => add_watchers::(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::::get().contains(&watcher)); @@ -236,7 +237,7 @@ benchmarks_instance_pallet! { emergency_stop { let r in 1 .. T::MaxWatchers::get() => add_watchers::(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::::get(), BridgeState::Stopped); diff --git a/frame/federated-bridge/src/lib.rs b/frame/federated-bridge/src/lib.rs index 8ec721740..d56b22f7a 100644 --- a/frame/federated-bridge/src/lib.rs +++ b/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>; + #[pallet::constant] + /// Minimum fee that admin can set. + type MinimumFee: Get>; + + #[pallet::constant] + /// Maximum fee that admin can set. + type MaximumFee: Get>; + /// Origin that's authorized to set Admin and SuperAdmin type ForceOrigin: EnsureOrigin; @@ -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::::BridgeStopped); ensure!(relays.contains(&relay), Error::::Unauthorized); ensure!( - status == IncomingReceiptStatus::Voting || - matches!(status, IncomingReceiptStatus::Approved(_)), + status == IncomingReceiptStatus::Voting + || matches!(status, IncomingReceiptStatus::Approved(_)), Error::::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, amount: BalanceOf) -> DispatchResult { Self::ensure_admin(origin)?; + + ensure!(amount >= T::MinimumFee::get(), Error::::InvalidValue); + ensure!(amount <= T::MaximumFee::get(), Error::::InvalidValue); + Fee::::set(amount); Ok(()) } @@ -828,8 +849,8 @@ pub mod pallet { fn ensure_admin(origin: OriginFor) -> DispatchResult { let caller = ensure_signed(origin)?; ensure!( - Some(caller.clone()) == Admin::::get() || - Some(caller) == SuperAdmin::::get(), + Some(caller.clone()) == Admin::::get() + || Some(caller) == SuperAdmin::::get(), Error::::Unauthorized ); Ok(()) diff --git a/frame/federated-bridge/src/mock.rs b/frame/federated-bridge/src/mock.rs index efc17412e..e9171ef94 100644 --- a/frame/federated-bridge/src/mock.rs +++ b/frame/federated-bridge/src/mock.rs @@ -95,6 +95,8 @@ impl pallet_federated_bridge::Config for Test { type ForceOrigin = EnsureRoot; type MaxTotalLocked = ConstU64<10000>; type MinimumTransfer = ConstU64<2>; + type MinimumFee = ConstU64<10>; + type MaximumFee = ConstU64<100>; type WeightInfo = (); } diff --git a/frame/federated-bridge/src/tests.rs b/frame/federated-bridge/src/tests.rs index b0e57c3c8..cb6049205 100644 --- a/frame/federated-bridge/src/tests.rs +++ b/frame/federated-bridge/src/tests.rs @@ -6,7 +6,10 @@ use crate::{ VotesRequired, Voting, Watchers, }; use frame_support::{assert_noop, assert_ok}; -use sp_runtime::{TokenError, traits::{AccountIdConversion, BadOrigin}}; +use sp_runtime::{ + traits::{AccountIdConversion, BadOrigin}, + TokenError, +}; fn eth_recipient(n: u8) -> EthAddress { let mut addr: EthAddress = Default::default(); @@ -42,7 +45,8 @@ fn deposit_emits_receipt() { new_test_ext().execute_with(|| { assert_ok!(Bridge::deposit(RuntimeOrigin::signed(0), 2, eth_recipient(0))); System::assert_last_event( - Event::::OutgoingReceipt { from: 0, amount: 2, eth_recipient: eth_recipient(0) }.into(), + Event::::OutgoingReceipt { from: 0, amount: 2, eth_recipient: eth_recipient(0) } + .into(), ); }); } @@ -515,6 +519,14 @@ fn set_fee_works() { }); } +#[test] +fn set_fee_respects_min_max() { + new_test_ext().execute_with(|| { + assert_noop!(Bridge::set_fee(RuntimeOrigin::signed(100), 9), Error::::InvalidValue); + assert_noop!(Bridge::set_fee(RuntimeOrigin::signed(100), 101), Error::::InvalidValue); + }); +} + #[test] fn remove_relay_checks_origin() { new_test_ext().execute_with(|| {