0x3b - Actors can game the withdraw waiting time

[sherlock-admin4]

0x3b

Medium

Actors can game the withdraw waiting time

Summary

Actors can keep a withdrawal request scheduled all the time, which enables them to cut the withdrawal waiting time in half (or more).

Vulnerability Detail

As the README explains, participants who have scheduled a withdrawal still fully participate in the system.

When stake is withdrawn, it still has the effect of being active so the consequences of placing stake there are fully felt. You can cancel a stake withdrawal at any time.

This is actually implemented in the code, as RemoveStake (inside msg_server_stake) only records the removal and RemoveStakes (inside stake_removals) does the system changes.

This enables users to always have a withdrawal scheduled and cancel it just before it gets executed if they want to stay, and let it execute if they want to leave. This will cut the waiting period significantly. Here are two examples:

1. User1 has a withdrawal request running constantly. He cancels it just before it gets executed and makes a new one.
2. User2 is staking normally and makes the withdrawal request when he wants to leave.

At time T, both users decide to leave because of some event, either inside or outside the system. User1 will already have his withdrawal request at N% completed (where N is between 0% and 100%, average 50%), and User2 will have his withdrawal request at 0% (he just made one).

Impact

Users can game the withdrawal system by having a withdrawal constantly scheduled.

Code Snippet

//@audit The user can still act on the system, like he never made a withdraw request
func (ms msgServer) RemoveStake(ctx context.Context, msg *types.MsgRemoveStake) (*types.MsgRemoveStakeResponse, error) {
    if msg.Amount.IsZero() {
        return nil, types.ErrReceivedZeroAmount
    }

    stakePlaced, err := ms.k.GetStakeReputerAuthority(ctx, msg.TopicId, msg.Sender)
    if err != nil {
        return nil, err
    }

    delegateStakeUponReputerInTopic, err := ms.k.GetDelegateStakeUponReputer(ctx, msg.TopicId, msg.Sender)
    if err != nil {
        return nil, err
    }

    reputerStakeInTopicWithoutDelegateStake := stakePlaced.Sub(delegateStakeUponReputerInTopic)
    if msg.Amount.GT(reputerStakeInTopicWithoutDelegateStake) {
        return nil, types.ErrInsufficientStakeToRemove
    }

    moduleParams, err := ms.k.GetParams(ctx)
    if err != nil {
        return nil, err
    }

    sdkCtx := sdk.UnwrapSDKContext(ctx)
    removal, found, err := ms.k.GetStakeRemovalForReputerAndTopicId(sdkCtx, msg.Sender, msg.TopicId)
    if err != nil {
        return nil, errorsmod.Wrap(err, "error while searching previous stake removal")
    }

    if found {
        err = ms.k.DeleteStakeRemoval(ctx, removal.BlockRemovalCompleted, removal.TopicId, removal.Reputer)
        if err != nil {
            return nil, errorsmod.Wrap(err, "failed to delete previous stake removal")
        }
    }

    stakeToRemove := types.StakeRemovalInfo{
        BlockRemovalStarted:   sdkCtx.BlockHeight(),
        BlockRemovalCompleted: sdkCtx.BlockHeight() + moduleParams.RemoveStakeDelayWindow,
        TopicId:               msg.TopicId,
        Reputer:               msg.Sender,
        Amount:                msg.Amount,
    }

    err = ms.k.SetStakeRemoval(ctx, stakeToRemove)
    if err != nil {
        return nil, err
    }
    return &types.MsgRemoveStakeResponse{}, nil
}

Tool Used

Manual Review

Recommendation

This is an issue in many protocols with withdrawal queues. A general solution is to keep their commitment (the stake can be impacted), but remove their rights (they can't act on the system).

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/allora-network/allora-chain/pull/362

[imsrybr0]

Escalate

I don't think this is an issue as it seems to be working as designed.

Additionally, the PR mentioned is from the 25th of June.

[sherlock-admin3]

Escalate

I don't think this is an issue as it seems to be working as designed.

Additionally, the PR mentioned is from the 25th of June.

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.

[0x3b33]

I don't think this is an issue as it seems to be working as designed.

It's not desired to trick the system into having a way out before other users. Issues regarding sceduling withdraws to bypass/lower withdraw windows are common in contests. In the above example it's clearly shown how sceduling such withraws can gain you advantage when it comes to withdraws.

[mystery0x]

Escalate

I don't think this is an issue as it seems to be working as designed.

Additionally, the PR mentioned is from the 25th of June.

I don't think this is desired behavior, as it allows a user to game the withdrawal system by having a withdrawal perpetually scheduled.

[imsrybr0]

Hi @mystery0x, allowing the participants to still be active even if they have a stake removal in progress and to have the ability to cancel it any time they want is a design choice as outlined in the README's Q: Please discuss any design choices you made. section.

[0x3b33]

The README doesn't clearly state what's desired behavior and what's not.

It's desired that users can schedule withdraws and cancel them if need be, but besides that it doesn't cover the above case. It's not desired for users to lower or even skip the withdraw queue. Given the fact that users are still treated normally -- can perform actions/earn rewards -- then nothing would prevent a user from employing such tactic to lower/skip the waiting period of the queue.

[imsrybr0]

Hi @0x3b33,

The users are not lowering or skipping withdrawals queue, the cooldown period is still enforced for each withdrawal and they are just using the provided functionality as it was designed.

For reference, please check the judging criteria docs :

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.

User experience and design improvement issues: Issues that cause minor inconvenience to users where there is no material loss of funds are not considered valid. Funds are temporarily stuck and can be recovered by the administrator or owner. Also, if a submission is a design opinion/suggestion without any clear indications of loss of funds is not a valid issue.

[0x3b33]

Hey @imsrybr0 Thanks for quick reply, really appreciate it.

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.

I agree it's a design decision, but it's more than suboptimal, given that users would be able to schedule their withdraws constantly and cancel them before expiration they would be able to lower the impact of our withdraw queue - a core component of the system. I think the above report gave a clear description of how that is done and how it impacts the system.

Here is a similar medium issue - 162, that explained how a design decision impacts the system.

[WangSecurity]

I see the confusion here and how it may seem intended. Before we get to it, I want to clarify three things:

1. The example from Mellow I believe is irrelevant. Design decisions are not sources of truth, moreover, it's not about bypassing the withdrawal queue and it leads to a loss of funds. This issue is just bypassing the withdrawal queue (not actually bypassing technically). So they're not similar.
2. Does the user execute the withdrawal themselves or it's done by a trusted operator, i.e. does the user schedule a withdrawal and then withdraw themselves when the delay is over, or do they schedule a withdrawal and then an operator executes it after a delay?
3. There's no loss of funds and it's just a "broken" functionality?

[0x3b33]

3. There's no loss of funds and it's just a "broken" functionality?

No direct loss of funds, just broken core functionality.

2. Does the user execute the withdrawal themselves or it's done by a trusted operator, i.e. does the user schedule a withdrawal and then withdraw themselves when the delay is over, or do they schedule a withdrawal and then an operator executes it after a delay?

Withdraws are scheduled executed automatically when they reach their end. However during that schedule users are treated as active, as if they haven't scheduled a withdraw. They would be able to impact the system (upload losses) and get rewards.

In a sense the system doesn't differentiate between a reputer who has scheduled 100% of his stake to be withdrawn and one that hasn't.

[WangSecurity]

Thank you for these clarifications, but in that case, I would say it's the design of the protocol with a suboptimal tradeoff. Hence, since it doesn't cause a loss of funds, I believe it should be low severity:

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

Planning to accept the escalation and invalidate the report

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• imsrybr0: accepted