c4-bot-8 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/x/observer/genesis.go#L11

Vulnerability details

Impact

Due to the non-deterministic behavior of iteration over maps in Go, there could be two different types of errors in ballotListForHeight. Some examples could be Inconsistent State Initialization and Inconsistent Behavior. In Go, iteration over a map does not guarantee any specific order. This means that the order in which the BallotListForHeight objects are stored in the key-value store can vary between different runs of the program.

The BallotListForHeight is a struct that represents a list of ballots for a specific height. It has two properties: Height and BallotsIndexList.

The Height property is an int64 that represents the height for which the ballot list is applicable.

The BallotsIndexList property is a slice of strings. Each string in this slice is an index of a ballot. The GetBallotList function is used to retrieve a BallotListForHeight from the store using a height.

The AddBallotToList function retrieves a BallotListForHeight for a specific height, adds a ballot to it, and then stores it back in the store.

The GetMaturedBallotList function retrieves a BallotListForHeight for a specific height and returns the BallotsIndexList. The specific height is calculated as the current block height minus the BallotMaturityBlocks parameter.

Once the order in which the BallotListForHeight objects are stored in the key-value store. This order is not guaranteed to be consistent because Go's map does not guarantee a specific iteration order!!

Tools Used

Manual Review; Unit Test; Integration Test; VSCODE

Recommended Mitigation Steps

Modifying the data structure that holds the set of dependencies so that iteration becomes deterministic. For example, an array could be used which could be iterated over in the deduplication step in line 76(Line)[https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/x/observer/genesis.go#L76]. Alternatively, insert the code to sort the keys of the ballotListForHeight map right before the loop it iterate over this map to set the ballot list. This would be between lines 76 and 77. OP1:

// Existing code up to line 76...
//..........................
// Replace the map with a slice of struct that holds height and ballotList
type heightBallotList struct {
    Height     int64
    BallotList []string
}
var ballotListForHeight []heightBallotList

// When adding to ballotListForHeight, search for the existing height and append to its BallotList
// If the height does not exist, append a new heightBallotList to ballotListForHeight

// Sort the slice before iterating over it
sort.Slice(ballotListForHeight, func(i, j int) bool {
    return ballotListForHeight[i].Height < ballotListForHeight[j].Height
})

// Iterate over the sorted slice
for _, hbl := range ballotListForHeight {
    k.SetBallotList(ctx, &types.BallotListForHeight{
        Height:           hbl.Height,
        BallotsIndexList: hbl.BallotList,
    })
}

// .......

OP2:

// Existing code up to the creation of ballotListForHeight...

// Start of new code
heights := make([]int64, 0, len(ballotListForHeight))
for height := range ballotListForHeight {
    heights = append(heights, height)
}
sort.Slice(heights, func(i, j int) bool { return heights[i] < heights[j] })
// End of new code

// Replace the existing loop with this one
for _, height := range heights {
    ballotList := ballotListForHeight[height]
    k.SetBallotList(ctx, &types.BallotListForHeight{
        Height:           height,
        BallotsIndexList: ballotList,
    })
}