[Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider #2136

mattheworris · 2024-08-21T14:29:52Z

Summary

A user can stake to multiple providers, but the minimum staking amount is enforced only for the first staking action that the user submits.

Issue details

During the staking process, both for the stake and provider_boost extrinsics, a minimum staking amount should be enforced by the Runtime::MinimumStakingAmount parameter.

The current implementation of ensure_can_stake verifies if the amount that the account is staking in total (the current active staking and the new staking amount) is greater than the MinimumStakingAmount:

let new_active_staking_amount = staking_details
	.active
	.checked_add(&stakable_amount)
	.ok_or(ArithmeticError::Overflow)?;

ensure!(
	new_active_staking_amount >= T::MinimumStakingAmount::get(),
	Error::<T>::StakingAmountBelowMinimum
);

This implies that the user has to stake the MinimumStakingAmount only when they first submit a stake for a provider, all the subsequent staking requests being accepted even with the amount of 1.

Risk

The current implementation does not follow the intended use-case, as described in the design documentation.
An attacker could stake the minimum required amount to a provider, and then stake 1 token to each of the other available providers, underpaying for the storage that they use. This can cause a storage bloating, slowing down the network in the long run.

Mitigation

The ensure_can_stake function should verify that the stakable_amount is greater than MinimumStakingAmount, instead of verifying the new_active_staking_amount.

Closes #2135

Discussion

ensure_can_stake updated to remove the addition of the existing stakes and check stakable_amount
get_stakable_amount_for updated to use reducible_balance as recommended by parity when using the fungible trait. reducible_balance requires an Existential Deposit, so the unit test values were updated.
/designdocs/capacity.md updated to the latest code, and reflects changes in language due to replacing locks with freezes.
CI Checks are not running on this branch, but the unit tests and e2e tests passed in local.

Checklist

…pl' into audit-2024

…king module

mattheworris · 2024-08-21T14:36:21Z

designdocs/provider_boosting_implementation.md

@@ -255,6 +255,41 @@ pub enum Error<T> {
 }
 ```

+### NEW: Events


In updating the capacity events, noticed these events were added and this seemed like the best place to reference them.

mattheworris · 2024-08-21T14:37:59Z

designdocs/capacity.md

@@ -23,89 +23,105 @@ Frequency explains how Capacity can be acquired through staking, refills, and us
 - [Block space allocation for Capacity transactions](#block-space)
 - [Implementation of spending Capacity to perform transactions](#capacity-transactions)

-  **Implementation of how to acquire through staking:** <a id='staking'></a>
+### **Implementation of how to acquire Capacity through staking:** <a id='staking'></a>


markdown lint complains if there is no heading, so I tried to make the headings make sense.

mattheworris · 2024-08-21T14:39:22Z

designdocs/capacity.md


 ```rust
+ #[pallet::config]


Updated to the latest source code.

mattheworris · 2024-08-21T14:40:08Z

designdocs/capacity.md

+  #[pallet::constant]
+  type CapacityPerToken: Get<Perbill>;
+
+ // ...


Provider Boosting additions detailed in provider boosting docs, so left out here.

mattheworris · 2024-08-21T14:41:02Z

designdocs/capacity.md


-LockIdentifier is an eight-character long identifier used to distinguish between different locks.
+FreezeReason is an enum that defines the reason for freezing funds in an account.


There are many updates from the Currency->fungible changes from locks to freezes

mattheworris · 2024-08-21T14:48:51Z

designdocs/capacity.md


-Unstaking tokens allow you to schedule a number of tokens to be unlocked from your balance. There is no limit on the amount that you can schedule to be unlocked (up to the amount staked), but there is a limit on how many scheduled requests you can make. After scheduling tokens to be unlocked using **`unstake`**, you can withdraw those tokens after a thaw period has elapsed by using the **`withdraw_unstaked`** extrinsic. If the call is successful, all thawed tokens become unlocked and increase the ability to make more scheduled requests.
+Unstaking tokens allow you to schedule a number of tokens to be unfrozen from your balance. There is no limit on the amount that you can schedule to be unfrozen (up to the amount staked), but there is a limit on how many scheduled requests you can make. After scheduling tokens to be unfrozen using **`unstake`**, you can withdraw those tokens after a thaw period has elapsed by using the **`withdraw_unstaked`** extrinsic. If the call is successful, all thawed tokens become unfrozen and increase the ability to make more scheduled requests.


Note: I tried to be consistent and use the verb unfreeze when referring to unlock and keeping thaw to reference the period of time required before unfreezing.

thaw is also the verb used by fungible traits for unfreezing, previously unlocking.

Good catch since this was written before the Fungible trait change.

…ce() -> reducible_balance() as per fungible

mattheworris · 2024-08-21T16:17:06Z

pallets/capacity/src/lib.rs

@@ -831,7 +830,8 @@ impl<T: Config> Pallet<T> {
 		staker: &T::AccountId,
 		proposed_amount: BalanceOf<T>,
 	) -> BalanceOf<T> {
-		let account_balance = T::Currency::balance(&staker);
+		let account_balance =
+			T::Currency::reducible_balance(&staker, Preservation::Preserve, Fortitude::Polite);


This should have been changed with fungible, polkadot-sdk comments indicate that balance() is really only useful for testing and reducible_balance() is recommended.

mattheworris · 2024-08-21T16:18:19Z

pallets/capacity/src/tests/stake_and_deposit_tests.rs

@@ -255,10 +255,10 @@ fn stake_when_staking_amount_is_greater_than_free_balance_it_stakes_maximum() {
 		assert_ok!(Capacity::stake(RuntimeOrigin::signed(account), target, amount));

 		// Check that StakingAccountLedger is updated.
-		assert_eq!(StakingAccountLedger::<Test>::get(account).unwrap().active, 190);
+		assert_eq!(StakingAccountLedger::<Test>::get(account).unwrap().active, 189);


reducible_balance requires +1 for Existential Deposit, so these values are updated in the tests.

Further, I think the existing tests cover the changes, as the code is now more strict, so there are no additional tests to add.

mattheworris · 2024-08-21T16:19:48Z

pallets/capacity/src/lib.rs

-		tokens::fungible::{Inspect as InspectFungible, InspectFreeze, Mutate, MutateFreeze},
+		tokens::{
+			fungible::{Inspect as InspectFungible, InspectFreeze, Mutate, MutateFreeze},
+			Fortitude, Preservation,


Required for reducible_balance() changes

designdocs/capacity.md

shannonwells · 2024-08-22T17:14:05Z

designdocs/capacity.md


-  /// The available Capacity for an MSA account.
-  fn balance(msa_id: MessageSourceId) -> Result<Balance, DispatchError>;
+    /// The balance Capacity for an MSA.


Capacity balance? balance of Capacity?

Comments were copied from the code. I've updated both comments to read as the original:

/// The available Capacity for an MSA.

shannonwells

Whoops, my bad, I didn't notice it was a PR against the PB branch, sorry!
~~The only thing is to leave added Provider Boost related stuff out of main's version of the Design Doc.~~

shannonwells · 2024-08-22T17:14:54Z

designdocs/capacity.md

@@ -532,15 +573,15 @@ pub type CurrentBlockUsedCapacity<T: Config> = StorageValue<_, BalanceOf<T>, Val

 ```

-**Prioritization of Capacity transactions** <a id='priority'></a>
+### **Prioritization of Capacity transactions** <a id='priority'></a>


nit: Do we need both boldface and header markup?

shannonwells · 2024-08-22T17:17:18Z

designdocs/provider_boosting_implementation.md

+   amount: BalanceOf<T>,
+  },
+  /// Tokens have been staked on the network for Provider Boosting
+  ProviderBoosted {


The Provider Boost branch updates the Design Docs; let's keep those updates there instead of causing conflicts.

shannonwells

Didn't check the merge-into branch - looks good! The other comments are non-blocking.

enddynayn

👍 LGTM

Co-authored-by: Shannon Wells <[email protected]>

…ists for a different provider (#2136) A user can stake to multiple providers, but the minimum staking amount is enforced only for the first staking action that the user submits. During the staking process, both for the `stake` and `provider_boost` extrinsics, a minimum staking amount should be enforced by the `Runtime::MinimumStakingAmount` parameter. The current implementation of [ensure_can_stake](https://github.com/frequency-chain/frequency/blob/9ef58185f2230aaf92dc166fb8eade56a78e4cf3/pallets/capacity/src/lib.rs#L638) verifies if the amount that the account is staking in total (the current active staking and the new staking amount) is greater than the `MinimumStakingAmount`: ```rust let new_active_staking_amount = staking_details .active .checked_add(&stakable_amount) .ok_or(ArithmeticError::Overflow)?; ensure!( new_active_staking_amount >= T::MinimumStakingAmount::get(), Error::<T>::StakingAmountBelowMinimum ); ``` This implies that the user has to stake the `MinimumStakingAmount` only when they first submit a stake for a provider, all the subsequent staking requests being accepted even with the amount of 1. The current implementation does not follow the intended use-case, as described in the design documentation. An attacker could stake the minimum required amount to a provider, and then stake 1 token to each of the other available providers, underpaying for the storage that they use. This can cause a storage bloating, slowing down the network in the long run. The `ensure_can_stake` function should verify that the `stakable_amount` is greater than `MinimumStakingAmount`, instead of verifying the `new_active_staking_amount`. Closes #2135 - `ensure_can_stake` updated to remove the addition of the existing stakes and check `stakable_amount` - `get_stakable_amount_for` updated to use `reducible_balance` as recommended by parity when using the `fungible` trait. `reducible_balance` requires an Existential Deposit, so the unit test values were updated. - `/designdocs/capacity.md` updated to the latest code, and reflects changes in language due to replacing `lock`s with `freeze`s. - CI Checks are not running on this branch, but the unit tests and e2e tests passed in local. - [ ] Updated Pallet Readme? - [ ] Updated js/api-augment for Custom RPC OR Runtime API? - [x] Design doc(s) updated? - [ ] Unit Tests added? - [ ] e2e Tests added? - [ ] Benchmarks added? - [x] Spec version incremented? --------- Co-authored-by: Wil Wade <[email protected]> Co-authored-by: Shannon Wells <[email protected]>

wilwade and others added 7 commits August 2, 2024 16:28

Enable Passkey everywhere

777b20f

Merge branch 'feat/capacity-staking-rewards-impl' into audit-2024

30fef9e

Merge branch 'feat/capacity-staking-rewards-impl' into audit-2024

18f1ebf

Merge branch 'feat/capacity-staking-rewards-impl' into audit-2024

b0a3515

Merge remote-tracking branch 'origin/feat/capacity-staking-rewards-im…

3e38906

…pl' into audit-2024

Merge remote-tracking branch 'origin/feat/capacity-staking-rewards-im…

9ef5818

…pl' into audit-2024

chore: Update variable names and comments for clarity in capacity sta…

f3c43fc

…king module

mattheworris self-assigned this Aug 21, 2024

mattheworris changed the title ~~chore: Update variable names and comments for clarity in capacity sta…~~ [Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider Aug 21, 2024

mattheworris added bug Something isn't working Capacity Capacity Transactions labels Aug 21, 2024

mattheworris commented Aug 21, 2024

View reviewed changes

designdocs/capacity.md

```rust

#[pallet::config]

Copy link

Collaborator Author

mattheworris Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to the latest source code.

mattheworris commented Aug 21, 2024

View reviewed changes

chore: update unlock/freeze languauge in capacity.md

c9d74e7

mattheworris commented Aug 21, 2024

View reviewed changes

fix: remove addition of existing staking from ensure_can_stake; balan…

31bede1

…ce() -> reducible_balance() as per fungible

mattheworris changed the title ~~[Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider~~ [DRAFT] [Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider Aug 21, 2024

mattheworris marked this pull request as ready for review August 21, 2024 16:15

mattheworris requested a review from wilwade as a code owner August 21, 2024 16:15

mattheworris commented Aug 21, 2024

View reviewed changes

chore: Update spec version to 111 in runtime/frequency

26b111b

mattheworris changed the base branch from audit-2024 to feat/capacity-staking-rewards-impl August 21, 2024 16:31

fix: update branch changes

a64f659

mattheworris requested review from a team and removed request for a team August 21, 2024 18:17

mattheworris requested review from shannonwells, enddynayn, aramikm, claireclark1 and JoeCap08055 August 21, 2024 18:17

mattheworris changed the title ~~[DRAFT] [Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider~~ [Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider Aug 21, 2024

shannonwells reviewed Aug 22, 2024

View reviewed changes

designdocs/capacity.md Outdated Show resolved Hide resolved

shannonwells reviewed Aug 22, 2024

View reviewed changes

shannonwells requested changes Aug 22, 2024

View reviewed changes

shannonwells approved these changes Aug 23, 2024

View reviewed changes

JoeCap08055 approved these changes Aug 23, 2024

View reviewed changes

enddynayn approved these changes Aug 26, 2024

View reviewed changes

mattheworris and others added 2 commits August 26, 2024 07:54

Update designdocs/capacity.md

fac176a

Co-authored-by: Shannon Wells <[email protected]>

fix: comment typo

04d32ae

mattheworris merged commit e3ae518 into feat/capacity-staking-rewards-impl Aug 26, 2024

mattheworris deleted the 2135-capacity-minimumstakingamount-is-not-enforced-if-a-stake-already-exists-for-a-different-provider branch August 26, 2024 14:08

wilwade mentioned this pull request Sep 3, 2024

[Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider #2135

Closed


		LockIdentifier is an eight-character long identifier used to distinguish between different locks.
		FreezeReason is an enum that defines the reason for freezing funds in an account.


		Unstaking tokens allow you to schedule a number of tokens to be unlocked from your balance. There is no limit on the amount that you can schedule to be unlocked (up to the amount staked), but there is a limit on how many scheduled requests you can make. After scheduling tokens to be unlocked using `unstake`, you can withdraw those tokens after a thaw period has elapsed by using the `withdraw_unstaked` extrinsic. If the call is successful, all thawed tokens become unlocked and increase the ability to make more scheduled requests.
		Unstaking tokens allow you to schedule a number of tokens to be unfrozen from your balance. There is no limit on the amount that you can schedule to be unfrozen (up to the amount staked), but there is a limit on how many scheduled requests you can make. After scheduling tokens to be unfrozen using `unstake`, you can withdraw those tokens after a thaw period has elapsed by using the `withdraw_unstaked` extrinsic. If the call is successful, all thawed tokens become unfrozen and increase the ability to make more scheduled requests.

[Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider #2136

[Capacity] MinimumStakingAmount is not enforced if a stake already exists for a different provider #2136

Uh oh!

Conversation

mattheworris commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue details

Risk

Mitigation

Discussion

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattheworris Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattheworris Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattheworris Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shannonwells left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shannonwells left a comment

Choose a reason for hiding this comment

Uh oh!

enddynayn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattheworris commented Aug 21, 2024 •

edited

Loading

mattheworris Aug 21, 2024 •

edited

Loading

mattheworris Aug 21, 2024 •

edited

Loading

mattheworris Aug 21, 2024 •

edited

Loading

shannonwells left a comment •

edited

Loading