# MEV Bot Code Review — October 27, 2025

## Overview
- **Scope:** Core arbitrage execution path, security/key management, operational configuration.
- **Reviewer:** Codex (GPT-5)  
- **Tests Run:** `go test ./...` *(fails — see Findings #6)*  
- **Status:** Blocking issues identified; remediation required before further rollout.

## Critical Findings

### 1. Live framework constructed with nil dependencies
- **Location:** `pkg/arbitrage/service.go:247-276`, `pkg/arbitrage/service.go:1668-1767`
- **What’s wrong:** `FlashSwapExecutor` and `LiveExecutionFramework` are instantiated with a nil `KeyManager` and zeroed contract addresses, yet `StartLiveMode` / `ExecuteOpportunityLive` invoke them during runtime.
- **Impact:** First execution attempts call `getTransactionOptions` → `executor.keyManager.GetActivePrivateKey()` and crash with “key manager not configured,” disabling live/monitor modes in production.
- **Fix:** Pass the real `security.KeyManager` and configured contract addresses into `NewFlashSwapExecutor` / `NewLiveExecutionFramework`, or guard live mode until those dependencies exist.

### 2. Shared `TransactOpts` causes race conditions & nonce reuse
- **Location:** `pkg/arbitrage/executor.go:384-407`, `pkg/arbitrage/service.go:636-674`, `config/arbitrum_production.yaml:255`
- **What’s wrong:** A single `*bind.TransactOpts` is stored on `ArbitrageExecutor` and mutated inside `ExecuteArbitrage`. Multiple opportunities are launched concurrently (config allows 3), so goroutines share and overwrite nonce, gas limit, and fee fields.
- **Impact:** High probability of duplicate nonces, incorrect gas bids, and data races leading to reverted or invalid transactions under load.
- **Fix:** Create a fresh `TransactOpts` per execution (clone signer) and guard shared state; ensure concurrency-safe nonce management.

### 3. Key derivation invalidates keystore on restart
- **Location:** `pkg/security/keymanager.go:1295-1314`, `pkg/security/keymanager.go:303-336`, `pkg/security/security_manager.go:129-138`
- **What’s wrong:** `deriveEncryptionKey` generates a new random salt each time the process starts, producing a different AES key for the same master secret. Stored encrypted keys/backups become unreadable, and the security manager’s separately constructed key manager derives yet another key.
- **Impact:** Keys created in one run cannot be decrypted after restart; signing fails, and backups are useless. Production restart equals outage.
- **Fix:** Persist the salt (e.g., alongside keystore) so the derived key is stable, or accept a user-provided salt. Ensure a single key-manager instance per keystore.

### 4. Production config silently overrides local runs
- **Location:** `cmd/mev-bot/main.go:83-89`
- **What’s wrong:** If `config/arbitrum_production.yaml` exists, it is always loaded—even in development—overriding `config/local.yaml`.
- **Impact:** Local tests default to live Arbitrum RPC endpoints and contracts, risking quota drain or unwanted on-chain activity.
- **Fix:** Respect `GO_ENV` (development vs production) before loading the production config; make the override explicit via CLI flag or env var.

### 5. Public repo leaks Chainstack endpoint token
- **Location:** `config/providers.yaml:36-55`, `docker-compose.production.yaml:17-19`
- **What’s wrong:** Hard-coded Chainstack URL includes an active access token.
- **Impact:** Anyone cloning the repo can consume RPC quota or launch attacks through that endpoint; provider may revoke access.
- **Fix:** Rotate the credential immediately and remove it from version control. Replace with placeholder variables documented for operators.

## Additional Findings

### 6. Test suite failing due to duplicate main packages
- **Location:** `scripts/load-pools.go:1-89`, `scripts/generate-key.go:1-70`
- **What’s wrong:** Multiple standalone utilities share the `github.com/fraktal/mev-beta/scripts` package, so `go test ./...` fails with “main redeclared.”
- **Impact:** CI cannot stay green; future regressions won’t surface automatically.
- **Fix:** Move each tool into its own module/folder (e.g., `scripts/cmd/...`) or add build tags to exclude them from the main module tests.

## Recommended Next Steps
1. Wire live execution components to real contract/key dependencies or gate the feature.
2. Refactor transaction signing to use isolated `TransactOpts` and robust nonce management.
3. Stabilize key derivation and consolidate key-manager ownership.
4. Adjust config loading logic; require explicit opt-in for production settings.
5. Rotate the leaked Chainstack token and scrub it from history/configs.
6. Reorganize Go scripts so `go test ./...` passes again; rerun full regression afterward.

## Test Status
- `go test ./...` — **FAILED**: build error (`github.com/fraktal/mev-beta/scripts` contains multiple `main` definitions).