J4X_ - Lockup of vestings or completion time can be bypassed due to missing check for staked tokens

[sherlock-admin3]

J4X_

Medium

Lockup of vestings or completion time can be bypassed due to missing check for staked tokens

Summary

The vesting module in the Andromeda protocol allows multiple vestings to be created. Currently restricted to the owner, it will be extended to any user. While tokens are vesting, they can be staked to earn rewards. However, the protocol does not account for the staked tokens when claiming vestings. This allows users to withdraw staked tokens, potentially circumventing the lockup period and withdrawing tokens from other vestings that are not yet matured. This issue results in the ability to bypass vesting schedules and access locked tokens prematurely.

Vulnerability Detail

The vesting module allows for the creation of multiple vestings. This is restricted to the owner for now, but it will be extended to anyone. The current version can be used to proof lockup periods & vesting schedules to users. This is done by the owner depositing tokens into the contract and setting parameters for the vesting. While the tokens are vesting, they can be staked to a delegator to earn rewards by calling the execute_delegate() function. The vestings are tracked using batch struct.

pub struct Batch {
    /// The amount of tokens in the batch
    pub amount: Uint128,
    /// The amount of tokens that have been claimed.
    pub amount_claimed: Uint128,
    /// When the lockup ends.
    pub lockup_end: u64,
    /// How often releases occur.
    pub release_unit: u64,
    /// Specifies how much is to be released after each `release_unit`. If
    /// it is a percentage, it would be the percentage of the original amount.
    pub release_amount: WithdrawalType,
    /// The time at which the last claim took place in seconds.
    pub last_claimed_release_time: u64,
}

The problem occurs because the batches do not account for how many of their tokens were staked. As a result, the recipient can still withdraw tokens from a vesting that is currently staked. This can be seen when looking at the function handling the claiming.

fn claim_batch(
    querier: &QuerierWrapper,
    env: &Env,
    batch: &mut Batch,
    config: &Config,
    number_of_claims: Option<u64>,
) -> Result<Uint128, ContractError> {
    let current_time = env.block.time.seconds();
    ensure!(
        batch.lockup_end <= current_time,
        ContractError::FundsAreLocked {}
    );
    let amount_per_claim = batch.release_amount.get_amount(batch.amount)?;

    let total_amount = AssetInfo::native(config.denom.to_owned())
        .query_balance(querier, env.contract.address.to_owned())?;

    let elapsed_time = current_time - batch.last_claimed_release_time;
    let num_available_claims = elapsed_time / batch.release_unit;

    let number_of_claims = cmp::min(
        number_of_claims.unwrap_or(num_available_claims),
        num_available_claims,
    );

    let amount_to_send = amount_per_claim * Uint128::from(number_of_claims);
    let amount_available = cmp::min(batch.amount - batch.amount_claimed, total_amount);

    let amount_to_send = cmp::min(amount_to_send, amount_available);

    // We dont want to update the last_claim_time when there are no funds to claim.
    if !amount_to_send.is_zero() {
        batch.amount_claimed += amount_to_send;
        batch.last_claimed_release_time += number_of_claims * batch.release_unit;
    }

    Ok(amount_to_send)
}

The vulnerability leads to further issues if multiple vestings exist. In that case, the user will actually be sent tokens from one of the other vestings, which are not currently staked. This is an issue as the other vesting from which the tokens will originate might still be in its lockup period, and the tokens should not be withdrawable.

Exemplary scenario

1. VestingA (100 tokens) gets created with a lockup_end in 1 month and full claiming after that
2. User stakes all 100 tokens
3. VestingB (100 tokens) with lockup_end in 10 years is added
4. One month passes, and VestingA matures
5. The user does not want to wait for the completion time when unstaking his tokens from VestingA, so he just calls to claim VestingA while they are still staked
6. As it is not checked which tokens are staked, the claim passes
7. The user has effectively bypassed the completion time/lockup period.

Impact

This issue allows the recipient to circumvent the lockup duration of his vestings by withdrawing the tokens through another staked vesting.

Code Snippet

Tool used

Manual Review

Recommendation

We recommend adding the parameter staked_tokens to the batch struct.

pub struct Batch {
    /// The amount of tokens in the batch
    pub amount: Uint128,
    /// The amount of tokens that have been claimed.
    pub amount_claimed: Uint128,
    /// The amount of tokens that have been staked.
    pub amount_staked: Uint128, // <--- New variable
    /// When the lockup ends.
    pub lockup_end: u64,
    /// How often releases occur.
    pub release_unit: u64,
    /// Specifies how much is to be released after each `release_unit`. If
    /// it is a percentage, it would be the percentage of the original amount.
    pub release_amount: WithdrawalType,
    /// The time at which the last claim took place in seconds.
    pub last_claimed_release_time: u64,
}

This variable should be updated on each call to executed_delegate() and execute_undelegate. When a user tries to withdraw funds from his batch, the function must check if amount - (amount_claimed + staked_tokens) >= tokens_to_withdraw.

