search

sherlock-audit / 2024-05-andromeda-ado-judging

1 stars 0 forks source link

J4X_ - Slashing allows users to bypass the lockup period of vestings #55

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 3 months ago

J4X_

Medium

Slashing allows users to bypass the lockup period of vestings

Summary

The vesting module allows owners to create and manage multiple vestings, which can be claimed independently. The vestings are tracked using the Batch structure. However, if tokens are staked and the chosen validator is slashed, the protocol does not adjust the amount parameter in the Batch structure. This oversight allows users to claim the original total amount, even if some tokens were slashed, potentially leading to bypassing the lockup period of other vestings.

Vulnerability Detail

The vesting module allows the owner to create multiple vestings, which should be claimable independently of each other. The vestings are tracked using the Batch structure.

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,
}

This struct's amount parameter tracks the total vesting. The amount_claimed increases on every claim until it reaches the amount. After that, no more tokens can be claimed.

The protocol allows the owner/user to stake the tokens while they are vesting. By doing this, users can gain staking rewards while their vesting matures. The rewards can be claimed through the execute_withdraw_rewards().

The problem is that the chosen validator can be slashed while the tokens are staked. If the selected validator is slashed, the tokens delegated to him will also get slashed. The protocol does not account for this, as even if the tokens are slashed, the amount of the vesting stays the same. As a result, the user, even after slashing, can claim the total amount of the batch. This is fine if only one batch is used, but it leads to issues if multiple batches are used. The slashed user can claim his total amount if multiple batches are used. The difference between his slashed amount and the actual amount will be taken from one of the other batches. This leads to issues if the other issue that the tokens are taken from is still in its lockup period, as this way, tokens will be taken from it before the lockup_end has been reached.

Exemplary Scenario

To showcase this issue, we can use a simple example.

  1. A vesting (VestingA) of 100 tokens is generated
  2. The user stakes the tokens of VestingA
  3. Another 100 token vesting batch (VestingB), which is locked for the next 10 years, gets added
  4. The staked tokens get slashed by 20%
  5. The user unstakes all his tokens again
  6. VestingA matures
  7. The user claims the full 100 tokens
  8. There are now only 80 tokens left for VestingB

In this case, the user can recoup his slashing losses from a vesting still in lockup. This should never be possible.

Impact

The issue results in a slashed user being able to funnel funds from a locked vesting into an unlocked vesting to recoup slashing losses.

Code Snippet

https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/bbbf73e5d1e4092ab42ce1f827e33759308d3786/andromeda-core/contracts/finance/andromeda-vesting/src/state.rs#L14-L28

Tool used

Manual Review

Recommendation

We recommend adapting the amount parameter of the Batch struct if the un-staked tokens are less than the staked ones. This mitigation is crucial as it ensures accurate accounting for slashing, thereby preventing the potential loss of tokens.

Duplicate of #58

gjaldon commented 3 months ago

Escalate

This is invalid since there is one recipient set for the Vesting ADO Contract, which means there is no damage/losses. If each vested batch belongs to a different recipient this would be valid. However, the Vesting ADO only allows to set one recipient for all vested batches. The recipient would only claim funds earlier for one batch and later for another.

sherlock-admin3 commented 3 months ago

Escalate

This is invalid since there is one recipient set for the Vesting ADO Contract, which means there is no damage/losses. If each vested batch belongs to a different recipient this would be valid. However, the Vesting ADO only allows to set one recipient for all vested batches. The recipient would only claim funds earlier for one batch and later for another.

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 commented 3 months ago

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.

You are correct that there are no losses, this is why this was handed in as a medium, not a high.

gjaldon commented 3 months ago

There is no loss of funds and functionality because all vesting batches belong to the same recipient. Whether the funds are funneled from one batch to another has no impact. This can only be valid if there were multiple different recipients and funneling funds from one batch to another leads to a loss of funds.

J4X-98 commented 3 months ago

There indeed is a loss of functionality. The lockup period is bypassed which is one of the safeguards of the vesting contract. There is no loss of funds which is correct, that's why it is a medium and not a high.

gjaldon commented 3 months ago

