Unnecessary iteration increases gas costs and decreases performance efficiency.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L191-L251

Vulnerability details

There is a potential performance inefficiency when updating withdrawn rewards for reward currencies in which the user has no balance.

• The function currently iterates through all reward currencies, even if the user has no withdrawn or pending rewards in some of those currencies.
• This unnecessary iteration could lead to suboptimal performance, especially if there are many reward currencies and users with sparse reward balances.

Impact

• The performance inefficiency in the remove_share() function does not directly impact the functionality or security of the system.
• However, it may lead to suboptimal performance and increased gas costs, especially in scenarios with many reward currencies and users with sparse reward balances.
• This could potentially affect the overall efficiency and scalability of the system.

Issue Description

• The remove_share() function is called when a user wants to remove a portion of their staked shares from a pool.
• It updates the user's share balance, the pool's total shares, and the user's withdrawn rewards for each reward currency.
• The function takes the following inputs:
  • who: The account ID of the user removing shares
  • pool: The ID of the pool from which shares are being removed
  • remove_amount: The amount of shares to be removed
• Key steps:
  1. Claim any pending rewards for the user in the specified pool.
  2. _Update the user's share balance and the pool's total shares based on the remove_amount._
  3. Calculate the proportion of removed shares to the user's total shares.
  4. Iterate through all reward currencies and update the user's withdrawn rewards and the pool's total rewards based on the removed share proportion.

It is expected to update the user's share balance, the pool's total shares, and the user's withdrawn rewards correctly based on the removed share amount.

• It should calculate the proportion of removed shares to the user's total shares and update the withdrawn rewards and pool's total rewards accordingly.

The remove_share() function behaves as expected, updating the share balances and withdrawn rewards correctly. However, it iterates through all reward currencies, even for currencies in which the user has no balance, leading to unnecessary computation. That's my point.

Proof of Concept

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L191-L251

fn remove_share(who: &T::AccountId, pool: &T::PoolId, remove_amount: T::Share) {
    // ... (existing code)

    // Update withdrawn rewards for each reward currency
    withdrawn_rewards
        .iter_mut()
        .for_each(|(reward_currency, withdrawn_reward)| {
            // ... (existing code)
        });

    // ... (existing code)
}

Tools Used

Manual Audit

Recommended Mitigation Steps

Consider modifying the iteration over reward currencies to only update withdrawn rewards for currencies in which the user has a non-zero balance.

     fn remove_share(who: &T::AccountId, pool: &T::PoolId, remove_amount: T::Share) {
         // ... (existing code)

         // Update withdrawn rewards only for currencies with non-zero user balance
         for (reward_currency, withdrawn_reward) in user_reward_currencies(who) {
             if let Some((total_reward, total_withdrawn_reward)) = pool_info.rewards.get_mut(reward_currency) {
                 // ... (existing code)
             }
         }

         // ... (existing code)
     }

• Now, the user_reward_currencies(who) function would return an iterator over the reward currencies in which the user has a non-zero balance, allowing for efficient iteration and update of withdrawn rewards.

Assessed type

Loop

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[DadeKuma]

Spam

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Insufficient quality