0x416 - Non-deterministic approach to iterate over the map can result in inconsistent state between nodes and validators

[sherlock-admin3]

0x416

High

Non-deterministic approach to iterate over the map can result in inconsistent state between nodes and validators

Summary

Non-deterministic approach to iterate over the map can result in inconsistent state between nodes and validators

Vulnerability Detail

https://www.halborn.com/blog/post/top-5-security-vulnerabilities-cosmos-developers-need-to-watch-out-for

the third one applies to the current codebase:

Iterating Over A Map

A common example of non-determinism in Cosmos projects occurs when iterating over a ‘map’ which is analogous to a dictionary or hashmap in other programming languages. Map orderings are not deterministic. This means that if a loop is used to list the elements of a map, the loop could traverse the elements of the map in a different order for every execution even when the data in the map has not changed

the same issue is here in this line of code

func ConvertWeightsToArrays(weights map[Worker]Weight) []*types.RegretInformedWeight {
    weightsArray := make([]*types.RegretInformedWeight, 0)
    for worker, weight := range weights {
        weightsArray = append(weightsArray, &types.RegretInformedWeight{Worker: worker, Weight: weight})
    }
    return weightsArray
}

func ConvertForecastImpliedInferencesToArrays(
    forecastImpliedInferenceByWorker map[string]*types.Inference,
) []*types.WorkerAttributedValue {
    forecastImpliedInferences := make([]*types.WorkerAttributedValue, 0)
    for worker, inference := range forecastImpliedInferenceByWorker {
        forecastImpliedInferences = append(forecastImpliedInferences, &types.WorkerAttributedValue{Worker: worker, Value: inference.Value})
    }
    return forecastImpliedInferences
}

these function is used by:

func (qs queryServer) GetLatestNetworkInference(
    ctx context.Context,
    req *types.QueryLatestNetworkInferencesAtBlockRequest,
) (
    *types.QueryLatestNetworkInferencesAtBlockResponse,
    error,
) {

    networkInferences, forecastImpliedInferenceByWorker, infererWeights, forecasterWeights, err := synth.GetLatestNetworkInference(
        sdk.UnwrapSDKContext(ctx),
        qs.k,
        req.TopicId,
    )
    if err != nil {
        return nil, err
    }

    return &types.QueryLatestNetworkInferencesAtBlockResponse{
        NetworkInferences:         networkInferences,
        InfererWeights:            synth.ConvertWeightsToArrays(infererWeights),
        ForecasterWeights:         synth.ConvertWeightsToArrays(forecasterWeights),
        ForecastImpliedInferences: synth.ConvertForecastImpliedInferencesToArrays(forecastImpliedInferenceByWorker),
    }, nil
}

calling this function first time the function could return

[(worker1, weight1), (worker2, weigh2)]

then next time the function could return

[(worker2, weight2), (worker1, weight1)]

then the code access the array first time at index 0, the code get worker1 info,

then the code access the array second time at index 0, the code get worker2 info.

this would leads to inconsistent state between nodes for data saved from inference synthesis

Impact

this would leads to inconsistent state between nodes for data saved from inference synthesis

Code Snippet

https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/inference_synthesis/common.go#L37

Tool used

Manual Review

Recommendation

the current code convert map to array, but the code should iterate over array instead of maps

Duplicate of #38

[sherlock-admin3]

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

0xmystery commented:

Map is non-deterministic

[sherlock-admin2]

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