There is no loss of funds or core functionality. All the vested funds belong to the same recipient anyway, so it does not matter if funds are funneled from a locked batch to an unlocked batch.

J4X-98 commented 3 months ago

If you don't identify breaking a vestings lockup as breaking the intended functionality I don't think arguing with you makes sense. I'll leave the decision to the judge.

cvetanovv commented 2 months ago

Although we have no loss of funds, breaking a vestings lockup is a loss of functionality, which is Medium, according to Sherlock's documentation.

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

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

gjaldon commented 2 months ago

Vested funds for all batches are POOLED in the contract. Staking will get funds from the pooled funds of all batches in the contract and does not record which batch the staked tokens come from. Claiming will also get the claim amount from the pooled funds.

This report states that the lockup_end for VestingB is bypassed in the example scenario. How is it bypassed when VestingA only claimed from the pooled funds in the contract? Since the funds are pooled, these funds are not exclusive to VestingB. VestingB's amount_claimed and last_claimed_release_time were also not updated, so VestingB's funds were not deducted.

There is no bypass of the lockup period since:

Since there is no loss of funds and no loss of functionality and no damage to any party (protocol, recipient, or owner), I think this report is informational and only describes a Design Decision. Pooling all the vested funds is a design decision and that is what leads to the behavior that is described as a vulnerability in this report.

Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational.

J4X-98 commented 2 months ago

The problem you miss is that in the described scenario tokens from vesting A are staked and should not be withdrawable (they might have a lockup period of months on the staking). In return the user effectively uses tokens from a still locked up batch. This issue is indeed resulting out of the pooling as you mention. What you are missing is that the pooling is the root issue I describe, which is why the mitigation stops the pooling and separates the tokens per vesting. The pooling is the root cause of the issue and not a fixated thing that can't be changed, which is what you describe it as.

"There is no documentation that states that one batch can not claim funds from another batch." -> This is behavior that should not occur on any vesting.

cvetanovv commented 2 months ago

I agree with @J4X-98 last comment. Therefore, my decision to reject the escalation remains.

gjaldon commented 2 months ago

@cvetanovv can the sponsor be asked whether this is an issue?

gjaldon commented 2 months ago

Pooling of funds is not an issue because all batches are owned by 1 recipient anyway.

J4X-98 commented 2 months ago

Pooling of funds is not an issue because all batches are owned by 1 recipient anyway.

I don't know how often I should explain this. It is not an issue of lost funds between different recipients, so it is not high. It indeed is an issue of safety measures (unstaking period, lockup period) being bypassed by using funds from another (locked) vesting. This occurs due to the funds not being separated between vestings. That's why it is a valid medium.

cvetanovv commented 2 months ago

Again, I agree with @J4X-98 . Just because there is no loss of funds doesn't mean it should be invalid.

My final decision is to reject the escalation and leave this issue a valid Medium.

gjaldon commented 2 months ago

@cvetanovv there seems to be a misunderstanding.

I wasn't disputing that the issue is invalid because there is no loss of funds. I stated that the issue is a design decision made by the protocol and they decided on pooling funds because there is only 1 recipient anyway.

Since it is unclear whether this issue was a design decision or not it seems to make sense to consult the sponsor. I understand if that is no longer possible. Just thought maybe it was possible considering the sponsor was being asked anyway about this issue.

J4X-98 commented 2 months ago

You are right that the protocol implemented pooling of funds. But this decision lead to vulnerabilities due to missing to check for certain security assumptions in the development process. Just because the protocol decided on implementing a feature, not all vulnerabilities related to the feature are invalid.

Let's say the protocol implemented a DEX, and their DEX would be vulnerable to slippage because they forget to implement a minAmountOut. Based on your reasoning the slippage issue would be invalid because the protocol implemented a DEX knowingly, although they just forgot to think about sandwich attacks. If we take this reasoning one step further, all vulnerabilities are pretty much invalid because every vulnerability results out of a project decision that forgot some security property.

cvetanovv commented 2 months ago

To be clear and for the Head of Judge to resolve this escalation, my last decision remains to reject the escalation and leave this issue a valid Medium.

WangSecurity commented 2 months ago

Result: Medium Unique

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status: