Unvalidated input allows an attacker to gain disproportionate rewards unfairly.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/modules/incentives/src/lib.rs#L515 https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/modules/incentives/src/lib.rs#L512-L523

Vulnerability details

Due to insufficient validation of the amount parameter in the deposit_dex_share function.

• The function directly uses the amount provided by the user to calculate and update the user's shares via the add_share function.

  • However, there is no validation to ensure that the amount is within a reasonable range or matches the user's actual token balance.

• An attacker can potentially exploit this vulnerability by providing a large amount value, even if they don't possess the corresponding number of tokens.
• Line 515

    <orml_rewards::Pallet<T>>::add_share(who, &PoolId::Dex(lp_currency_id), amount.unique_saturated_into());

• This line directly passes the user-provided amount to the add_share function without proper validation.

Issue Description

The purpose of this is to allow users to stake their tokens and receive shares in the DEX pool.

• The function takes the following inputs:

  • who: The account ID of the user staking the tokens.
  • lp_currency_id: The ID of the LP token being staked.
  • amount: The amount of tokens being staked.

• The function performs the following key steps:

  1. Transfers the staked tokens from the user's account to the Incentives module's account.
  2. Calls the add_share function from the Rewards module to calculate and update the user's shares based on the staked amount.
  3. Emits a DepositDexShare event to record the staking activity.

• Code snippet:

  
  ensure!(lp_currency_id.is_dex_share_currency_id(), Error::<T>::InvalidCurrencyId);

T::Currency::transfer(lp_currency_id, who, &Self::account_id(), amount)?; <orml_rewards::Pallet>::add_share(who, &PoolId::Dex(lp_currency_id), amount.unique_saturated_into());

Self::deposit_event(Event::DepositDexShare { who: who.clone(), dex_share_type: lp_currency_id, deposit: amount, }); Ok(()) }

   - Should only accept valid `amount` values that correspond to the user's actual token balance.
   - It should enforce proper validation and checks to ensure that the staked `amount` is within a reasonable range and does not exceed the user's available tokens.

   - An attacker can potentially provide a large `amount` value to the `deposit_dex_share` function, even if they don't have sufficient tokens.
   - The function will proceed to calculate and update the user's shares based on the manipulated `amount`, resulting in the attacker gaining a disproportionate number of shares.
   - This behavior deviates from the expected outcome, as the attacker can obtain shares that are not proportional to their actual staked amount.

## Proof of Concept
> Lack of proper validation of the staked amount provided by the user during the staking process As a result, an attacker can manipulate the staked amount to gain a disproportionate number of shares in the DEX pool, leading to an unfair distribution of rewards.

Let's assume all the incentive modules and configure them with the necessary parameters, including the DEX pool and reward distribution rules.

Legitimate user stake their LP tokens by calling the `deposit_dex_share` function with a valid `amount` parameter that matches their actual token balance.

```rust
let legitimate_user = AccountId::from([1; 32]);
let lp_currency_id = CurrencyId::from([1; 32]);
let legitimate_stake_amount = 1000; // Assuming the user holds at least 1000 tokens

Pallet::<T>::deposit_dex_share(&legitimate_user, lp_currency_id, legitimate_stake_amount)?;

3. Attacker Staking with Manipulated Amount: An attacker account calls the deposit_dex_share function with a manipulated amount parameter that exceeds their actual token balance.

let attacker = AccountId::from([2; 32]);
let manipulated_stake_amount = 1_000_000_000; // A large amount that exceeds the attacker's balance

Pallet::<T>::deposit_dex_share(&attacker, lp_currency_id, manipulated_stake_amount)?;

QQuery the Rewards module's storage to check the shares allocated to the legitimate user and the attacker. You should observe that the attacker has gained a disproportionate number of shares compared to the legitimate user, despite not actually staking the corresponding amount of tokens.

let legitimate_user_shares = <rewards::Pallet<T>>::shares(&PoolId::Dex(lp_currency_id), &legitimate_user);
let attacker_shares = <rewards::Pallet<T>>::shares(&PoolId::Dex(lp_currency_id), &attacker);

assert!(attacker_shares >> legitimate_user_shares);

Impact

This can lead to an unfair distribution of shares and rewards among users, favoring the attacker.

• The attacker may be able to manipulate the share-based reward calculations and potentially drain rewards meant for legitimate users.

Tools Used

Manual Audit

Recommended Mitigation Steps

The function should ensure that the amount is within a valid range and does not exceed the user's available token balance.

         // Check if the user has sufficient balance
         ensure!(T::Currency::free_balance(lp_currency_id, who) >= amount, Error::<T>::InsufficientBalance);

         // Validate the staked amount against a reasonable range
         ensure!(amount >= T::MinStakingAmount::get(), Error::<T>::AmountTooLow);
         ensure!(amount <= T::MaxStakingAmount::get(), Error::<T>::AmountTooHigh);

         T::Currency::transfer(lp_currency_id, who, &Self::account_id(), amount)?;

         <rewards::Pallet<T>>::add_share(who, &PoolId::Dex(lp_currency_id), amount.unique_saturated_into());

         Self::deposit_event(Event::DepositDexShare {
             who: who.clone(),
             dex_share_type: lp_currency_id,
             deposit: amount,
         });

         Ok(())
     }

• In this version:
  • The function checks if the user has a sufficient balance using T::Currency::free_balance before proceeding with the staking.
  • It validates the amount against a reasonable range defined by T::MinStakingAmount and T::MaxStakingAmount to prevent extremely low or high staking amounts.
  • If any of the validation checks fail, appropriate error messages are returned to the user.

     // Check if the user has sufficient balance
     ensure!(T::Currency::free_balance(lp_currency_id, who) >= amount, Error::<T>::InsufficientBalance);

     // Validate the staked amount against a reasonable range
     ensure!(amount >= T::MinStakingAmount::get(), Error::<T>::AmountTooLow);
     ensure!(amount <= T::MaxStakingAmount::get(), Error::<T>::AmountTooHigh);

Assessed type

Invalid Validation

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[DadeKuma]

Spam

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Insufficient quality