#37443 [SC-Insight] Race Condition in KeyedBroadcaster Implementation

Submitted on Dec 4th 2024 at 20:30:36 UTC by @jovi for Audit Comp | Celo

  • Report ID: #37443

  • Report Type: Smart Contract

  • Report severity: Insight

  • Target: https://github.com/celo-org/optimism/blob/celo10/op-chain-ops/deployer/broadcaster/keyed.go

  • Impacts:

    • L2 re-org

    • Node clients may operate in non-deterministic manners

    • Increased node latency

Description

Brief/Intro

The current implementation of the KeyedBroadcaster lacks proper synchronization mechanisms, specifically mutex locks, when accessing and modifying the bcasts slice. This oversight can lead to data corruption, inconsistent state, and potential security vulnerabilities in the transaction broadcasting process.

Vulnerability Details

The KeyedBroadcaster struct in optimism/op-chain-ops/deployer/broadcaster/keyed.go is responsible for managing and broadcasting transactions. It contains a slice bcasts that stores pending broadcasts. The current implementation allows concurrent access to this slice without proper synchronization. Differently from Optimism's implementation at optimism/op-deployer/pkg/deployer/broadcaster/keyed.go at 4ee839ae8996c2d421a2d85fd5471897840014fa · ethereum-optimism/optimism

Key vulnerable points:

  1. The Hook method appends to the bcasts slice without any synchronization:

  1. The Broadcast method reads and modifies the bcasts slice without synchronization:

These operations are not thread-safe and can lead to race conditions when multiple goroutines access the KeyedBroadcaster concurrently.

In contrast, the Optimism implementation uses mutex locks to ensure thread-safety:

The lack of such synchronization in the Celo implementation exposes the system to race conditions.

Impact Details

The race condition in the KeyedBroadcaster can have the following consequences:

  1. Data Corruption: Concurrent modifications to the bcasts slice can lead to corrupted data, potentially resulting in invalid transactions being broadcasted or valid transactions being omitted.

  2. Inconsistent State: The state of the bcasts slice may become inconsistent across different goroutines, leading to missed broadcasts or duplicate broadcasts.

  3. Transaction Failures: Incorrect nonce values or gas limits may be used due to race conditions, causing transactions to fail.

  4. Increased Latency: The system may experience increased latency due to retries and error handling necessitated by failed or inconsistent transactions.

  5. Unreliable System Behavior: The non-deterministic nature of race conditions can make the system behave unpredictably, making it difficult to debug and maintain.

Proof of Concept

Proof of concept

In order to test the race condition aforementioned, paste the following code snippet at the keyed.go file at op-chain-ops/deployer/broadcast.

Now create the broadcast_test.go file at the same folder:

Run the test without the non-mutex implementation with the following command from the ./deployer directory:

The output should contain this log:

Now uncomment the mutex-implemented version of the broadcaster and repeat the test. It should pass.

Last updated

Was this helpful?