Attackers can unbond tokens without paying fees or manipulate process.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/modules/earning/src/lib.rs#L180-L205 https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/modules/earning/src/lib.rs#L192-L194

Vulnerability details

Description

The unbond_instant() function is expected to withdraw the instant unstake fee from the user's account and transfer it to the treasury account. The user should receive the unbonded amount minus the fee, and the unbonding should be processed instantly.

The function enables users to instantly unbond their tokens by paying a fee.

lib.rs#pub fn unbond_instant

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

    let fee_ratio = T::ParameterStore::get(InstantUnstakeFee).ok_or(Error::<T>::NotAllowed)?;

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

    if let Some(change) = change {
        let amount = change.change;
        let fee = fee_ratio.mul_ceil(amount);
        let final_amount = amount.saturating_sub(fee);

        let unbalance =
            T::Currency::withdraw(&who, fee, WithdrawReasons::TRANSFER, ExistenceRequirement::KeepAlive)?;
        T::OnUnstakeFee::on_unbalanced(unbalance);

        T::OnUnbonded::happened(&(who.clone(), final_amount));
        Self::deposit_event(Event::InstantUnbonded {
            who,
            amount: final_amount,
            fee,
        });
    }

    Ok(())
}

1. It retrieves the instant unstake fee ratio from the ParameterStore.
2. It calls the unbond_instant() function from the BondingController trait to perform the instant unbonding.
3. If the unbonding is successful, it calculates the fee based on the fee ratio and the unbonded amount.
4. It withdraws the fee from the user's account using the Currency trait and transfers it to the treasury account via the OnUnstakeFee hook.
5. It emits an InstantUnbonded event with the final unbonded amount and the fee.

Vulnerability Details

If the Balances module which is used to withdraw the instant unstake fee, allows reentrancy. Reentrancy occurs when an external contract or module can call back into the original contract or module before the initial function execution is complete, potentially leading to unexpected behavior or exploitation.

We can see in this line: lib.rs#192-L194

let unbalance =
T::Currency::withdraw(&who, fee, WithdrawReasons::TRANSFER, ExistenceRequirement::KeepAlive)?;
T::OnUnstakeFee::on_unbalanced(unbalance);

If the withdraw function in the Balances module allows reentrancy, an attacker could potentially exploit this by creating a malicious contract that receives the instant unstake fee. The malicious contract could then call back into the unbond_instant() function, triggering a recursive call and potentially bypassing the intended unbonding process or manipulating the state.

The actual behavior of the unbond_instant() function could be manipulated. An attacker could potentially exploit the reentrancy vulnerability to bypass the fee payment, manipulate the unbonded amount, or perform other unintended actions.

Impact

It may allow attackers to instant unbond their tokens without paying the required fee or to manipulate the unbonding process in their favor. This could lead to financial losses for the Acala platform and its users, as well as undermine the integrity and trust in the system.

Tools Used

Vs

Recommended Mitigation Steps

1. Using a ReentrancyGuard modifier or a similar mechanism to ensure that the function cannot be called recursively.

2. Move the fee transfer logic to the end of the unbond_instant() function, after all other state changes have been made. This reduces the window of opportunity for reentrancy attacks.

use frame_support::traits::ReentrancyGuard;

impl<T: Config> Currency<T::AccountId> for Pallet<T> {
    // ...

    fn withdraw(
        who: &T::AccountId,
        amount: Self::Balance,
        reasons: WithdrawReasons,
        liveness: ExistenceRequirement,
    ) -> Result<Self::NegativeImbalance, DispatchError> {
        let mut reentrancy_guard = ReentrancyGuard::new();
        reentrancy_guard.enter();

        // Perform the withdrawal logic
        // ...

        reentrancy_guard.exit();

        Ok(/* ... */)
    }

    // ...
}

I employ some methods which include ReentrancyGuard is used to prevent recursive calls to the withdraw function. The enter() method is called before performing the withdrawal logic, and the exit() method is called after the withdrawal is complete. If a recursive call is attempted, the ReentrancyGuard will prevent it from executing.

Assessed type

Reentrancy

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[DadeKuma]

Reentrancy is not possible by default on Substrate

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid