abdulsamijay - Removing Negative amounts in Stake & DelegateStake leads to chain halt

[sherlock-admin2]

abdulsamijay

High

Removing Negative amounts in Stake & DelegateStake leads to chain halt

Summary

The functions RemoveStake & RemoveDelegateStake do not check whether the msg.Amount is positive before proceeding with the stake removal process. This oversight allows negative amounts to be scheduled, which causes a critical error during the EndBlocker execution, resulting in a chain halt.

Vulnerability Detail

The vulnerability stems from the lack of validation for positive amounts in the RemoveDelegateStake and RemoveStake functions located in the following lines.

• x/emissions/keeper/msgserver/msg_server_stake.go#L62

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

  - x/emissions/keeper/msgserver/msg_server_stake.go#L175

  func (ms msgServer) RemoveDelegateStake(ctx context.Context, msg *types.MsgRemoveDelegateStake) (*types.MsgRemoveDelegateStakeResponse, error) {
  if msg.Amount.IsZero() {
      return nil, types.ErrReceivedZeroAmount
  }
  // -- Snip -- 
  }

  When these transactions are processed, they are scheduled for removal after a delay specified by moduleParams.RemoveStakeDelayWindow. When the EndBlocker Runs after the scheduled time stakes gets removed & it runs the following code in [x/emissions/module/abci.go#L22-23](https://github.com/allora-network/allora-chain/blob/3a97afe7af027c96749fac7c4327ae85359a61c8/x/emissions/module/abci.go#L22-L23)

  func EndBlocker(ctx context.Context, am AppModule) error {
  sdkCtx := sdk.UnwrapSDKContext(ctx)
  blockHeight := sdkCtx.BlockHeight()
  sdkCtx.Logger().Debug(
      fmt.Sprintf("\n ---------------- Emissions EndBlock %d ------------------- \n",
          blockHeight))
  
  // Remove Stakers that have been wanting to unstake this block. They no longer get paid rewards
  RemoveStakes(sdkCtx, blockHeight, am.keeper)
  RemoveDelegateStakes(sdkCtx, blockHeight, am.keeper)
  // Existing Code ...
  }

  This leads to calling SendCoinsFromModuleToAccount with a negative amount leading to panic.

  func RemoveStakes(
  sdkCtx sdk.Context,
  currentBlock int64,
  k emissionskeeper.Keeper,
  ) {
  // Existing Code
  for _, stakeRemoval := range removals {
  
      coins := sdk.NewCoins(sdk.NewCoin(chainParams.DefaultBondDenom, stakeRemoval.Amount))
      err = k.SendCoinsFromModuleToAccount(sdkCtx, emissionstypes.AlloraStakingAccountName, stakeRemoval.Reputer, coins)
  }
   // Existing Code ...
  }

  func RemoveDelegateStakes(
  sdkCtx sdk.Context,
  currentBlock int64,
  k emissionskeeper.Keeper,
  ) {
  // Existing Code
  for _, stakeRemoval := range removals {
  
      coins := sdk.NewCoins(sdk.NewCoin(chainParams.DefaultBondDenom, stakeRemoval.Amount))
      err = k.SendCoinsFromModuleToAccount(sdkCtx, emissionstypes.AlloraStakingAccountName, stakeRemoval.Delegator, coins)
  }
   // Existing Code ...
  }

Impact

Here is the simplified steps how an attacker can cause a chain halt.

1. Alice creates a topic with id 1
2. Alice registers herself as a reputer in the system.
3. Alice adds stakes for a topic with id 1.
4. Alice calls remove-stake from the topic with negative amount. Now the removal is scheduled for moduleParams.RemoveStakeDelayWindow. When the delayedWindow has passed EndBlocker will call RemoveStake function & the chain will panic.

The similar attack vector applies with the delegator who wishes to delegate to a reputer.

1. Bob is a delegator who delegates stake to Alice.
2. Bob removes delegated stake from the reputer with negative amount. After the delay, the chain halts.

Code Snippet

Tool used

Manual Review

Recommendation

To prevent this issue, add a validation check to ensure that the msg.Amount is positive in both the RemoveDelegateStake and RemoveStake functions.

func (ms msgServer) RemoveDelegateStake(ctx context.Context, msg *types.MsgRemoveDelegateStake) (*types.MsgRemoveDelegateStakeResponse, error) {
    if msg.Amount.IsZero() {
        return nil, types.ErrReceivedZeroAmount
    }

    // Add check for positive amount
    if msg.Amount.IsNegative() {
        return nil, types.ErrNegativeAmount
    }

    // Existing code...
}

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

    // Add check for positive amount
    if msg.Amount.IsNegative() {
        return nil, types.ErrNegativeAmount
    }

    // Existing code...
}

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