Joystream / joystream

Joystream Monorepo
http://www.joystream.org
GNU General Public License v3.0
1.42k stars 115 forks source link

[Nara] Shouldn't be possible to buy/sell token when CRT pallet is frozen #5028

Open freakstatic opened 10 months ago

freakstatic commented 10 months ago

As @ivanturlakov reported here it seems that is possible to buy and sell tokens even with the CRT pallet frozen. Since there are some runtime tests covering the sale, maybe atlas is not displaying the error or something happen during the tests that made the pallet not being actually frozen.

freakstatic commented 10 months ago

it seems that this bug could be related with freezing, unfreezing the pallet and then freezing it again

mnaamani commented 10 months ago

Looks like we simply forgot to add a guard for both buy_on_amm and sell_on_amm dispatch calls

Fix:

diff --git a/runtime-modules/project-token/src/lib.rs b/runtime-modules/project-token/src/lib.rs
index 960ea3a284..5fba7a5ab3 100644
--- a/runtime-modules/project-token/src/lib.rs
+++ b/runtime-modules/project-token/src/lib.rs
@@ -836,6 +836,8 @@ decl_module! {
         /// - event deposited
         #[weight = WeightInfoToken::<T>::buy_on_amm_with_existing_account()]
         fn buy_on_amm(origin, token_id: T::TokenId, member_id: T::MemberId, amount: <T as Config>::Balance, slippage_tolerance: Option<(Permill, JoyBalanceOf<T>)>) -> DispatchResult {
+            Self::ensure_unfrozen_state()?;
+
             if amount.is_zero() {
                 return Ok(()); // noop
             }
@@ -916,6 +918,8 @@ decl_module! {
         /// - event deposited
         #[weight = WeightInfoToken::<T>::sell_on_amm()]
         fn sell_on_amm(origin, token_id: T::TokenId, member_id: T::MemberId, amount: <T as Config>::Balance, slippage_tolerance: Option<(Permill, JoyBalanceOf<T>)>) -> DispatchResult {
+            Self::ensure_unfrozen_state()?;
+
             if amount.is_zero() {
                return Ok(()); // noop
             }
mnaamani commented 10 months ago

My best guess is in the QA testing, not the same extrinsic was tested with pallet frozen vs when it was not-frozen.

mnaamani commented 10 months ago

It seems that when reviewing https://github.com/Joystream/joystream/pull/4871 I missed these two new dispatch calls