Audit Ref 22-05-982-REP v1.1 - INFO_15_17_18 LOW_20_21_22

[L3pereira]

Fixes some of the issues in the audit report #3840

Bounty module create_bounty INFO_15 Unsafe increment of the bounty_count

fund_bounty LOW_20 Unsafe arithmetic in funding_period_expired

announce_work_entry LOW_21 Unsafe increment of the entry_count INFO_18 Unsafe increment in increment_active_work_entry_counter

terminate_bounty INFO_17 Unnecessary work in the get_terminate_bounty_actor function

submit_oracle_judgment LOW_22 Missing event in case of reject

┆Issue is synchronized with this Asana task by Unito

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
pioneer-testnet	⬜️ Ignored (Inspect)		Sep 5, 2022 at 1:39PM (UTC)

[L3pereira]

@bedeho I have to change this storagemap Entries

        /// Work entry storage map.
        pub Entries get(fn entries): double_map
            hasher(blake2_128_concat) T::BountyId,
            hasher(blake2_128_concat) T::EntryId => Entry<T>;

https://github.com/L3pereira/joystream/blob/91b5bdcc49e78d269e933e82fcdcc79002e86f81/runtime-modules/bounty/src/lib.rs#L567-L569

into this

        /// Work entry storage map.
        pub Entries get(fn entries): double_map
            hasher(blake2_128_concat) T::BountyId,
            hasher(blake2_128_concat) T::EntryId => Option<Entry<T>>; <-------

because we have to implement std::default::Default in EntryRecord

#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
#[derive(Encode, Decode, Default, Clone, PartialEq, Eq, Debug)]
pub struct EntryRecord<AccountId, MemberId, BlockNumber> {
    /// Work entrant member ID.
    pub member_id: MemberId,

    /// Account ID for staking lock.
    pub staking_account_id: AccountId,

    /// Work entry submission block.
    pub submitted_at: BlockNumber,

    /// Signifies that an entry has at least one submitted work.
    pub work_submitted: bool,
}

https://github.com/L3pereira/joystream/blob/91b5bdcc49e78d269e933e82fcdcc79002e86f81/runtime-modules/bounty/src/lib.rs#L449-L463

The problem is, in this substrate version AccountId doesn't implement std::default::Default, so I've to implement an Option<Entry<T>> because we've no default value for AccountId. for example instead of verifying this way

    // Verifies work entry existence and retrieves an entry from the storage.
    fn ensure_work_entry_exists(
        bounty_id: &T::BountyId,
        entry_id: &T::EntryId,
    ) -> Result<Entry<T>, DispatchError> {
        ensure!(
            <Entries<T>>::contains_key(bounty_id, entry_id),
            Error::<T>::WorkEntryDoesntExist
        );

        let entry = Self::entries(bounty_id, entry_id);

        Ok(entry)
    }

We can use something like this

    // Verifies work entry existence and retrieves an entry from the storage.
    fn ensure_work_entry_exists(
        bounty_id: &T::BountyId,
        entry_id: &T::EntryId,
    ) -> Result<Entry<T>, DispatchError> {
        match Self::entries(bounty_id, entry_id) {
            Some(entry) => Ok(entry),
            None => Err(Error::<T>::WorkEntryDoesntExist)
        }
    }

[L3pereira]

• As described above there was a change in the Entries storage map (I'm open to a better solution)
• Constants in runtime lib have higher values due to (currency::DOLLARS), this makes the bounty benchmarks throw errors while generating weights, because of this, some initial values and asserts were modified.

[L3pereira]

@Lezek123 4. There's a serious issue with validate_judgment, because reward_sum_from_judgment is vulnerable to being purposefully overflown just to pass the reward_sum_from_judgment == bounty.total_funding check. Any unpaid reward will then be ignored, becasue transfer_funds_from_bounty_account is infallible (actually I think you should also make it fallible again, because now we have the default transactional protection)

Could you write a test where you exploit this vulnerability, I don't see how I can overflow the reward_sum_from_judgment and pass reward_sum_from_judgment == bounty.total_funding

[L3pereira]

5. Remark tests are missing cases where BountyActor::Member(id) is, for example, a valid creator (in case of creator_remark), but the signer is not the member controller account of this member. I think that's the case for all extrinsics that rely on BountyActor checking, because the mocks don't even differentiate between valid and invalid controller account of a member (ensure_member_controller_account_origin never fails)

I opened an issue to implement ensure_member_controller_account_origin, but doesn't make sense to implement it, you would be just testing a mock function. So Issue was closed

[bedeho]

The problem is that you are doing addition which can overflow here

https://github.com/L3pereira/joystream/blob/audit_report_ref_22-05-982-REP_v1.1_%233824/runtime-modules/bounty/src/lib.rs#L2183

So the oracle can provide a judgment where the rewards cause reward_sum_from_judgment to overflow, but the final value still matches the bounty.total_funding, this is trivial. This means the final check reward_sum_from_judgment == bounty.total_funding will incorrectly pass. The good thing is that when reward payouts happen, it is constrained by the balance of the bounty module account in this transfer https://github.com/L3pereira/joystream/blob/audit_report_ref_22-05-982-REP_v1.1_%233824/runtime-modules/bounty/src/lib.rs#L1407, however, it should still not happen, and in the future if the bounty module is changed without someone being aware of this, it could cause very bad unintended consequences.

Even if it’s not clear to you how exploitable this is at this point, lets just have it fixed, as it is more correct.

[Lezek123]

doesn't make sense to implement it, you would be just testing a mock function.

You would be testing at least whether it's actually being called when expected or not. Right now the tests would pass regardless of whether, for example, the ensure_bounty_actor_manager function actually calls ensure_member_controller_account_origin or not. I think it makes sense to have tests like that (we have such tests for content pallet for example).