defsec - Pagination method fails to return complete pages for non-consecutive active topic IDs

[sherlock-admin2]

defsec

Medium

Pagination method fails to return complete pages for non-consecutive active topic IDs

Summary

The current implementation of GetIdsOfActiveTopics does not correctly handle cases where active topic IDs are non-consecutive, potentially returning incomplete pages of results.

Vulnerability Detail

The GetIdsOfActiveTopics function uses a range-based iteration approach that assumes topic IDs are consecutive. When there are gaps in the active topic ID sequence, this method may return fewer results than requested, even when more active topics exist.

For example, if topic IDs 1 and 3 are active, but 2 is not, a request for 2 items might only return ID 1, missing ID 3.

Example case :

func TestGetIdsOfActiveTopics(t *testing.T) {
    // Setup a mock keeper and context
    keeper, ctx := setupTestKeeper(t)

    // Set up active topics with non-consecutive IDs
    activeTopics := []TopicId{1, 3, 5, 7, 10}
    for _, topicId := range activeTopics {
        err := keeper.activeTopics.Set(ctx, topicId)
        require.NoError(t, err)
    }

    testCases := []struct {
        name           string
        pagination     *types.SimpleCursorPaginationRequest
        expectedTopics []TopicId
        expectedNext   bool
    }{
        {
            name:           "No pagination",
            pagination:     nil,
            expectedTopics: []TopicId{1, 3, 5, 7, 10},
            expectedNext:   false,
        },
        {
            name:           "Limit 2, no cursor",
            pagination:     &types.SimpleCursorPaginationRequest{Limit: 2},
            expectedTopics: []TopicId{1, 3},
            expectedNext:   true,
        },
        {
            name:           "Limit 3, cursor after 3",
            pagination:     &types.SimpleCursorPaginationRequest{Limit: 3, Key: makeKey(3)},
            expectedTopics: []TopicId{5, 7, 10},
            expectedNext:   false,
        },
        {
            name:           "Limit 2, cursor after 7",
            pagination:     &types.SimpleCursorPaginationRequest{Limit: 2, Key: makeKey(7)},
            expectedTopics: []TopicId{10},
            expectedNext:   false,
        },
    }

    for _, tc := range testCases {
        t.Run(tc.name, func(t *testing.T) {
            topics, res, err := keeper.GetIdsOfActiveTopics(ctx, tc.pagination)
            require.NoError(t, err)
            require.Equal(t, tc.expectedTopics, topics)
            if tc.expectedNext {
                require.NotNil(t, res.NextKey)
            } else {
                require.Nil(t, res.NextKey)
            }
        })
    }
}

func makeKey(id uint64) []byte {
    key := make([]byte, 8)
    binary.BigEndian.PutUint64(key, id)
    return key
}

func setupTestKeeper(t *testing.T) (*Keeper, context.Context) {
    // Create a mock keeper and context
    // This is a simplified setup and may need to be adjusted based on your actual Keeper structure
    keeper := &Keeper{
        activeTopics: collections.NewMap[TopicId, struct{}](nil, "active_topics"),
    }
    ctx := context.Background()
    return keeper, ctx
}

Impact

This issue can lead to:

1. Incomplete data retrieval for clients requesting active topics.
2. Inefficient pagination, requiring more requests than necessary to retrieve all active topics.
3. Potential inconsistencies in application logic that relies on complete pages of active topics.

Code Snippet

keeper.go#L1605

func (k Keeper) GetIdsOfActiveTopics(ctx context.Context, pagination *types.SimpleCursorPaginationRequest) ([]TopicId, *types.SimpleCursorPaginationResponse, error) {
    limit, start, err := k.CalcAppropriatePaginationForUint64Cursor(ctx, pagination)
    if err != nil {
        return nil, nil, err
    }

    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)
    if err != nil {
        return nil, nil, err
    }
    activeTopics, err := rng.Keys()
    if err != nil {
        return nil, nil, err
    }
    defer rng.Close()

    // If there are no topics, we return the nil for next key
    if activeTopics == nil {
        nextKey = make([]byte, 0)
    }

    return activeTopics, &types.SimpleCursorPaginationResponse{
        NextKey: nextKey,
    }, nil
}

Tool used

Manual Review

Recommendation

Modify the GetIdsOfActiveTopics function to ensure it returns the correct number of active topic IDs, regardless of gaps in the ID sequence. This could involve:

1. Implementing a cursor-based pagination system that tracks the last returned topic ID rather than using a fixed range.
2. Modifying the iteration logic to continue searching for active topics until the requested limit is reached or all topics have been checked.
3. Considering a different storage structure for active topics that allows for more efficient retrieval of non-consecutive IDs.

Here's a high-level pseudocode example of how the function could be modified:

func (k Keeper) GetIdsOfActiveTopics(ctx context.Context, pagination *types.SimpleCursorPaginationRequest) ([]TopicId, *types.SimpleCursorPaginationResponse, error) {
    limit, cursor, err := k.CalcAppropriatePaginationForUint64Cursor(ctx, pagination)
    if err != nil {
        return nil, nil, err
    }

    var activeTopics []TopicId
    var lastKey []byte

    iter, err := k.activeTopics.Iterate(ctx, collections.StartAfter(cursor))
    if err != nil {
        return nil, nil, err
    }
    defer iter.Close()

    for ; iter.Valid() && len(activeTopics) < int(limit); iter.Next() {
        topicId, err := iter.Key()
        if err != nil {
            return nil, nil, err
        }
        activeTopics = append(activeTopics, topicId)
        lastKey = iter.Key().Bytes()
    }

    var nextKey []byte
    if iter.Valid() {
        nextKey = lastKey
    }

    return activeTopics, &types.SimpleCursorPaginationResponse{
        NextKey: nextKey,
    }, nil
}

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

GetIdsOfActiveTopics incorrectly assumes that IDs of active topics are always sequential

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/allora-network/allora-chain/pull/406

[WangSecurity]

Based on the discussion under #115 planning to invalidate this report as it doesn’t have medium or high impact