Weight Calculation and Benchmarking Should Be Improved

[timurguvenkaya]

Description

It was observed that add_attribute, update_attribute, read_attribute, and remove_attributeuse a static weight while some arguments provided are vectors. Incorrect weight calculation might lead to situations when a malicious actor can spam a network (this can cause performance degradation) by paying low fees while sending a tx, which causes longer calculation time (large vector).

#[pallet::weight(T::WeightInfo::add_attribute())]
        pub fn add_attribute(
            origin: OriginFor<T>,
            did_account: T::AccountId,
            name: Vec<u8>,
            value: Vec<u8>,
            valid_for: Option<T::BlockNumber>,
        ) -> DispatchResult {
    
 #[pallet::weight(T::WeightInfo::update_attribute())]
        pub fn update_attribute(
            origin: OriginFor<T>,
            did_account: T::AccountId,
            name: Vec<u8>,
            value: Vec<u8>,
            valid_for: Option<T::BlockNumber>,
        ) -> DispatchResult {
        
      #[pallet::weight(T::WeightInfo::read_attribute())]
        pub fn read_attribute(
            origin: OriginFor<T>,
            did_account: T::AccountId,
            name: Vec<u8>,
        ) -> DispatchResult {
        
        #[pallet::weight(T::WeightInfo::remove_attribute())]
        pub fn remove_attribute(
            origin: OriginFor<T>,
            did_account: T::AccountId,
            name: Vec<u8>,
        ) -> DispatchResult {

In addition to this, in benchmarking.rs worst case scenarios are not used. Hence, it won't reflect the performance under worst-case situations.

const CALLER_ACCOUNT_STR: &str = "Iredia1";
const DID_ACCOUNT_STR: &str = "Iredia2";
const NAME_BYTES: &[u8; 2] = b"id";
const ATTRITUBE_BYTES: &[u8; 17] = b"did:pq:1234567890";

Recommendation

You should take into account the number of elements in the vector when calculating weight.

Example:

#[pallet::weight(T::WeightInfo::remove_attribute(name.len() as u32))]

In benchmarking.rs, follow your worst-case constraints, and you should update it to take into account a dynamic value.

For instance, in add_attribute, you have:

   ensure!(name.len() <= 64, Error::<T>::AttributeNameExceedMax64);

Then, in benchmarking for add_attribute, you should generate an attribute name with a range between 1 -64 and the value between 1 - worst-case range you expect

Example from another project: https://github.com/NodleCode/chain/blob/master/pallets/sponsorship/src/benchmarking.rs#L136C21-L136C27