0x416 - the decay rate can be applied multiple times

[sherlock-admin2]

0x416

Medium

the decay rate can be applied multiple times

Summary

the decay rate can be applied multiple times

Vulnerability Detail

// Apply a function on all active topics that also have an epoch ending at this block
// Active topics have more than a globally-set minimum weight, a function of revenue and stake
// "Safe" because bounded by max number of pages and apply running, online operations.
func SafeApplyFuncOnAllActiveEpochEndingTopics(
    ctx sdk.Context,
    k keeper.Keeper,
    block BlockHeight,
    fn func(sdkCtx sdk.Context, topic *types.Topic) error,
    topicPageLimit uint64,
    maxTopicPages uint64,
) error {
    topicPageKey := make([]byte, 0)
    i := uint64(0)
    for {
        topicPageRequest := &types.SimpleCursorPaginationRequest{Limit: topicPageLimit, Key: topicPageKey}
        topicsActive, topicPageResponse, err := k.GetIdsOfActiveTopics(ctx, topicPageRequest)
        if err != nil {
            Logger(ctx).Warn(fmt.Sprintf("Error getting ids of active topics: %s", err.Error()))
            continue
        }

        for _, topicId := range topicsActive {
            topic, err := k.GetTopic(ctx, topicId)
            if err != nil {
                Logger(ctx).Warn(fmt.Sprintf("Error getting topic: %s", err.Error()))
                continue
            }

            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++
    }
    return nil
}

as the function comments suggests,

Apply a function on all active topics that also have an epoch ending at this block

the problem is that if the

    topicsActive, topicPageResponse, err := k.GetIdsOfActiveTopics(ctx, topicPageRequest)
        if err != nil {
            Logger(ctx).Warn(fmt.Sprintf("Error getting ids of active topics: %s", err.Error()))
            continue
        }

we should break instead of continue becaues the error is not expected.

Impact

the decay rate can be applied multiple times

Code Snippet

Tool used

Manual Review

Recommendation

.

Duplicate of #80

[sherlock-admin2]

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

0xmystery commented:

Function loop continues even after error when it should likely break

[sherlock-admin2]

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