0x3b - `GetForecastScoresUntilBlock` can get more score samples than the max allowed

[sherlock-admin3]

0x3b

Medium

GetForecastScoresUntilBlock can get more score samples than the max allowed

Summary

A for loop loop inside GetForecastScoresUntilBlock can accidentally pick different sized sample for participants scores, which will lead to different rewards.

Vulnerability Detail

The for loop inside GetForecastScoresUntilBlock that extracts score samples has count < int(maxNumTimeSteps) where it would prevent it from appending more score samples than the maxNumTimeSteps .
https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/keeper/keeper.go#L1902-L1914

    maxNumTimeSteps := moduleParams.MaxSamplesToScaleScores

    count := 0
    for ; iter.Valid() && count < int(maxNumTimeSteps); iter.Next() {
        existingScores, err := iter.KeyValue()
        if err != nil {
            return nil, err
        }
        //@audit count can pass `maxNumTimeSteps` here ?
        for _, score := range existingScores.Value.Scores {
            scores = append(scores, score)
            count++
        }
    }

However as we can see score is iterated inside the inner for loop and that one doesn't sop even if we surpass maxNumTimeSteps. This would cause a discrepancy between different topics and workers, as [GetForecastScoresUntilBlock]() is used inside GenerateRewardsDistributionByTopicParticipant ( GetForecastingTaskRewardFractions -> GetWorkersRewardFractions) to generate the scores and from them the rewards each worker should have.

https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/module/rewards/worker_rewards.go#L61-L69

   latestScoresFromLastestTimeSteps, err := k.GetInferenceScoresUntilBlock(ctx, topicId, blockHeight)
   if err != nil {
      return []string{}, []alloraMath.Dec{}, errors.Wrapf(err, "failed to get worker inference scores from the latest time steps")
   }

   var workerLastScoresDec []alloraMath.Dec
   for _, score := range latestScoresFromLastestTimeSteps {
      workerLastScoresDec = append(workerLastScoresDec, score.Score)
   }

   scores = append(scores, workerLastScoresDec)
   }
    ...

   rewardFractions, err := GetScoreFractions(latestWorkerScores, flatten(scores), pReward, cReward, moduleParams.Epsilon)
   if err != nil {
      return []string{}, []alloraMath.Dec{}, errors.Wrapf(err, "failed to get score fractions")
   }

   return workers, rewardFractions, nil

Having different number of score samples (some above the max) will yield different results when it comes to reward calculation.

Impact

Workers will calculate different reward fractions (done inside GetWorkersRewardFractions) even if the scores are the same, as the for loop may pick max scores for one topic and above the max for another.

Code Snippet

    maxNumTimeSteps := moduleParams.MaxSamplesToScaleScores

    count := 0
    for ; iter.Valid() && count < int(maxNumTimeSteps); iter.Next() {
        existingScores, err := iter.KeyValue()
        if err != nil {
            return nil, err
        }
        //@audit count can pass `maxNumTimeSteps` here ?
        for _, score := range existingScores.Value.Scores {
            scores = append(scores, score)
            count++
        }
    }

Tool used

Manual Review

Recommendation

Have the cap also inside the inner for loop to prevent picking more scores than the max limit.

[sherlock-admin2]

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