Skip to content

pallet-collective: Unbounded ProposalOf causes weight under-estimation in close extrinsic #10209

@RomarQ

Description

@RomarQ

Description

The close extrinsic takes length_bound as a parameter for calculating a worst-case scenario weight for the call, but this can result in an under-estimation when the length_bound provided is lower than the real encoded call stored in ProposalOf.

When the length_bound is less than the real length of the proposal, the call will fail as expected with WrongProposalLength, but the charged weight will be less than it should, since it reads the entire encoded value from storage to be able to assert that length_bound is greater or equal to the real length of the proposal call.

Weight calculation:

/// + `proposal_weight_bound`: The maximum amount of weight consumed by executing the closed
/// proposal.
/// + `length_bound`: The upper bound for the length of the proposal in storage. Checked via
/// `storage::read` so it is `size_of::<u32>() == 4` larger than the pure length.
///
/// ## Complexity
/// - `O(B + M + P1 + P2)` where:
/// - `B` is `proposal` size in bytes (length-fee-bounded)
/// - `M` is members-count (code- and governance-bounded)
/// - `P1` is the complexity of `proposal` preimage.
/// - `P2` is proposal-count (code-bounded)
#[pallet::call_index(6)]
#[pallet::weight((
{
let b = *length_bound;
let m = T::MaxMembers::get();
let p1 = *proposal_weight_bound;
let p2 = T::MaxProposals::get();
T::WeightInfo::close_early_approved(b, m, p2)
.max(T::WeightInfo::close_early_disapproved(m, p2))
.max(T::WeightInfo::close_approved(b, m, p2))
.max(T::WeightInfo::close_disapproved(m, p2))
.saturating_add(p1)
},

Length check:
let key = ProposalOf::<T, I>::hashed_key_for(hash);
// read the length of the proposal storage entry directly
let proposal_len =
storage::read(&key, &mut [0; 0], 0).ok_or(Error::<T, I>::ProposalMissing)?;
ensure!(proposal_len <= length_bound, Error::<T, I>::WrongProposalLength);

As a solution for this, the pallet collective should leverage the preimage pallet which offers mechanisms to correctly handle bounds. More context on what can be done: paritytech/substrate#11649

Impact

The impact seems minimal because only members of the collective can create proposals.

Metadata

Metadata

Assignees

No one assigned

    Labels

    I10-unconfirmedIssue might be valid, but it's not yet known.I2-bugThe node fails to follow expected behavior.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions