From dc08003d8afec0c406a7ff243d667252cec6f82a Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Thu, 27 Jul 2023 09:30:30 +0300 Subject: [PATCH 1/3] Make TRO instruction revert on zero coin amount --- fuel-asm/src/panic_reason.rs | 2 + fuel-vm/src/interpreter/contract.rs | 6 +- fuel-vm/src/tests/outputs.rs | 113 ++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) diff --git a/fuel-asm/src/panic_reason.rs b/fuel-asm/src/panic_reason.rs index 858e0b18e2..6317d64ae8 100644 --- a/fuel-asm/src/panic_reason.rs +++ b/fuel-asm/src/panic_reason.rs @@ -108,6 +108,8 @@ enum_from! { ArithmeticError = 0x23, /// The contract instruction is not allowed in predicates. ContractInstructionNotAllowed = 0x24, + /// Transfer of zero coins is not allowed. + TransferZeroCoins = 0x25, } } diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 46de1dfa80..38a46bed82 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -237,7 +237,7 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { .check(&destination)?; if amount == 0 { - return Err(PanicReason::NotEnoughBalance.into()) + return Err(PanicReason::TransferZeroCoins.into()) } let internal_context = match internal_contract(self.context, self.fp, self.memory) @@ -315,6 +315,10 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { .expect("Unreachable! Checked memory range"); let amount = c; + if amount == 0 { + return Err(PanicReason::TransferZeroCoins.into()) + } + let internal_context = match internal_contract(self.context, self.fp, self.memory) { // optimistically attempt to load the internal contract id diff --git a/fuel-vm/src/tests/outputs.rs b/fuel-vm/src/tests/outputs.rs index 450c102a6d..8251b29fea 100644 --- a/fuel-vm/src/tests/outputs.rs +++ b/fuel-vm/src/tests/outputs.rs @@ -272,6 +272,119 @@ fn change_is_not_reduced_by_external_transfer_on_revert() { assert_eq!(change, input_amount); } +#[test] +fn zero_amount_transfer_reverts() { + let gas_price = 0; + let gas_limit = 1_000_000; + let asset_id = AssetId::default(); + + // setup state for test + // simple dummy contract for transferring value to + let contract_code = vec![op::ret(RegId::ONE)]; + + let mut test_context = TestBuilder::new(2322u64); + let contract_id = test_context + .setup_contract(contract_code, None, None) + .contract_id; + + // setup script for transfer + let (script, _) = script_with_data_offset!( + data_offset, + vec![ + // set reg 0x10 to contract id + op::movi(0x10, data_offset), + // set reg 0x12 to asset id + op::movi(0x11, data_offset + ContractId::LEN as u32), + // transfer to contract id at 0x10, amount of coins is zero, asset id at 0x11 + op::tr(0x10, RegId::ZERO, 0x11), + op::ret(RegId::ONE), + ], + test_context.get_tx_params().tx_offset() + ); + + let script_data = [contract_id.as_ref(), asset_id.as_ref()] + .into_iter() + .flatten() + .copied() + .collect(); + + // execute and get receipts + let result = test_context + .start_script(script, script_data) + .gas_price(gas_price) + .gas_limit(gas_limit) + .coin_input(asset_id, 0) + .contract_input(contract_id) + .change_output(asset_id) + .contract_output(&contract_id) + .execute(); + + let receipts = result.receipts(); + + if let Some(Receipt::Panic { reason, .. }) = receipts.first() { + assert_eq!(reason.reason(), &PanicReason::TransferZeroCoins); + } else { + panic!("Expected a panic receipt"); + } +} + +#[test] +fn zero_amount_transfer_out_reverts() { + let rng = &mut StdRng::seed_from_u64(2322u64); + + // the initial external (coin) balance + let external_balance = 1_000_000; + // the amount to transfer out from external balance + let gas_price = 0; + let gas_limit = 1_000_000; + let asset_id = AssetId::default(); + let owner: Address = rng.gen(); + + let (script, _) = script_with_data_offset!( + data_offset, + vec![ + // load amount of coins to 0x10 + op::movi(0x10, data_offset), + op::lw(0x10, 0x10, 0), + // load asset id to 0x11 + op::movi(0x11, data_offset), + // load address to 0x12 + op::movi(0x12, data_offset + 32), + // call contract without any tokens to transfer in or out + op::tro(0x12, RegId::ZERO, RegId::ZERO, 0x11), + op::ret(RegId::ONE), + ], + TxParameters::DEFAULT.tx_offset() + ); + + let script_data: Vec = [ + asset_id.as_ref(), + owner.as_ref(), + ] + .into_iter() + .flatten() + .copied() + .collect(); + + // execute and get receipts + let result = TestBuilder::new(2322u64) + .start_script(script, script_data) + .gas_price(gas_price) + .gas_limit(gas_limit) + .coin_input(asset_id, external_balance) + .variable_output(asset_id) + .change_output(asset_id) + .execute(); + + let receipts = result.receipts(); + + if let Some(Receipt::Panic { reason, .. }) = receipts.first() { + assert_eq!(reason.reason(), &PanicReason::TransferZeroCoins); + } else { + panic!("Expected a panic receipt"); + } +} + #[test] fn variable_output_set_by_external_transfer_out() { let rng = &mut StdRng::seed_from_u64(2322u64); From ae15cb487ca75eb893af55dd509e8689260ed32e Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Thu, 27 Jul 2023 09:35:04 +0300 Subject: [PATCH 2/3] Add changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7fd2c742e..b7a1a29864 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,12 +10,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [#525](https://github.com/FuelLabs/fuel-vm/pull/525): The `$hp` register is no longer restored to it's previous value when returning from a call, making it possible to return heap-allocated types from `CALL`. - #### Breaking - [#514](https://github.com/FuelLabs/fuel-vm/pull/514/): Add `ChainId` and `GasCosts` to `ConsensusParameters`. Break down `ConsensusParameters` into sub-structs to match usage. Change signatures of functions to ask for necessary fields only. +- [#532](https://github.com/FuelLabs/fuel-vm/pull/532): The `TRO` instruction now reverts when attempting to send zero coins to an output. Panic reason of this `TransferZeroCoins`, and `TR` was changed to use the same panic reason as well. ### Fixed From a3a7f3d16ae3ec534361a2a609ea91326f84e2f9 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Thu, 27 Jul 2023 09:37:13 +0300 Subject: [PATCH 3/3] fmt --- fuel-vm/src/tests/outputs.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/fuel-vm/src/tests/outputs.rs b/fuel-vm/src/tests/outputs.rs index 8251b29fea..117520e119 100644 --- a/fuel-vm/src/tests/outputs.rs +++ b/fuel-vm/src/tests/outputs.rs @@ -357,14 +357,11 @@ fn zero_amount_transfer_out_reverts() { TxParameters::DEFAULT.tx_offset() ); - let script_data: Vec = [ - asset_id.as_ref(), - owner.as_ref(), - ] - .into_iter() - .flatten() - .copied() - .collect(); + let script_data: Vec = [asset_id.as_ref(), owner.as_ref()] + .into_iter() + .flatten() + .copied() + .collect(); // execute and get receipts let result = TestBuilder::new(2322u64)