mev-beta/docs/8_reports/code_review_2025-10-27.md

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).