fn claim_batch(
    querier: &QuerierWrapper,
    env: &Env,
    batch: &mut Batch,
    config: &Config,
    number_of_claims: Option<u64>,
) -> Result<Uint128, ContractError> {
    let current_time = env.block.time.seconds();
    ensure!(
        batch.lockup_end <= current_time,
        ContractError::FundsAreLocked {}
    );
    let amount_per_claim = batch.release_amount.get_amount(batch.amount)?;

    let total_amount = AssetInfo::native(config.denom.to_owned())
        .query_balance(querier, env.contract.address.to_owned())?;

    let elapsed_time = current_time - batch.last_claimed_release_time;
    let num_available_claims = elapsed_time / batch.release_unit;

    let number_of_claims = cmp::min(
        number_of_claims.unwrap_or(num_available_claims),
        num_available_claims,
    );

    let amount_to_send = amount_per_claim * Uint128::from(number_of_claims);
    let amount_available = cmp::min(batch.amount - (batch.amount_claimed + batch.amount_staked), total_amount); // <---- Changed LOC

    let amount_to_send = cmp::min(amount_to_send, amount_available);

    // We dont want to update the last_claim_time when there are no funds to claim.
    if !amount_to_send.is_zero() {
        batch.amount_claimed += amount_to_send;
        batch.last_claimed_release_time += number_of_claims * batch.release_unit;
    }

    Ok(amount_to_send)
}

[cu5t0mPeo]

escalate The owner is trustworthy, and there is no reason for the owner to bypass the lockup period. This is an input error on the part of the owner.

From the README description: Contract owner should be assumed trusted.

[sherlock-admin3]

escalate The owner is trustworthy, and there is no reason for the owner to bypass the lockup period. This is an input error on the part of the owner.

From the README description: Contract owner should be assumed trusted.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[J4X-98]

Hey,

The issue is that vestings are used as a way to restrict addresses from dumping tokens directly at the start and are intended to let tokens slowly vest over time. This is the only reason why they exist, as otherwise tokens can be directly distributed to the recipient. A user should never be able to take tokens from a vesting before its lockup-time, as it is an invariant that should never be broken. This issue clearly shows a case where this can occur. While the owner is trusted, we can assume that a owner vesting should not be claimable before the lockup time, as it is a security guarantee to the users of the protocol. That's why the lockup time was implemented in the end.

Additionally this functionality is intended to be used for recipients and the owner, where recipients can very well act maliciously.

[cvetanovv]

I disagree with the escalation, and @J4X-98 describes things very well. This issue has no relation to admin trust or admin mistakes.

Planning to reject the escalation and leave the issue as is.

[WangSecurity]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cu5t0mPeo: rejected

[gjaldon]

@cvetanovv @WangSecurity I would like to mention that this issue is similar to https://github.com/sherlock-audit/2024-05-andromeda-ado-judging/issues/55. I think it is invalid for the same reason which is that there is no damage to any party (recipient, owner, or protocol) because all Vesting batches belong to the same recipient.

ref: execute_claim() in Vesting ADO

    let amount_to_send = claim_batch(&deps.querier, &env, &mut batch, &config, number_of_claims)?;
    // ... snip ...
    // There is only one configured recipient for the Vesting ADO and all vesting batches go to that one recipient
    let withdraw_msg = config.recipient.generate_direct_msg(
        &deps.as_ref(),
        vec![Coin::new(amount_to_send.u128(), config.denom)],
    )?;

In the example scenario, both VestingA and VestingB belong to the same Recipient, so even if VestingA takes the tokens from VestingB, there is no damage. It's like the recipient is only stealing from themself. I think the intention here was to keep the Vesting ADO simple with just one recipient so there is no need to track which tokens belong to which vesting batch.

[J4X-98]

This comment, as well as the arguments of you on other issues clearly ignores the fact that the vestings have a lockup duration which is a security measure that should not be circumvented in any way. It is used to prevent users from dumps etc, so if a vesting exists but the lockup period does not work, the vesting itself does not serve any purpose.

If an issue shows a way to bypass this safeguard it becomes a medium, similar to bypassing a blacklist. This has been pointed out to you by me and the judge. You seem to be missing the point that a medium can also result from the breaking of functionality or security measures not just the stealing of funds.

Additionally you mention that #55 is invalid which is simply untrue as the issue has been validated by the judge, you just escalated against it.

[gjaldon]

@WangSecurity @cvetanovv This is a duplicate of #55 because:

1. The root cause is the same: funds are pooled. The issue happens in the same contract, Vesting ADO, and the same function claim_batch.
2. The impact is the same: bypass of lockup period of vesting.
3. The fix in this report will also fix #55. The check that will be added when claiming funds amount - (amount_claimed + staked_tokens) >= tokens_to_withdraw. will prevent VestingA from getting VestingB's funds in the example scenario in #55.

[J4X-98]

Escalations for this issue have already been resolved so it would be nice of you to respect the judges decision. Nevertheless I will answer your comment for the sake of completeness.

While the two issues are similar, the scenarios are not the same:

1. Tokens are slashed and this slashed amount is recouped from another vesting that is still in lockup. This other vesting can then never fully be withdrawn as the contract will not receive enough tokens.
2. The user stakes his tokens and then withdraws them which will pull them out of another locked up vesting.

[gjaldon]

Yes, the scenarios are not the same but the issues are.

Will the fix for #58 prevent the issue in #55?

[cvetanovv]

I agree with @gjaldon comment that #58 and #55 are duplicates.

Although there are differences between the two issues, the main rules we look at when deciding whether to duplicate them are the same.

The root cause is the same. Fixing one issue will fix the other. And the impact is the same. So I'll duplicate #58 and #55

[sherlock-admin2]

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

[bin2chen66]

fix-reviews note: https://github.com/andromedaprotocol/andromeda-core/pull/554 PR removed the stake logic This issue has been resolved