volodya - any user can halt chain with a negative amount request using RemoveStake

[sherlock-admin2]

volodya

High

any user can halt chain with a negative amount request using RemoveStake

Summary

any user can halt chain with a negative amount request using RemoveStake

Vulnerability Detail

Any staker can call RemoveStake with a negative Amount variable, request will go through and when unstake will happen -> leads to a chain halt

func (ms msgServer) RemoveStake(ctx context.Context, msg *types.MsgRemoveStake) (*types.MsgRemoveStakeResponse, error) {
    if msg.Amount.IsZero() {
        return nil, types.ErrReceivedZeroAmount
    }

    // Check the sender has enough stake already placed on the topic to remove the stake
    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)
    // find out if we have a stake removal in progress, if so overwrite it
    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,
    }

    // If no errors have occurred and the removal is valid, add the stake removal to the delayed queue
    err = ms.k.SetStakeRemoval(ctx, stakeToRemove)
    if err != nil {
        return nil, err
    }
    return &types.MsgRemoveStakeResponse{}, nil
}

x/emissions/keeper/msgserver/msg_server_stake.go#L61 EndBlocker -> RemoveStakes( NewCoin panics when there is negative values leading to a halt of blockchain

        coins := sdk.NewCoins(sdk.NewCoin(chainParams.DefaultBondDenom, stakeRemoval.Amount)) //@audit panic
        err = k.SendCoinsFromModuleToAccount(sdkCtx, emissionstypes.AlloraStakingAccountName, stakeRemoval.Reputer, coins)
        if err != nil {
            sdkCtx.Logger().Error(fmt.Sprintf(
                "Error removing stake: %v | %v",
                stakeRemoval,
                err,
            ))
            continue
        }

module/stake_removals.go#L35

func NewCoin(denom string, amount math.Int) Coin {
    coin := Coin{
        Denom:  denom,
        Amount: amount,
    }

    if err := coin.Validate(); err != nil {
        panic(err)
    }

    return coin
}

types/coin.go#L27

Impact

Code Snippet

Tool used

Manual Review

Recommendation

Add validation

func (ms msgServer) RemoveStake(ctx context.Context, msg *types.MsgRemoveStake) (*types.MsgRemoveStakeResponse, error) {
    if msg.Amount.IsZero() {
        return nil, types.ErrReceivedZeroAmount
    }

+   if msg.Amount.IsNegative() {
+       return nil, types.ErrReceivedNegativeAmount
+   }

Duplicate of #21

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Negative value in stake-specific functions causes panic() which permanently halts chain

[sherlock-admin2]

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