op-program: Improve transitionState sanity check during consolidation
Spearbit audit finding:
Description:
The current implementation compares the length of transitionState.PendingProgress against the constant ConsolidateStep (value: 127)`, unrelated to the actual length requirement.
} else if transitionState.Step == ConsolidateStep {
logger.Info("Running consolidate step")
// sanity check
if len(transitionState.PendingProgress) >= ConsolidateStep {
return common.Hash{}, fmt.Errorf("pending progress length does not match the expected step")
}
// ...
This check fails to validate that the PendingProgress array has the correct number of elements to match the superRoot.Chains array. In the subsequent RunConsolidation() function, the code accesses elements from both arrays using the same indices, assuming a one-to-one correspondence.
func RunConsolidation(
logger log.Logger,
bootInfo *boot.BootInfoInterop,
l1PreimageOracle l1.Oracle,
l2PreimageOracle l2.Oracle,
transitionState *types.TransitionState,
superRoot *eth.SuperV1,
tasks taskExecutor,
) (eth.Bytes32, error) {
consolidateState := consolidateState{
TransitionState: &types.TransitionState{
PendingProgress: make([]types.OptimisticBlock, len(transitionState.PendingProgress)),
SuperRoot: transitionState.SuperRoot,
Step: transitionState.Step,
},
replacedChains: make(map[eth.ChainID]bool),
}
// We will be updating the transition state as blocks are replaced, so make a copy
copy(consolidateState.PendingProgress, transitionState.PendingProgress)
// ...
var consolidatedChains []eth.ChainIDAndOutput
for i, chain := range superRoot.Chains {
consolidatedChains = append(consolidatedChains, eth.ChainIDAndOutput{
ChainID: chain.ChainID,
Output: consolidateState.PendingProgress[i].OutputRoot,
})
}
// ...
}
This could lead to:
Array Index Out of Bounds: If PendingProgress has fewer elements than superRoot.Chains, the program will attempt to access non-existent elements in the PendingProgress array.
Silent Data Inconsistency: If PendingProgress has more elements than superRoot.Chains, the extra elements will be ignored, potentially leading to state inconsistencies.
Recommendation:
Add sanity checks to directly validate the relationship between PendingProgress and superRoot.Chains:
if len(transitionState.PendingProgress) != len(superRoot.Chains) {
return common.Hash{}, fmt.Errorf("pending progress length (%d) does not match chains length (%d)",
len(transitionState.PendingProgress), len(superRoot.Chains))
}
- This ensures that there is exactly one progress entry for each chain, which is the invariant required for the consolidation process to work correctly.