H-4 Order of EVM Events within a Block not resprected

[Lambda6080604052]

Description and context

In keeper.go, there is the function ProcessStakingEvents that iterates over all events which are emitted from the staking event (in one block):

story/client/x/evmstaking/keeper/keeper.go

Line 111 in 46dabe3

func (k Keeper) ProcessStakingEvents(ctx context.Context, height uint64, logs []*evmenginetypes.EVMEvent) error {

However, this iteration is not performed in the order of emission for these events. The following code enforces a particular order for prevPayloadEvents:

    // Collect local view of the evm logs from the previous payload.
    evmEvents, err := s.evmEvents(ctx, payload.ParentHash)
    if err != nil {
        return nil, errors.Wrap(err, "prepare evm event logs")
    }

    // Ensure the proposed evm event logs are equal to the local view.
    if err := evmEventsEqual(evmEvents, msg.PrevPayloadEvents); err != nil {
        return nil, errors.Wrap(err, "verify prev payload events")
    }

They need to have the same order as the function evmEvents returns, which is sorted by Address > Topics > Data:

    // Sort by Address > Topics > Data
    // This avoids dependency on runtime ordering.
    sort.Slice(events, func(i, j int) bool {
        if cmp := bytes.Compare(events[i].Address, events[j].Address); cmp != 0 {
            return cmp < 0
        }

        topicI := slices.Concat(events[i].Topics...)
        topicJ := slices.Concat(events[j].Topics...)
        if cmp := bytes.Compare(topicI, topicJ); cmp != 0 {
            return cmp < 0
        }

        return bytes.Compare(events[i].Data, events[j].Data) < 0
    })

This is problematic because the order is important for some events. For instance, if you submit three calls addOperator(A), removeOperator(A), addOperator(B) (in this order). You would expect a final result of operator B being added. If they are in the same block, this is not guaranteed and everything can happen based on the order of their topics and then the order of the data. In practice (because the distinguishing factor will be topic[0], i.e. the hash of the event definition), all addOperator or removeOperator calls would be processed first and then the other ones, which is not the expected behavior for the previously mentioned case.

Solution recommendation

Consider the log index when sorting (this is returned by k.engineCl.FilterLogs, which is called already, so should be easy to implement).

[corverroos]

Nice catch, I think this is a good solution to the problem