Risk of race condition in witness allocation may result in multiple threads eventually retrieving the same witness or overwriting each other's changes

[c4-submissions]

Lines of code

https://github.com/matter-labs/era-zkevm_circuits/blob/main/src/storage_application/mod.rs#L128-L138

Vulnerability details

Summary

The methods in ConditionalWitnessAllocator such as conditionally_allocate_with_default and conditionally_allocate_with_default_biased use a shared Arc<RwLock<VecDeque<EL::Witness>>> to manage witnesses. However, the code seems to be vulnerable to race conditions that could lead to unexpected behavior if two or more threads attempt to allocate a witness at the same time.

Vulnerability Detail

When a thread attempts to allocate a witness, it acquires a write lock on the witness_source and pops the front element. If two or more threads attempt to do this simultaneously, there's a chance that they may pop the same witness or encounter a race condition. Even though Rust's memory safety guarantees would prevent data races, logical race conditions are still possible.

The specific code in question is:

let witness = if should_allocate == true {
    let mut guard = witness.write().expect("not poisoned");
    let witness_element = guard.pop_front().expect("not empty witness");
    drop(guard);
    witness_element
} else {
    let witness_element = (default_values_closure)();
    witness_element
};

This snippet is problematic because, while acquiring the write lock guarantees that the actual mutation of the witness_source is thread-safe, the decision to pop the element from the queue (based on should_allocate) is not atomic with the actual popping.

Impact

This issue can lead to unexpected behavior where multiple threads could eventually retrieve the same witness or overwrite each other's changes, leading to unpredictable outcomes in the application's state.

Proof of Concept

Reproducing this vulnerability would require creating multiple threads that simultaneously attempt to allocate witnesses. In a real-world scenario, a high throughput of requests could trigger this race condition, thus leading to inconsistent state in the CA.

Tools Used

Manual review + GPT for further text enhancements.

Recommended Mitigation Steps

1. The simplest way to fix this would be to move the should_allocate check inside the write lock, ensuring atomicity:

let mut guard = witness.write().expect("not poisoned");
let witness_element = if should_allocate {
    guard.pop_front().expect("not empty witness")
} else {
    (default_values_closure)()
};
drop(guard);

2. Consider using a thread-safe queue like crossbeam::queue::ArrayQueue or other concurrent data structures that handle concurrent pushes/pops safely without the need for manual locking.

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as low quality report

[miladpiri]

For cases when those could happen we bias entering the next segment of such code by requiring a witness variable from that section to be resolved. There is a comment over it in the corresponding files.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid