[High] Reachable panic via index out of bounds in gated-marketplace pallet

[sebastianmontero]

[High] Reachable panic via index out of bounds in gated-marketplace pallet

Summary

An index out of bounds panic can be triggered by a malicious actor calling an extrinsic in the gated-marketplace pallet.

Issue details

The set_up_application helper function can be called with input such that fields.len() > custodian_fields.as_ref().unwrap().1.len() -- for example, if there are elements in fields but not in custodian_fields.1.

In such cases, the function will try to access a non-existent index in custodian_fields.1, throwing an index out of bounds exception. This causes the runtime to panic and could potentially cause a deny-of-service of the chain.

The helper function is directly reachable from the following extrinsics:

• pallet_gated_marketplace::invite
• pallet_gated_marketplace::apply
• pallet_gated_marketplace::reapply

For example here:

#[pallet::call_index(3)]
#[pallet::weight(Weight::from_parts(10_000,0) + T::DbWeight::get().writes(1))]
pub fn apply(
	origin: OriginFor<T>,
	marketplace_id: [u8; 32],
	// Getting encoding errors from polkadotjs if an object vector have optional fields
	fields: Fields<T>,
	custodian_fields: Option<CustodianFields<T>>,
) -> DispatchResult {
	let who = ensure_signed(origin)?;

	let (custodian, fields) = Self::set_up_application(fields, custodian_fields);

Risk

Since this can be triggered by a call to an extrinsic, an attacker could cause a validator node to crash by submitting a specific crafted extrinsic and cause them to miss their block authoring slot. This could lead to a denial-of-service of the whole chain with only very low cost requirements for the attacker.

Mitigation

Validate the arguments prior to calling set_up_application, e.g., ensure that custodian_fields.as_ref().unwrap().1.len() == fields.len() holds before calling the function. Note that this change needs to be applied everywhere set_up_application is called.
Alternatively, modify set_up_application to gracefully handle errors.

[sebastianmontero]

I think we should have a single structure for the application fields, where the custodian fields are optional, this would restrict the possibility of calling the extrinsic with an invalid configuration of fields.