From d587c4fe619458b9e036ab44ecb8318cd91a0de6 Mon Sep 17 00:00:00 2001 From: Diederik Loerakker Date: Wed, 26 Jun 2019 21:21:17 +0200 Subject: [PATCH] Critical fix: introduce back total-value check (#1220) This was dropped in a376b6607fe5e6406371f44254960e891ee5ee8d, as improvement in dust checking. Now that dust-checking is done, we still need to check if the sender has the minimum value, as decrease balance just clips to 0. See be86f966f87958856584b3f20c095abf910a3d0c for older dust-creation problem work around, which was dropped in the above. The bug enabled you to transfer your full balance to someone else, and pay the same amount in fee, possibly to a puppet proposer to collect back funds. Effectively enabling printing of money. Silly bug, good to fix and introduce tests for. --- specs/core/0_beacon-chain.md | 4 +- .../block_processing/test_process_transfer.py | 138 +++++++++++++++++- 2 files changed, 138 insertions(+), 4 deletions(-) diff --git a/specs/core/0_beacon-chain.md b/specs/core/0_beacon-chain.md index 56a6fd06a..264c6d23b 100644 --- a/specs/core/0_beacon-chain.md +++ b/specs/core/0_beacon-chain.md @@ -1795,8 +1795,8 @@ def process_transfer(state: BeaconState, transfer: Transfer) -> None: """ Process ``Transfer`` operation. """ - # Verify the amount and fee are not individually too big (for anti-overflow purposes) - assert state.balances[transfer.sender] >= max(transfer.amount, transfer.fee) + # Verify the balance the covers amount and fee (with overflow protection) + assert state.balances[transfer.sender] >= max(transfer.amount + transfer.fee, transfer.amount, transfer.fee) # A transfer is valid in only one slot assert state.slot == transfer.slot # Sender must satisfy at least one of the following conditions in the parenthesis: diff --git a/test_libs/pyspec/eth2spec/test/phase_0/block_processing/test_process_transfer.py b/test_libs/pyspec/eth2spec/test/phase_0/block_processing/test_process_transfer.py index e9d282b3a..89246cc51 100644 --- a/test_libs/pyspec/eth2spec/test/phase_0/block_processing/test_process_transfer.py +++ b/test_libs/pyspec/eth2spec/test/phase_0/block_processing/test_process_transfer.py @@ -114,7 +114,7 @@ def test_incorrect_slot(spec, state): @with_all_phases @spec_state_test -def test_insufficient_balance_for_fee(spec, state): +def test_insufficient_balance_for_fee_result_dust(spec, state): sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] state.balances[sender_index] = spec.MAX_EFFECTIVE_BALANCE transfer = get_valid_transfer(spec, state, sender_index=sender_index, amount=0, fee=1, signed=True) @@ -127,7 +127,20 @@ def test_insufficient_balance_for_fee(spec, state): @with_all_phases @spec_state_test -def test_insufficient_balance(spec, state): +def test_insufficient_balance_for_fee_result_full(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=0, fee=state.balances[sender_index] + 1, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_for_amount_result_dust(spec, state): sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] state.balances[sender_index] = spec.MAX_EFFECTIVE_BALANCE transfer = get_valid_transfer(spec, state, sender_index=sender_index, amount=1, fee=0, signed=True) @@ -138,6 +151,127 @@ def test_insufficient_balance(spec, state): yield from run_transfer_processing(spec, state, transfer, False) +@with_all_phases +@spec_state_test +def test_insufficient_balance_for_amount_result_full(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=state.balances[sender_index] + 1, fee=0, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_for_combined_result_dust(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee without dust, and amount without dust, but not both. + state.balances[sender_index] = spec.MIN_DEPOSIT_AMOUNT + 1 + transfer = get_valid_transfer(spec, state, sender_index=sender_index, amount=1, fee=1, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_for_combined_result_full(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee fully without dust left, and amount fully without dust left, but not both. + state.balances[sender_index] = spec.MIN_DEPOSIT_AMOUNT * 2 + 1 + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=spec.MIN_DEPOSIT_AMOUNT + 1, + fee=spec.MIN_DEPOSIT_AMOUNT + 1, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_for_combined_big_amount(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee fully without dust left, and amount fully without dust left, but not both. + # Try to create a dust balance (off by 1) with combination of fee and amount. + state.balances[sender_index] = spec.MIN_DEPOSIT_AMOUNT * 2 + 1 + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=spec.MIN_DEPOSIT_AMOUNT + 1, fee=1, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_for_combined_big_fee(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee fully without dust left, and amount fully without dust left, but not both. + # Try to create a dust balance (off by 1) with combination of fee and amount. + state.balances[sender_index] = spec.MIN_DEPOSIT_AMOUNT * 2 + 1 + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=1, fee=spec.MIN_DEPOSIT_AMOUNT + 1, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_off_by_1_fee(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee fully without dust left, and amount fully without dust left, but not both. + # Try to print money by using the full balance as amount, plus 1 for fee. + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=state.balances[sender_index], fee=1, signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_off_by_1_amount(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee fully without dust left, and amount fully without dust left, but not both. + # Try to print money by using the full balance as fee, plus 1 for amount. + transfer = get_valid_transfer(spec, state, sender_index=sender_index, amount=1, + fee=state.balances[sender_index], signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + +@with_all_phases +@spec_state_test +def test_insufficient_balance_duplicate_as_fee_and_amount(spec, state): + sender_index = spec.get_active_validator_indices(state, spec.get_current_epoch(state))[-1] + # Enough to pay fee fully without dust left, and amount fully without dust left, but not both. + # Try to print money by using the full balance, twice. + transfer = get_valid_transfer(spec, state, sender_index=sender_index, + amount=state.balances[sender_index], + fee=state.balances[sender_index], signed=True) + + # un-activate so validator can transfer + state.validators[transfer.sender].activation_eligibility_epoch = spec.FAR_FUTURE_EPOCH + + yield from run_transfer_processing(spec, state, transfer, False) + + @with_all_phases @spec_state_test def test_no_dust_sender(spec, state):