Static fee charged despite dynamic storage (Bad extrinsic weight)

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/main/src/modules/incentives/src/lib.rs#L265 https://github.com/code-423n4/2024-03-acala/blob/main/src/modules/incentives/src/lib.rs#L425

Vulnerability details

Impact

Blockchain computations must have appropriate fees to prevent network congestion. For substrate extrinsics, these fees are set by computing an associated weight for the operation, where the weight should capture the maximum computation cost. As reads and writes to storage are expensive, the weights should consider the number of these operations that are performed.

But as can be seen in the claim_rewards function of the incentive module,

        #[pallet::call_index(2)]
    #[pallet::weight(<T as Config>::WeightInfo::claim_rewards())]
        pub fn claim_rewards(origin: OriginFor<T>, pool_id: PoolId) -> DispatchResult {

a fixed weight have been defined, despite performing a dynamic number of the read/write operations. In the for loop of the function, it writes to storage, based on the size of pending_multi_reward as seen in (https://github.com/code-423n4/2024-03-acala/blob/main/src/modules/incentives/src/lib.rs#L460)

*pending_reward = Zero::zero();

However, the weight fails to consider this, and instead uses a fixed weight. This may lead to DOS scenerio. As the size of the pending_multi_rewards increases, the for loop will increase, which will increase the above write operation. If the size of loop becomes too large, this may lead to Denial Of Service issue

Proof of Concept

https://github.com/code-423n4/2024-03-acala/blob/main/src/modules/incentives/src/lib.rs#L460

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to use a dynamic weight

Assessed type

Other

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[DadeKuma]

Weight might be underestimated or overestimated

[c4-sponsor]

xlc (sponsor) disputed

[xlc]

It is not an issue to overestimate weight (unless it is extraordinary overestimate). A protected origin (i.e. governance) is required to adjust reward pool so it is not possible to make the number of pending rewards too high. Therefore the benchmark only need to work with a upper cap number of pending rewards and there won't be any issues.

[OpenCoreCH]

Wrong weights are a potential issue on Polkadot, but just pointing out that a weight is currently static and the code contains dynamic logic fails to show any real impact and the issue as it stands is only a QA recommendation. Therefore downgrading

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)