Only reset builder withdrawal epoch if its balance has been swept (#5384)

@0xMushow raised a point on discord that an attacker could continually deposit
1 ETH every `MIN_BUILDER_WITHDRAWABILITY_DELAY` epochs to prevent a builder
from getting their balance back. One solution to this would be to only reset
the withdrawalable epoch on top-ups if the balance has already been swept. This
guarantees that the builder will get their balance back & prevents the original
attack which this block of code prevented. See the following for more details:

* #5373

Big thanks to @0xMushow for recognizing this!

Author: jtraglia

specs/gloas/beacon-chain.md

@@ -1700,13 +1700,13 @@ def process_builder_deposit_request(state: BeaconState, request: BuilderDepositR
         builder_index = BuilderIndex(builder_pubkeys.index(request.pubkey))
         builder = state.builders[builder_index]
 
-        # Increase balance by deposit amount
-        builder.balance += request.amount
-
-        # If exited, reset the withdrawable epoch
-        if builder.withdrawable_epoch != FAR_FUTURE_EPOCH:
+        # If exited and swept, reset the withdrawable epoch
+        if builder.withdrawable_epoch != FAR_FUTURE_EPOCH and builder.balance == 0:
             epoch = get_current_epoch(state)
             builder.withdrawable_epoch = epoch + MIN_BUILDER_WITHDRAWABILITY_DELAY
+
+        # Increase balance by deposit amount
+        builder.balance += request.amount
 ```
 
 ##### Builder exit requests

tests/core/pyspec/eth_consensus_specs/test/gloas/block_processing/test_process_builder_deposit_request.py

@@ -712,12 +712,13 @@ def test_process_builder_deposit_request__no_reuse_nonzero_balance(spec, state):
 
 @with_gloas_and_later
 @spec_state_test
-def test_process_builder_deposit_request__exited_builder_top_up(spec, state):
+def test_process_builder_deposit_request__exited_builder_top_up_zero_balance(spec, state):
     """
-    Test top-up to an exited builder resets its withdrawable epoch.
+    Test top-up to a fully-swept exited builder resets its withdrawable epoch.
 
     Input State Configured:
-        - Existing builder at index 0 that has exited (withdrawable_epoch in the past)
+        - Existing builder at index 0 that has exited (withdrawable_epoch in the
+          past) and has been fully swept (balance == 0)
 
     Output State Verified:
         - Builder balance increased (top-up)
@@ -727,22 +728,24 @@ def test_process_builder_deposit_request__exited_builder_top_up(spec, state):
     amount = spec.MIN_DEPOSIT_AMOUNT
     pre_builder_count = len(state.builders)
 
-    # Advance an epoch and mark builder 0 as exited (withdrawable_epoch in the past)
+    # Advance an epoch and mark builder 0 as exited (withdrawable_epoch in the
+    # past) and fully swept (balance == 0)
     deposit_request = prepare_process_builder_deposit_request(
         spec,
         state,
         pubkey=builder_pubkey,
         amount=amount,
         signed=True,
         advance_epochs=1,
-        builder_modifications={0: {"withdrawable_epoch": "current_epoch-1"}},
+        builder_modifications={0: {"withdrawable_epoch": "current_epoch-1", "balance": 0}},
     )
     pre_state = state.copy()
     expected_withdrawable_epoch = (
         spec.get_current_epoch(state) + spec.config.MIN_BUILDER_WITHDRAWABILITY_DELAY
     )
     # Sanity check: the exited epoch differs from the expected reset value
-    assert state.builders[0].withdrawable_epoch != expected_withdrawable_epoch
+    assert pre_state.builders[0].withdrawable_epoch != expected_withdrawable_epoch
+    assert pre_state.builders[0].balance == 0
 
     yield from run_builder_deposit_request_processing(spec, state, deposit_request)
 
@@ -756,3 +759,53 @@ def test_process_builder_deposit_request__exited_builder_top_up(spec, state):
         expected_builder_balance_delta=amount,
         expected_builder_withdrawable_epoch=expected_withdrawable_epoch,
     )
+
+
+@with_gloas_and_later
+@spec_state_test
+def test_process_builder_deposit_request__exited_builder_top_up_nonzero_balance(spec, state):
+    """
+    Test that a top-up to an exited builder that still holds a balance does NOT
+    reset its withdrawable epoch.
+
+    Input State Configured:
+        - Existing builder at index 0 that has exited (withdrawable_epoch in the
+          past) and still holds a large non-zero balance
+
+    Output State Verified:
+        - Builder balance increased by the top-up amount
+        - withdrawable_epoch unchanged (no reset)
+    """
+    builder_pubkey = state.builders[0].pubkey
+    balance = 1 * spec.ETH_TO_GWEI
+    amount = spec.Gwei(1 * spec.ETH_TO_GWEI)
+    pre_builder_count = len(state.builders)
+
+    # Advance an epoch and mark builder 0 as exited (withdrawable_epoch in the
+    # past) while keeping a non-zero balance (funds not yet swept)
+    deposit_request = prepare_process_builder_deposit_request(
+        spec,
+        state,
+        pubkey=builder_pubkey,
+        amount=amount,
+        signed=True,
+        advance_epochs=1,
+        builder_modifications={0: {"withdrawable_epoch": "current_epoch-1", "balance": balance}},
+    )
+    pre_state = state.copy()
+    # Sanity check: the builder is exited but still holds a balance
+    assert pre_state.builders[0].withdrawable_epoch != spec.FAR_FUTURE_EPOCH
+    assert pre_state.builders[0].balance > 0
+
+    yield from run_builder_deposit_request_processing(spec, state, deposit_request)
+
+    assert_process_builder_deposit_request(
+        spec,
+        state,
+        pre_state,
+        builder_deposit_request=deposit_request,
+        expected_builder_count=pre_builder_count,
+        expected_builder_index=0,
+        expected_builder_balance_delta=amount,
+        expected_builder_withdrawable_epoch=pre_state.builders[0].withdrawable_epoch,
+    )