The `set_stickied_threads` extrinsic can be used to cheaply fill up the blockchain storage

[viniul]

Summary

The extrinsic set_stickied_threads in runtime-modules/forum/src/lib.rs allows a forum moderator to store a vector of stickied_ids on-chain. There are two issues in this extrinsic, that together result in an issue of moderate severity:

1. The stickied_ids vector is not checked for duplicate ids.
2. No deposit is charged for setting stickied_ids.

Combining this two issues would allow an attacker to fill up storage very cheaply and clutter the blockchain storage.

Checking for duplicate ids and fixing issue X would mitigate this issue.

Issue details

A forum moderator can set sticky threads by calling the set_stickied_threads extrinsic in runtime-modules/forum/src/lib.rs. This extrinsic checks for every thread id if that thread id exists and then stores the whole stickied_ids vector on-chain:

fn set_stickied_threads(origin, actor: PrivilegedActor<T>, category_id: T::CategoryId, stickied_ids: Vec<T::ThreadId>) -> DispatchResult {
    [...]
    Self::ensure_can_set_stickied_threads(account_id, &actor, &category_id, &stickied_ids)?;
    <CategoryById<T>>::mutate(category_id, |category| category.sticky_thread_ids = stickied_ids.clone());
    [...]

There are two issues with that process:

1. The stickied_ids vector is not checked for duplicate ids. As a result, a malicious forum moderator can store a very large vector on-chain by submitting a vector stickied_ids that just consists of one thread id repeated a lot of times.
2. No deposit is charged for setting stickied_ids. We recommend to always require a deposit that scales linearly with the input when storing arbitrary-length user input on-chain.

Note that there is no deposit charged for creating a thread as well.

Risk

This issue would allow an attacker to fill up the blockchain storage by setting with a very long stickied_ids vector for a lot categories. A full storage is problematic because it could lead to an infeasible amount of storage being required to run a blockchain node.

The risk is reduced by two factors:

1. Only a category moderator can set sticky threads.
2. Only one sticky_thread_ids vector per category is stored. This limits the potential to fill up the blockchain storage.

Mitigation

The mitigation is two-fold:

1. Ensure that the stickied_ids vector does not contain any duplicate thread ids.

To ensure that a correct deposit is charged, we see two possibilities:

2.1 Charge a deposit that scales linearly with the length of the stickied_ids vector. An example of how this could be calculated can be found in the Substrate runtime, see here

Substrate/bin/node/runtime/src/constants.rs

which contains this function:

    pub const fn deposit(items: u32, bytes: u32) -> Balance {
        items as Balance * 20 * DOLLARS + (bytes as Balance) * 100 * MILLICENTS
    }

Another extrinsic in Substrate which can be taken as an example is the set_identity extrinsic in Substrate/frame/identity/src/lib.rs

fn set_identity(
   let extra_fields = info.additional.len() as u32;
   [...]
   let fd = <BalanceOf<T>>::from(extra_fields) * T::FieldDeposit::get();

2.2 Charge a deposit for each thread. This is the same fix as suggested in issue #5. Note that this will only completely fix the issue if the set_stickied_threads enforces that the stickied_ids vector is duplicate-free. If that is the case, then each of the threads will already have had a deposit charged for it, making it unnecessary to charge a deposit again here.