Olympia. Runtime. Fix moderation bug in the forum pallet.

[shamil-gadelshin]

Fixes #3111

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/joystream/pioneer-testnet/DmLJavgL9M72JzjWKNNfHd71DEYU
✅ Preview: Canceled

[Lezek123]

I'm not sure if this is an issue, but just wanted to point out that the reason for the initial implementation, ie.:

        let post = if Self::thread_exists(category_id, thread_id) {
            Self::ensure_post_is_mutable(&category_id, &thread_id, &post_id)?
        } else {
            <PostById<T>>::get(thread_id, post_id)
        };

Was probably to be consistent with the logic inside delete_posts.

And the reason why delete_posts allows deleting posts that belong to non-existing threads is because otherwise we may end up in a situation where a significat amount of PostDeposit stake is locked forever in state_cleanup_treasury_account in an unrecoverable state.

For example, in a situation where:

• A thread is created
• Multiple editable posts are created inside the thread (causing PostDeposit to be transferred to state_cleanup_treasury_account with each post)
• A thread is deleted

We are left with multiple posts that belong to a non-existing thread, but still exist in PostById map and have an associated stake.

Before the fix was implemented in this PR, it was possible to either slash the stake associated with those posts (using moderate_post) or recover it (using delete_posts).

After this fix only the latter will be possible.

[bedeho]

1. Starting with the original problem: can you confirm that the only way this problem occurs is that if the thread does not exist in storage (under stated category), in which case the elsebranch is executed in ensure_can_moderate_post, which leads to Ok returned? If the thread instead does exist under the given category, as indicated by a positive thread_exists, then there is no problem.

2. The reason this PR fixes the original issue is that ensure_post_exists basically checks that the thread corresponds to the given category, do you agree?

3. @Lezek123 Your point is, it should be possible to moderate, i.e. delete (by moderator), a post which corresponds to a thread which has been deleted? ( I agree).

4. @Lezek123 You said on the call that you were concerned that the fix in this PR would result in this no longer being possible, which further would result in bloat stake being perpetually lost. I tried to trace out the codepath of Self::ensure_post_is_mutable, which is very hard, but I think you are right about this also, namely:

   • ensure_post_is_mutable requires
     • ensure_post_exists: thread and post exist in storage (this is where we also verify that the thread belongs to provided catetegory)
     • ensure_thread_is_mutable requires
       • ensure_thread_exists: thread exists <- second time 🤕
       • ensure_category_is_mutable: requires
         • ensure_can_mutate_in_path_leaf: no ancestor categories are archived

5. This made my ask a more fundamental question: if the thread does not exist, how could we even in principle verify in which category it exists? This information lives in the Thread instance, which is gone.

6. This means there is no way to really solve the original problem (Forum bug: Posts can be moderated w/o category permissions #3111) properly. This is a fundamental design flaw of the system. One tempting fix is to replicate category information into the posts, but then moving threads between categories would cease to work. The fix in this PR does not really solve it, as it just does not allow moderation on threads that have been deleted.

7. The core problem of what happens when threads have been deleted actually causes problems other extrinsics also

• edit_post_text: fails, as it calls ensure_post_is_mutable as well.
• delete_posts: adds post to storage, without any state fee charged, before extrinsic is guaranteed to succeed
  

  joystream/runtime-modules/forum/src/lib.rs

  Line 1766 in ac4b65c

  <PostById<T>>::get(thread_id, post_id)

8. Overall I am not convinced about the integrity of the forum module as it currently stands, there have been too many cooks and I did not properly protect it in the latest rounds of review when state bloat management was introduced. Even the construct of having to identify posts by category_id x thread_id x post_id now seems like a total mess. I am very surprised the auditors accepted this.

9. My recommendation is: the original problem is less bad than any cure. having moderators be able to moderate outside their categories in the cases where threads have been deleted, it's not the end all be all. Its a bug which is even acceptable for mainnet, if it came to that, as you have the forensic evidence to see who did what, and its not something anyone can exploit. However, I suspect we should just redo this as a metaprotocol anyway, but we will see from our experience of trying that with video comments first. It holds the promise of a much simpler implementation.

[Lezek123]

having moderators be able to moderate outside their categories in the cases where threads have been deleted...

There is no need for any thread to be deleted to allow moderators to expoilt this issue.
The condition is:

if Self::thread_exists(category_id, thread_id) {

Where both category_id and thread_id is user input.

So, to give an example: If there is post 1 in thread 1 in cateogry 1 and a moderator has access to category 2, all he needs to do is provide input like: categoryId = 2, threadId = 1, postId = 1. thread_exists will return false, current permissions logic will allow the extrinsic to pass and since PostById::get(1, 1) is actually a valid post, it will be moderated.

[Lezek123]

So just to clarify the severity of the issue in one sentence: Any moderator having access to at least 1 category can moderate any post.

[bedeho]

So just to clarify the severity of the issue in one sentence: Any moderator having access to at least 1 category can moderate any post.

Yes, that was badly explained by me. We could however in fact restrict it to that case by doing

 let post = if Self::thread_exists(category_id, thread_id) {
 
            /** do  check on thread, which exists, to see that it lives in the given category **/
            
            Self::ensure_post_is_mutable(&category_id, &thread_id, &post_id)?
        } else {
            <PostById<T>>::get(thread_id, post_id)
        };

The fundamental problem is that when thread is gone, we can't even in principle offer this protection.

If this is correct, then I guess it is actually worth making this small improvement, as it radically restricts the set of posts a moderator actually can escalate privilege against.

[shamil-gadelshin]

else {
            <PostById<T>>::get(thread_id, post_id)
        };

This part could create the default post which would create a whole new set of problems.

[bedeho]

This part could create the default post which would create a whole new set of problems.

To clarify, we are talking about this

https://github.com/shamil-gadelshin/substrate-runtime-joystream/blob/olympia-forum-permissions-fix/runtime-modules/forum/src/lib.rs#L1754

Can we just drop this basically? If the post does not exist, then obviously ensure_can_delete_post is false anyway?

[bedeho]

End of the story here: #3111 (comment)