fix: claude-generated pre-audit of avm contract instance manager - completeness bug (#19254)

### Finding: Simulation Crash for Non-Existent Protocol Contracts

**Type:** Completeness
**Severity:** Medium (upgraded from Low after investigation)
**Location:** `simulation/gadgets/contract_instance_manager.cpp:47`

**Description:**
In `ContractInstanceManager::get_contract_instance()`, when a protocol
contract address (1 to MAX_PROTOCOL_CONTRACTS) is queried but the
derived address is not found (nullopt), the code crashes:

```cpp
if (ff_gt.ff_gt(MAX_PROTOCOL_CONTRACTS, contract_address - 1)) {
    std::optional<AztecAddress> derived_address = get_derived_address(protocol_contracts, contract_address);
    assert(derived_address.has_value() == maybe_instance.has_value() && ...);
    const ContractInstance& instance = maybe_instance.value();  // CRASH if nullopt
    event_emitter.emit({...});
    return instance;
}
```

**Evidence from Protocol:**
Investigation of
`yarn-project/protocol-contracts/src/protocol_contract_data.ts` reveals:
- `MAX_PROTOCOL_CONTRACTS = 11` (reserved capacity)
- Only **6 protocol contracts** are currently deployed (addresses 1-6)
- **Addresses 7-11 are explicitly zero** (empty slots)

```typescript
export const ProtocolContractsList = new ProtocolContracts([
  // 1-6: Actual protocol contracts with derived addresses
  AztecAddress.fromString('0x01826...'),  // AuthRegistry
  // ... 5 more ...
  // 7-11: Empty slots
  AztecAddress.fromString('0x0000000000...'),  // EMPTY
  AztecAddress.fromString('0x0000000000...'),  // EMPTY
  AztecAddress.fromString('0x0000000000...'),  // EMPTY
  AztecAddress.fromString('0x0000000000...'),  // EMPTY
  AztecAddress.fromString('0x0000000000...'),  // EMPTY
]);
```

**Impact:**
If a contract queries address 7, 8, 9, 10, or 11 using
`GETCONTRACTINSTANCE`:
1. The simulation crashes instead of returning `exists = false`
2. Valid AVM programs that check non-existent protocol contract
addresses will fail
3. The PIL constraints correctly handle this case (zero-check enforces
`exists = 0`), but simulation prevents reaching that path

**Recommendation:**
Handle the case where `derived_address` is nullopt by emitting an event
with `exists = false` and returning `std::nullopt`.

**Status: FIXED**

The fix was applied in `contract_instance_manager.cpp`:
- Use `maybe_instance.value_or(ContractInstance{})` for the event
(avoids `.value()` crash)
- Use `derived_address.has_value()` for the `exists` field
- Return `maybe_instance` directly (already an optional)
- Added comments explaining protocol contract capacity vs. usage

Test added:
`ContractInstanceManagerTest.EmptyProtocolContractSlotReturnsNullopt`

Author: dbanks12

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/contract_instance_manager.cpp

@@ -38,24 +38,30 @@ std::optional<ContractInstance> ContractInstanceManager::get_contract_instance(c
     std::optional<ContractInstance> maybe_instance = contract_db.get_contract_instance(contract_address);
 
     const auto& tree_state = merkle_db.get_tree_state();
-    // If this is a canonical address
+
+    // Check if this is a protocol contract address (addresses 1 to MAX_PROTOCOL_CONTRACTS).
+    // Protocol contracts are special reserved addresses that don't require nullifier checks.
     if (ff_gt.ff_gt(MAX_PROTOCOL_CONTRACTS, contract_address - 1)) {
-        // Handle protocol contract addresses
+        // Handle protocol contract addresses.
+        // The derived_address lookup returns nullopt if this protocol contract slot is empty.
+        // NOTE: MAX_PROTOCOL_CONTRACTS (currently 11) is the reserved capacity, but not all
+        // slots may be filled. For example, addresses 1-6 are currently used while 7-11 are
+        // empty (reserved for future protocol contracts).
         std::optional<AztecAddress> derived_address = get_derived_address(protocol_contracts, contract_address);
+
+        // Sanity check: if we found a derived address, we should also have the instance, and vice versa.
         assert(derived_address.has_value() == maybe_instance.has_value() &&
                "Derived address should be found if the instance was retrieved and vice versa");
-        const ContractInstance& instance = maybe_instance.value();
+
         event_emitter.emit({
             .address = contract_address,
-            .contract_instance = maybe_instance.value_or<ContractInstance>({}),
-            // Tree context
+            .contract_instance = maybe_instance.value_or(ContractInstance{}),
             .nullifier_tree_root = tree_state.nullifier_tree.tree.root,
             .public_data_tree_root = tree_state.public_data_tree.tree.root,
             .exists = derived_address.has_value(),
             .is_protocol_contract = true,
         });
-
-        return instance;
+        return maybe_instance;
     }
 
     if (!merkle_db.nullifier_exists(CONTRACT_INSTANCE_REGISTRY_CONTRACT_ADDRESS, contract_address)) {

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/contract_instance_manager.test.cpp

@@ -0,0 +1,195 @@
+#include "barretenberg/vm2/simulation/gadgets/contract_instance_manager.hpp"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <optional>
+
+#include "barretenberg/vm2/common/aztec_constants.hpp"
+#include "barretenberg/vm2/common/aztec_types.hpp"
+#include "barretenberg/vm2/simulation/events/contract_instance_retrieval_event.hpp"
+#include "barretenberg/vm2/simulation/events/event_emitter.hpp"
+#include "barretenberg/vm2/simulation/testing/mock_dbs.hpp"
+#include "barretenberg/vm2/simulation/testing/mock_field_gt.hpp"
+#include "barretenberg/vm2/simulation/testing/mock_update_check.hpp"
+#include "barretenberg/vm2/testing/fixtures.hpp"
+
+using ::testing::_;
+using ::testing::Return;
+using ::testing::SizeIs;
+using ::testing::StrictMock;
+
+namespace bb::avm2::simulation {
+namespace {
+
+class ContractInstanceManagerTest : public ::testing::Test {
+  protected:
+    StrictMock<MockContractDB> contract_db;
+    StrictMock<MockHighLevelMerkleDB> merkle_db;
+    StrictMock<MockUpdateCheck> update_check;
+    StrictMock<MockFieldGreaterThan> field_gt;
+    EventEmitter<ContractInstanceRetrievalEvent> event_emitter;
+};
+
+// Test that querying an empty protocol contract slot (addresses 7-11 are currently unused)
+// correctly returns nullopt with exists=false, rather than crashing.
+TEST_F(ContractInstanceManagerTest, EmptyProtocolContractSlotReturnsNullopt)
+{
+    // Create protocol contracts with only first 6 slots filled (matching real configuration)
+    // Slots 7-11 are zero (empty).
+    ProtocolContracts protocol_contracts;
+    for (uint32_t i = 0; i < 6; i++) {
+        // Fill slots 0-5 (addresses 1-6) with non-zero derived addresses
+        protocol_contracts.derived_addresses[i] = AztecAddress(FF(0x1000 + i));
+    }
+    // Slots 6-10 (addresses 7-11) are left as zero (default)
+
+    ContractInstanceManager manager(contract_db, merkle_db, update_check, field_gt, protocol_contracts, event_emitter);
+
+    // Query address 7 - an empty protocol contract slot
+    AztecAddress empty_slot_address = AztecAddress(7);
+
+    // Setup mocks
+    TreeStates tree_states = {};
+    EXPECT_CALL(merkle_db, get_tree_state()).WillOnce(Return(tree_states));
+
+    // The contract DB should return nullopt since the instance doesn't exist
+    EXPECT_CALL(contract_db, get_contract_instance(empty_slot_address)).WillOnce(Return(std::nullopt));
+
+    // ff_gt(MAX_PROTOCOL_CONTRACTS=11, 7-1=6) returns true because 11 > 6
+    // This means address 7 IS in the protocol contract range
+    EXPECT_CALL(field_gt, ff_gt(FF(MAX_PROTOCOL_CONTRACTS), FF(6))).WillOnce(Return(true));
+
+    // The call should NOT crash and should return nullopt
+    auto result = manager.get_contract_instance(empty_slot_address);
+
+    // Verify the result
+    EXPECT_FALSE(result.has_value());
+
+    // Verify the event was emitted correctly
+    auto events = event_emitter.dump_events();
+    ASSERT_THAT(events, SizeIs(1));
+    EXPECT_EQ(events[0].address, empty_slot_address);
+    EXPECT_FALSE(events[0].exists);
+    EXPECT_TRUE(events[0].is_protocol_contract);
+    EXPECT_EQ(events[0].contract_instance, ContractInstance{});
+}
+
+// Test that a valid protocol contract (e.g., address 1) works correctly
+TEST_F(ContractInstanceManagerTest, ValidProtocolContractReturnsInstance)
+{
+    // Create protocol contracts with first slot filled
+    ProtocolContracts protocol_contracts;
+    AztecAddress derived_addr = AztecAddress(FF(0x12345));
+    protocol_contracts.derived_addresses[0] = derived_addr; // Address 1 -> index 0
+
+    ContractInstanceManager manager(contract_db, merkle_db, update_check, field_gt, protocol_contracts, event_emitter);
+
+    // Query address 1 - a valid protocol contract
+    AztecAddress protocol_address = AztecAddress(1);
+
+    // Create a contract instance
+    ContractInstance instance = testing::random_contract_instance();
+
+    // Setup mocks
+    TreeStates tree_states = {};
+    EXPECT_CALL(merkle_db, get_tree_state()).WillOnce(Return(tree_states));
+    EXPECT_CALL(contract_db, get_contract_instance(protocol_address)).WillOnce(Return(instance));
+
+    // ff_gt(MAX_PROTOCOL_CONTRACTS=11, 1-1=0) returns true because 11 > 0
+    EXPECT_CALL(field_gt, ff_gt(FF(MAX_PROTOCOL_CONTRACTS), FF(0))).WillOnce(Return(true));
+
+    // The call should succeed and return the instance
+    auto result = manager.get_contract_instance(protocol_address);
+
+    // Verify the result
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(result.value(), instance);
+
+    // Verify the event was emitted correctly
+    auto events = event_emitter.dump_events();
+    ASSERT_THAT(events, SizeIs(1));
+    EXPECT_EQ(events[0].address, protocol_address);
+    EXPECT_TRUE(events[0].exists);
+    EXPECT_TRUE(events[0].is_protocol_contract);
+    EXPECT_EQ(events[0].contract_instance, instance);
+}
+
+// Test that a regular (non-protocol) contract that exists works correctly
+TEST_F(ContractInstanceManagerTest, RegularContractExistsReturnsInstance)
+{
+    ProtocolContracts protocol_contracts; // Empty - no protocol contracts
+
+    ContractInstanceManager manager(contract_db, merkle_db, update_check, field_gt, protocol_contracts, event_emitter);
+
+    // Query a regular address (outside protocol contract range)
+    AztecAddress regular_address = AztecAddress(FF(0x1234567890ULL));
+
+    // Create a contract instance
+    ContractInstance instance = testing::random_contract_instance();
+
+    // Setup mocks
+    TreeStates tree_states = {};
+    EXPECT_CALL(merkle_db, get_tree_state()).WillOnce(Return(tree_states));
+    EXPECT_CALL(contract_db, get_contract_instance(regular_address)).WillOnce(Return(instance));
+
+    // ff_gt(11, large_address - 1) returns false - not in protocol range
+    EXPECT_CALL(field_gt, ff_gt(FF(MAX_PROTOCOL_CONTRACTS), regular_address - 1)).WillOnce(Return(false));
+
+    // Nullifier exists - contract is deployed
+    EXPECT_CALL(merkle_db, nullifier_exists(FF(CONTRACT_INSTANCE_REGISTRY_CONTRACT_ADDRESS), regular_address))
+        .WillOnce(Return(true));
+
+    // Update check is performed for regular contracts
+    EXPECT_CALL(update_check, check_current_class_id(regular_address, instance));
+
+    // The call should succeed
+    auto result = manager.get_contract_instance(regular_address);
+
+    // Verify the result
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(result.value(), instance);
+
+    // Verify the event
+    auto events = event_emitter.dump_events();
+    ASSERT_THAT(events, SizeIs(1));
+    EXPECT_EQ(events[0].address, regular_address);
+    EXPECT_TRUE(events[0].exists);
+    EXPECT_FALSE(events[0].is_protocol_contract);
+}
+
+// Test that a regular contract that doesn't exist returns nullopt
+TEST_F(ContractInstanceManagerTest, RegularContractNotExistsReturnsNullopt)
+{
+    ProtocolContracts protocol_contracts; // Empty
+
+    ContractInstanceManager manager(contract_db, merkle_db, update_check, field_gt, protocol_contracts, event_emitter);
+
+    AztecAddress non_existent_address = AztecAddress(FF(0xDEADBEEFULL));
+
+    // Setup mocks
+    TreeStates tree_states = {};
+    EXPECT_CALL(merkle_db, get_tree_state()).WillOnce(Return(tree_states));
+    EXPECT_CALL(contract_db, get_contract_instance(non_existent_address)).WillOnce(Return(std::nullopt));
+
+    // Not in protocol range
+    EXPECT_CALL(field_gt, ff_gt(FF(MAX_PROTOCOL_CONTRACTS), non_existent_address - 1)).WillOnce(Return(false));
+
+    // Nullifier doesn't exist - contract not deployed
+    EXPECT_CALL(merkle_db, nullifier_exists(FF(CONTRACT_INSTANCE_REGISTRY_CONTRACT_ADDRESS), non_existent_address))
+        .WillOnce(Return(false));
+
+    // The call should return nullopt
+    auto result = manager.get_contract_instance(non_existent_address);
+
+    EXPECT_FALSE(result.has_value());
+
+    // Verify the event
+    auto events = event_emitter.dump_events();
+    ASSERT_THAT(events, SizeIs(1));
+    EXPECT_EQ(events[0].address, non_existent_address);
+    EXPECT_FALSE(events[0].exists);
+    EXPECT_FALSE(events[0].is_protocol_contract);
+}
+
+} // namespace
+} // namespace bb::avm2::simulation