Active topics eligible for churning may be skipped due to pagination assuming consecutive topic ids

Summary

Active topics eligible for churning may be skipped due to pagination assuming consecutive topic ids in GetIdsOfActiveTopics.

Vulnerability Detail

In the emissions module EndBlocker, the possible topics for churning are initially identified in GetAndUpdateActiveTopicWeights. Topics are only considered (added to weights) if the current block is the end of an epoch and their topic weight is at least the minimum weight. In SafeApplyFuncOnAllActiveEpochEndingTopics, the list of topics that is considered (ie. have fn applied to them) is retrieved using pagination in GetIdsOfActiveTopics. Note that the initial starting key is always for topic id 0 and topicPageLimit = maxTopicPages = moduleParams.DefaultPageLimit, and we continue until either a returned page is empty or we reach maxTopicPages. https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/module/rewards/topic_rewards.go#L61-L94

    topicPageKey := make([]byte, 0)
    i := uint64(0)
    for {
        topicPageRequest := &types.SimpleCursorPaginationRequest{Limit: topicPageLimit, Key: topicPageKey}
        topicsActive, topicPageResponse, err := k.GetIdsOfActiveTopics(ctx, topicPageRequest)
        ...
        for _, topicId := range topicsActive {
            ...
            if k.CheckCadence(block, topic) {
                // All checks passed => Apply function on the topic
                err = fn(ctx, &topic)
                if err != nil {
                    Logger(ctx).Warn(fmt.Sprintf("Error applying function on topic: %s", err.Error()))
                    continue
                }
            }
        }

        // if pageResponse.NextKey is empty then we have reached the end of the list
        if topicsActive == nil || i > maxTopicPages {
            break
        }
        topicPageKey = topicPageResponse.NextKey
        i++
    }

The issue is GetIdsOfActiveTopics assumes that active topics will be consecutive as nextKey (the end key for this page exclusive) is start + limit e.g. for the first page where startKey = 0, endKey = limit. https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/keeper.go#L1605-L1634

func (k Keeper) GetIdsOfActiveTopics(ctx context.Context, pagination *types.SimpleCursorPaginationRequest) ([]TopicId, *types.SimpleCursorPaginationResponse, error) {
    limit, start, err := k.CalcAppropriatePaginationForUint64Cursor(ctx, pagination)
    ...
    startKey := make([]byte, binary.MaxVarintLen64)
    binary.BigEndian.PutUint64(startKey, start)
    nextKey := make([]byte, binary.MaxVarintLen64)
    binary.BigEndian.PutUint64(nextKey, start+limit)

    rng, err := k.activeTopics.IterateRaw(ctx, startKey, nextKey, collections.OrderAscending)
    ...
    activeTopics, err := rng.Keys()
    ...
    return activeTopics, &types.SimpleCursorPaginationResponse{
        NextKey: nextKey,
    }, nil
}

Consequently, GetIdsOfActiveTopics as used in SafeApplyFuncOnAllActiveEpochEndingTopics will only ever check the consecutive topic ids up to maxTopicPages * topicPageLimit ie. any topics with id greater than this will never be churned. Additionally, if in this range there exists at least one page of consecutive topic ids where none are active, the loop in SafeApplyFuncOnAllActiveEpochsEndingTopics will terminate (since and active topics with larger ids will not be considered (added to weights) or churned despite potentially having greater weight.

Impact

Topics with large enough id will never be churned, and topics may not be churned and rewarded according to their topic weight (which would negatively impact the incentive structure and supply of inferences relative to demand).

Code Snippet

https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/module/rewards/topic_rewards.go#L61-L94 https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/keeper.go#L1605-L1634

Tool used

Manual Review

Recommendation

Iterate with a limit of elements retrieved set to limit (and nil end key, so we don't skip anything).

Duplicate of #91

sherlock-audit / 2024-06-allora-judging

Kow - Active topics eligible for churning may be skipped due to pagination assuming consecutive topic ids #73