Review Quarkslab SAS Joystream Security Audit Ref 22-05-982-REP v1.1 date 2022/05/31

Bounty module

---

create_bounty
LOW_19 No upper bound for funding_period
The upper bound for the funding period, shouldn't be a problem. Setting this a priori it's difficult because it's dependent on the block processing time, if the processing time is fast, an absolute maximum limit will affect the probability of getting the funds, since the funding period will terminate faster than expected.
The problem due to someone setting the funding period too high can be mitigated, because the council can terminate the bounty via terminate_bounty which can be done in Funding stage (with or without contributions)

INFO_15 Unsafe increment of the bounty_count
To be fixed, but we would have issues even with safe arithmetic (saturating_add),
because near the saturation we would create the same Id over and over again
We could use a check function before mutation safe

let next_bounty_count_value = Self::get_checked_add(&Self::bounty_count(), &1u32)?;

fn get_checked_add<U>(x: &U, y: &U) -> Result<U, DispatchError>
   where U: CheckedAdd{
      x.checked_add(y).ok_or(Error::<T>::AddOperationOverflow.into())
}

or

let next_bounty_count_value = Self::get_checked_inc_u32(&Self::bounty_count())?;

fn get_checked_inc_u32<U>(x: &U) -> Result<U, DispatchError>
 where U: CheckedAdd + From<u32>{
     x.checked_add(&1.into()).ok_or(Error::<T>::IncOperationOverflow.into())
}

INFO_16 User could be deleted by using this extrinsic
I don't see how this could happen
To be discussed: if It's a problem a creator be an oracle

---

fund_bounty
LOW_20 Unsafe arithmetic in funding_period_expired
Here we could use saturating_add, this means that the maximum funding period is always u64::max()

fn funding_period_expired(&self, created_at: T::BlockNumber) -> bool {
    match self.bounty.creation_params.funding_type {
         // Never expires
         FundingType::Perpetual { .. } => false,
         FundingType::Limited { funding_period, .. } => {
             created_at.saturating_add(funding_period) < self.now
         }
   }
}

---

terminate_bounty
INFO_17 Unnecessary work in the get_terminate_bounty_actor function
To be fixed

let terminate_bounty_actor = Self::get_terminate_bounty_actor(origin, &bounty)?;

we can apply the inlined function directly

let terminate_bounty_actor = BountyActorManager::<T>::ensure_bounty_actor_manager(
    origin,
    bounty.creation_params.creator.clone(),
)?;

---

announce_work_entry
LOW_21 Unsafe increment of the entry_count
INFO_18 Unsafe increment in increment_active_work_entry_counter
To be fixed, but we would have issues even with safe arithmetic (saturating_add),
because near the saturation we would create the same Id over and over again
We could use a check function before mutation safe

            Self::ensure_valid_contract_type(&bounty, &member_id)?;
            let next_entry_count_value = Self::get_checked_inc_u32(&Self::entry_count())?;
            let next_active_work_entry_count = Self::get_checked_inc_u32(&bounty.active_work_entry_count)?;

            //
            // == MUTATION SAFE ==
            //
            <Entries<T>>::insert(bounty_id, entry_id, entry);
            EntryCount::mutate(|count| {
                *count = next_entry_count_value
            });
            // Increment work entry counter and update bounty record.
            <Bounties<T>>::mutate(bounty_id, |bounty| {
                bounty.active_work_entry_count = next_active_work_entry_count;
            });

INFO_19 The worker can be the oracle
To be discussed: if It's a problem a worker be an oracle (at least there is conflict of interest, the oracle would be judging his work)

---

switch_oracle
INFO_20 The new oracle can be a worker
Same as INFO_19

---

submit_oracle_judgment
MEDIUM_10 Funds of participants can stay locked
This is not an issue, since the omitted participants
can unlock their funds by calling withdraw_entrant_stake
in the FailedBountyWithdrawal or SuccessfulBountyWithdrawal

LOW_22 Missing event in case of reject
If we already emmit an event for winners we also should emmit to rejected work

                    OracleWorkEntryJudgment::Rejected{
                        slashing_share,
                        ..
                    } => {
                        let entry = Self::entries(&bounty_id, &entry_id);
                        let slashing_amount = slashing_share * bounty.creation_params.entrant_stake;

                        if slashing_amount > Zero::zero() {
                            T::StakingHandler::slash(&entry.staking_account_id, Some(slashing_amount));
                        }

                        T::StakingHandler::unlock(&entry.staking_account_id);

                        Self::remove_work_entry(&bounty_id, &entry_id);

                        // Fire a WorkEntrantStakeSlashed event.
                        Self::deposit_event(RawEvent::WorkEntrantStakeSlashed(<------------------------------------------
                            bounty_id,
                            *entry_id,
                            entry.staking_account_id
                        ));
                    }

---

Storage module

---

update_data_size_fee
MEDIUM_4 The new_data_size_fee parameter is not bounded
This is related to the exchange rates so it's hard to set bounds that need to be changed dynamically, if this is to be set arbitrarily, I don't know what a reasonable value would be

check_buckets_for_overflow
MEDIUM_5 Usage of unsafe addition
This can be fixed by the following

    // Iterates through buckets. Verifies voucher parameters to fit the new limits:
    // objects number and total objects size.
    fn check_buckets_for_overflow(
        bucket_ids: &BTreeSet<T::StorageBucketId>,
        voucher_update: &VoucherUpdate,
    ) -> DispatchResult {
        for bucket_id in bucket_ids.iter() {

            let bucket = Self::storage_bucket_by_id(bucket_id);

            let objs_voucher_sum = Self::get_checked_add(
                       &voucher_update.objects_number, 
                       &bucket.voucher.objects_used)?;

            // Total object number limit is not exceeded.
            ensure!(
                objs_voucher_sum <= bucket.voucher.objects_limit,
                Error::<T>::StorageBucketObjectNumberLimitReached
            );

            let objs_voucher_size_sum = Self::get_checked_add(
                       &voucher_update.objects_total_size, 
                       &bucket.voucher.size_used)?;

            // Total object size limit is not exceeded.
            ensure!(
                objs_voucher_size_sum <= bucket.voucher.size_limit,
                Error::<T>::StorageBucketObjectSizeLimitReached
            );
        }

        Ok(())
    }

fn get_checked_add<U>(x: &U, y: &U) -> Result<U, DispatchError>
   where U: CheckedAdd{
      x.checked_add(y).ok_or(Error::<T>::AddOperationOverflow.into())
}

Start by reviewing the bounty-module entry points named in the audit, including create_bounty, fund_bounty, terminate_bounty, announce_work_entry, switch_oracle, and submit_oracle_judgment, then inspect the storage-module functions update_data_size_fee and check_buckets_for_overflow. Separate the proposed fixes from the findings marked for discussion and define the accepted behavior before making changes; the work is done when the agreed audit findings are resolved and the related behavior is verified.