abdulsamijay - Negative Stake & DelegateStek Amounts Causes Runtime Panic

[sherlock-admin2]

abdulsamijay

Medium

Negative Stake & DelegateStek Amounts Causes Runtime Panic

Summary

The Allora AppChain's staking mechanism allows for negative stake amounts to be processed. This issue arises from the absence of checks for negative values in the AddStake and AddDelegateStake functions. When a negative stake amount is provided, the system panics at runtime, this might lead to an unexpected behavior

Vulnerability Detail

n the Allora AppChain, the functions AddStake and AddDelegateStake are responsible for adding stakes to become a reputer and delegating stakes to a reputer, respectively. Both functions include checks to ensure that the amount is not zero. However, there is no check to prevent negative values from being processed. This oversight can lead to runtime panics when a negative stake amount is provided, temporarily disrupting the normal operation of the network.

• https://github.com/allora-network/allora-chain/blob/3a97afe7af027c96749fac7c4327ae85359a61c8/x/emissions/keeper/msgserver/msg_server_stake.go#L17-L20
• https://github.com/allora-network/allora-chain/blob/3a97afe7af027c96749fac7c4327ae85359a61c8/x/emissions/keeper/msgserver/msg_server_stake.go#L136-L138

  
  // Function for reputers to call to add stake to an existing stake position.
  func (ms msgServer) AddStake(ctx context.Context, msg *types.MsgAddStake) (*types.MsgAddStakeResponse, error) {
  if msg.Amount.IsZero() {
      return nil, types.ErrReceivedZeroAmount
  }

// Delegates a stake to a reputer. Sender need not be registered to delegate stake. func (ms msgServer) DelegateStake(ctx context.Context, msg types.MsgDelegateStake) (types.MsgDelegateStakeResponse, error) { if msg.Amount.IsZero() { return nil, types.ErrReceivedZeroAmount }

## Impact
Leads to a runtime panic which may cause unexpected or undefined behaviour.
<img width="1317" alt="image" src="https://github.com/user-attachments/assets/8677aaca-017f-44ff-8234-28847c19b370">

## Tool used
Manual Review

## Recommendation
It is recommended to implement a check for negative stake amounts in both the `AddStake` and `AddDelegateStake` functions.

[sherlock-admin4]

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

[imsrybr0]

Escalate

This is not a duplicate of #21 and should be invalid as it will not halt the chain.

The panic will be handled by the RunTx recovery mechanism and the transaction will just fail for the user sending an incorrect input without any impact on the chain or other users.

[sherlock-admin3]

Escalate

This is not a duplicate of #21 and should be invalid as it will not halt the chain.

The panic will be handled by the RunTx recovery mechanism and the transaction will just fail for the user sending an incorrect input without any impact on the chain or other users.

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.

[abdulsamijay]

The panic will be handled by the RunTx recovery mechanism and the transaction will just fail for the user sending an incorrect input without any impact on the chain or other users.

Hi, I agree that the RunTx recovery mechanism will cause the transaction to fail for the user sending incorrect input, but I believe this should still be considered a valid issue as it indicates a mishandled bug.

I was thinking more about the potential for different users continuously sending invalid amounts, which could cause runtime panics for a validator node. This might lead to performance issues, block delays, or even undefined behavior therefore I assigned this as medium.

Regarding the final severity, I leave it the the judges

[WangSecurity]

I agree that the report doesn't identify medium severity impact. Hence, should be invalid based on the current duplication rules:

The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate. Identify the root cause Identify at least a Medium impact Identify a valid attack path or vulnerability path Fulfills other submission quality requirements (e.g. provides a PoC for categories that require one) Only when the "potential duplicate" meets all three requirements will the "potential duplicate" be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates.

Planning to accept the escalation and invalidate this report.

[mystery0x]

Escalate

This is not a duplicate of #21 and should be invalid as it will not halt the chain.

The panic will be handled by the RunTx recovery mechanism and the transaction will just fail for the user sending an incorrect input without any impact on the chain or other users.

I agree that panic will be handled by the recovery mechanism. I would consider this issue invalid since there is no longer any impact.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• imsrybr0: accepted