J4X_ - Changes of the `UnbondingTime` are not accounted for

[sherlock-admin2]

J4X_

Medium

Changes of the UnbondingTime are not accounted for

Summary

The andromeda-validator-staking contract allows the owner to stake and unstake tokens, adding unstaking entries to an UNSTAKING_QUEUE. The unstaking process is dependent on the UnbondingTime parameter of the chain, which can be changed by governance. If the UnbondingTime is reduced while unstakings are already queued, it can result in a denial-of-service (DoS) situation where newer entries cannot be withdrawn until older entries expire. This could lead to tokens being stuck in the contract for a significant period.

Vulnerability Detail

The andromeda-validator-staking contract implements a way to allow the owner of the contract to stake tokens. When the owner of the contract wants to unstake tokens again he can do this by calling the execute_unstake() function. The contract will then, on response from the staking module, add an entry to the UNSTAKING_QUEUE.

pub fn on_validator_unstake(deps: DepsMut, msg: Reply) -> Result<Response, ContractError> {
    let attributes = &msg.result.unwrap().events[0].attributes;
    let mut fund = Coin::default();
    let mut payout_at = Timestamp::default();
    for attr in attributes {
        if attr.key == "amount" {
            fund = Coin::from_str(&attr.value).unwrap();
        } else if attr.key == "completion_time" {
            let completion_time = DateTime::parse_from_rfc3339(&attr.value).unwrap();
            let seconds = completion_time.timestamp() as u64;
            let nanos = completion_time.timestamp_subsec_nanos() as u64;
            payout_at = Timestamp::from_seconds(seconds);
            payout_at = payout_at.plus_nanos(nanos);
        }
    }
    UNSTAKING_QUEUE.push_back(deps.storage, &UnstakingTokens { fund, payout_at })?;

    Ok(Response::default())
}

Once the completion time has passed, the user can now call execute_withdraw_fund() to withdraw the funds. The function loops over the UNSTAKING_QUEUE and adds all unstakings until it fins one that has not expired. Afterwards all of the found expired unstakings are payed out to the user.

loop {
    match UNSTAKING_QUEUE.front(deps.storage).unwrap() {
        Some(UnstakingTokens { payout_at, .. }) if payout_at <= env.block.time => {
            if let Some(UnstakingTokens { fund, .. }) =
                UNSTAKING_QUEUE.pop_front(deps.storage)?
            {
                funds.push(fund)
            }
        }
        _ => break,
    }
}

The completion time that the loop is based upon is the UnbondingTime which is one of the params of the x/staking module. The governance can change this parameter at any time via a MsgUpdateParamsmessage.

The issue occurs as the loop expects that for each item in it $itemn.completionTime >= item{n-1}.completionTime$ . Unfortunately this is not the case if the governance reduces the UnbondingTimeparameter while unstakings are already queued. In that case it can occur that $itemn.completionTime < item{n-1}.completionTime$. This will result in $itemn$ being unable to withdraw until $item{n-1}$ has expired.

This DOS can be anwhere in the range from $0-UnbondingTime$. For most of the targeted chains the UnbondingTime is set to 21 days ( Incetive, Archway and Terra). While it is not a reasonable scenario that the UnbondingTime will be reduced to 0, a deduction of 1-2 weeks is possible. The default value of the UnbondingTime is only 3 days, and some other chains also user 1-2 weeks shorter unbonding times:

• Axelar - 7 days
• Bitcanna - 14 days
• Stargaze - 14 days

Based on this we can assume that a reduction by 1-2 weeks is a possible scenario. As the protocol will not only be deployed on andromeda's own chain but also on multiple other cosmos chains, the Andromeda Governance has no possibility to prevent such a change if it occurs.

Impact

The issue results in tokens getting stuck in the contract until the messages before them expire. This can result in a DOS of 1-2 weeks depending on the change of the UnbondingTime.

Code Snippet

https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/bbbf73e5d1e4092ab42ce1f827e33759308d3786/andromeda-core/contracts/finance/andromeda-validator-staking/src/contract.rs#L256-L267

Tool used

Manual Review

Recommendation

We recommend adapting the loop to not break if the payout_at > env.block.time. Instead in that case it should just do nothing and go on to the next element.

loop {
    match UNSTAKING_QUEUE.front(deps.storage).unwrap() {
        Some(UnstakingTokens { payout_at, .. }) if payout_at <= env.block.time => {
            if let Some(UnstakingTokens { fund, .. }) =
                UNSTAKING_QUEUE.pop_front(deps.storage)?
            {
                funds.push(fund)
            }
        }
        _ => continue,
    }
}

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/andromedaprotocol/andromeda-core/pull/551

[cowboy0015]

Updated the logic to removed all the expired unstaking requests. Thank you

[bin2chen66]

fix-reviews note: https://github.com/andromedaprotocol/andromeda-core/pull/551 This PR removes the logic of execute_withdraw_fund() to calculate the quantity, and directly withdraws all the balance, which solves the problem