When `AddVote` occurs, the array may be out of bounds, causing panic

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/2834e3f85b2c7774e97413936018a0814c57d860/repos/node/x/observer/types/ballot.go#L27

Vulnerability details

Impact

During voting time, there may be a panic, or malicious nodes intentionally create panic to crash the chain.

Proof of Concept

The Ballot module returns -1 if the address of the ballot does not exist in the VoterList

func (m Ballot) GetVoterIndex(address string) int {
    index := -1
    for i, addr := range m.VoterList {
        if addr == address {
            return i
        }
    }
    return index
}

The HasVoted function gets the Vote status directly using m.votes[index] after getting index, but if index is -1, the array is out of bounds.

func (m Ballot) HasVoted(address string) bool {
    index := m.GetVoterIndex(address)
    return m.Votes[index] != VoteType_NotYetVoted
}

This is likely to happen anywhere voting is required. For example, VoteOnObservedInboundTx, the node votes on InboundTx, which occurs if the node's address is added to the trust list after the vote is created.

There are two cases as follows:

1. During the normal running of the network, if a new node is added to the network, if the new node runs before it is added to the trust list, after the node is added to the trust list, it detects InboundTx and starts to vote,however, the vote(Ballot) has already been created and the new address is not in the VoterList, a panic occurs.
2. In another case, a malicious node actively launches an attack. The attacker can exit the trust list first, then re-join, and then vote on the Ballot already created.

The function call process is as follows: VoteOnObservedInboundTx -> AddVoteToBallot -> AddVote -> HasVoted

    func (k msgServer) VoteOnObservedInboundTx(goCtx context.Context, msg *types.MsgVoteOnObservedInboundTx) (*types.MsgVoteOnObservedInboundTxResponse, error) {
        .....
        // IsAuthorized does various checks against the list of observer mappers
@>      if ok := k.zetaObserverKeeper.IsAuthorized(ctx, msg.Creator, observationChain); !ok {
            return nil, observerTypes.ErrNotAuthorizedPolicy
        }

        ballot, isNew, err := k.zetaObserverKeeper.FindBallot(ctx, index, observationChain, observationType)
        if err != nil {
            return nil, err
        }
        if isNew {
            observerKeeper.EmitEventBallotCreated(ctx, ballot, msg.InTxHash, observationChain.String())
        }
        // AddVoteToBallot adds a vote and sets the ballot
@>      ballot, err = k.zetaObserverKeeper.AddVoteToBallot(ctx, ballot, msg.Creator, observerTypes.VoteType_SuccessObservation)
        if err != nil {
            return nil, err
        }
        _, isFinalized := k.zetaObserverKeeper.CheckIfFinalizingVote(ctx, ballot)
        if !isFinalized {
            // Return nil here to add vote to ballot and commit state
            return &types.MsgVoteOnObservedInboundTxResponse{}, nil
        }
        ......
    }

    func (k Keeper) AddVoteToBallot(ctx sdk.Context, ballot types.Ballot, address string, observationType types.VoteType) (types.Ballot, error) {
@>      ballot, err := ballot.AddVote(address, observationType)
        if err != nil {
            return ballot, err
        }
        ctx.Logger().Info(fmt.Sprintf("Vote Added | Voter :%s, ballot idetifier %s", address, ballot.BallotIdentifier))
        k.SetBallot(ctx, &ballot)
        return ballot, err
    }

    func (m Ballot) AddVote(address string, vote VoteType) (Ballot, error) {
@>      if m.HasVoted(address) {
            return m, errors.Wrap(ErrUnableToAddVote, fmt.Sprintf(" Voter : %s | Ballot :%s | Already Voted", address, m.String()))
        }
        // `index` is the index of the `address` in the `VoterList`
        // `index` is used to set the vote in the `Votes` array
        index := m.GetVoterIndex(address)
        m.Votes[index] = vote
        return m, nil
    }
}

Tools Used

vscode manual

Recommended Mitigation Steps

func (m Ballot) HasVoted(address string) bool {
    index := m.GetVoterIndex(address)
+   if index == -1 { return false }
    return m.Votes[index] != VoteType_NotYetVoted
}

Assessed type

DoS

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-sponsor]

lumtis marked the issue as disagree with severity

[lumtis]

A panic in a message execution would not crash the chain, it would only make the message fail

[c4-sponsor]

lumtis (sponsor) disputed

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked issue #116 as primary and marked this issue as a duplicate of 116

[c4-judge]

0xean marked the issue as partial-25