Native rewards will be lost after instant unbound `unbond_instant`

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/main/src/modules/earning/src/lib.rs#L190-L196 https://github.com/code-423n4/2024-03-acala/blob/main/src/modules/incentives/src/lib.rs#L607-L619

Vulnerability details

Issue Description

When a user creates a bond for their native currency, the corresponding bond amount will be deposited as shares into the rewards contract after the T::OnBonded call, which invokes the OnEarningBonded function (adding shares to the rewards).

pub fn bond(origin: OriginFor<T>, #[pallet::compact] amount: Balance) -> DispatchResult {
    let who = ensure_signed(origin)?;

    let change = <Self as BondingController>::bond(&who, amount)?;

    if let Some(change) = change {
        T::OnBonded::happened(&(who.clone(), change.change));
        Self::deposit_event(Event::Bonded {
            who,
            amount: change.change,
        });
    }
    Ok(())
}

pub struct OnEarningBonded<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> Happened<(T::AccountId, Balance)> for OnEarningBonded<T> {
    fn happened((who, amount): &(T::AccountId, Balance)) {
        <orml_rewards::Pallet<T>>::add_share(who, &PoolId::Earning(T::NativeCurrencyId::get()), *amount);
    }
}

However, when a user decides to unbound instantly by calling unbond_instant, which frees and removes the user shares while imposing an unstake fee (penalty for unbounding early), there is a discrepancy in the removal of shares:

pub fn bond(origin: OriginFor<T>, #[pallet::compact] amount: Balance) -> DispatchResult {
    let who = ensure_signed(origin)?;

    let change = <Self as BondingController>::bond(&who, amount)?;

    if let Some(change) = change {
        T::OnBonded::happened(&(who.clone(), change.change));
        Self::deposit_event(Event::Bonded {
            who,
            amount: change.change,
        });
    }
    Ok(())
}

pub struct OnEarningBonded<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> Happened<(T::AccountId, Balance)> for OnEarningBonded<T> {
    fn happened((who, amount): &(T::AccountId, Balance)) {
        <orml_rewards::Pallet<T>>::add_share(who, &PoolId::Earning(T::NativeCurrencyId::get()), *amount);
    }
}

The issue arises when attempting to remove the shares through the T::OnUnbonded call, which invokes remove_share under the hood. Only the amount minus the fee (final_amount) is removed from the shares, not the fully unbounded amount (amount). This means that the portion of shares corresponding to the fee will not be removed from the rewards pool and will remain in there without the possibility of removal later.

As a result, the native rewards total shares will contain a portion of shares that cannot be removed (redeemed), stemming from the unstaking fees when users do an instant unbounding. Consequently, this portion will result in a loss of a portion of the total rewards accumulated in the pool as they cannot be claimed by anyone.

Impact

The unstaking fee is not removed from the rewards shares when users call unbond_instant, leading to a portion of the rewards being accumulated in the pool to be non-claimable.

Tools Used

Manual review, VS Code

Recommended Mitigation

In the unbond_instant function, the full amount should be removed from the rewards pool.

Assessed type

Context

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #24

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-judge]

OpenCoreCH marked the issue as satisfactory