Bugfixes found during chatting with Auditors on July 7th (#504)

* Unified the check that the `ContractId` in inputs by `TouchedContracts` structure. Check that the contract in inputs for `CROO` and `CSIZ`. Fixed the bug with `total_code_size`. Fixed the bug with incorrect gas charging per unit. * Update CHANGELOG.md * Renamed `TouchedContracts` to `InputContracts`
FuelLabs · Jul 11, 2023 · bd985e2 · bd985e2
1 parent b8407f7
commit bd985e2
Show file tree

Hide file tree

Showing 13 changed files with 245 additions and 130 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,13 +8,31 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 Description of the upcoming release here.
 
-### Breaking
+### Fixed
+
+#### Breaking
 
 - [#502](https://github.com/FuelLabs/fuel-vm/pull/502) The algorithm used by the
     binary Merkle tree for generating Merkle proofs has been updated to remove
     the leaf data from the proof set. This change allows BMT proofs to conform
     to the format expected by the Solidity contracts used for verifying proofs.
 
+- [#503](https://github.com/FuelLabs/fuel-vm/pull/503): Use correct amount of gas in call
+    receipts when limited by cgas. Before this change, the `Receipt::Call` could show an incorrect value for the gas limit.
+
+- [#504](https://github.com/FuelLabs/fuel-vm/pull/504): The `CROO` and `CSIZ` opcodes require 
+    the existence of corresponding `ContractId` in the transaction's 
+    inputs(the same behavior as for the `CROO` opcode).
+
+- [#504](https://github.com/FuelLabs/fuel-vm/pull/504): The size of the contract 
+    was incorrectly padded. It affects the end of the call frame in the memory, 
+    making it not 8 bytes align. Also, it affects the cost of the contract 
+    call(in some cases, we charged less in some more).
+
+- [#504](https://github.com/FuelLabs/fuel-vm/pull/504): The charging for `DependentCost`
+    was done incorrectly, devaluing the `dep_per_unit` part. After the fixing of 
+    this, the execution should become much more expensive.
+
 ## [Version 0.34.1]
 
 Mainly new opcodes prices and small performance improvements in the `BinaryMerkleTree`.

diff --git a/fuel-vm/src/call.rs b/fuel-vm/src/call.rs
@@ -258,7 +258,8 @@ impl CallFrame {
 
     /// Padding to the next word boundary.
     pub fn code_size_padding(&self) -> Word {
-        self.code_size() % WORD_SIZE as Word
+        const WORD_SIZE: Word = crate::consts::WORD_SIZE as Word;
+        (WORD_SIZE - self.code_size() % WORD_SIZE) % WORD_SIZE
     }
 
     /// Total code size including padding.

diff --git a/fuel-vm/src/interpreter.rs b/fuel-vm/src/interpreter.rs
@@ -461,3 +461,27 @@ impl CheckedMetadata for CreateCheckedMetadata {
         self.gas_used_by_predicates = gas_used;
     }
 }
+
+pub(crate) struct InputContracts<'vm, I> {
+    tx_input_contracts: I,
+    panic_context: &'vm mut PanicContext,
+}
+
+impl<'vm, I: Iterator<Item = &'vm ContractId>> InputContracts<'vm, I> {
+    pub fn new(tx_input_contracts: I, panic_context: &'vm mut PanicContext) -> Self {
+        Self {
+            tx_input_contracts,
+            panic_context,
+        }
+    }
+
+    /// Checks that the contract is declared in the transaction inputs.
+    pub fn check(&mut self, contract: &ContractId) -> Result<(), PanicReason> {
+        if !self.tx_input_contracts.any(|input| input == contract) {
+            *self.panic_context = PanicContext::ContractId(*contract);
+            Err(PanicReason::ContractNotInInputs)
+        } else {
+            Ok(())
+        }
+    }
+}
diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs
@@ -53,7 +53,7 @@ use crate::{
     gas::DependentCost,
     interpreter::{
         receipts::ReceiptsCtx,
-        PanicContext,
+        InputContracts,
     },
     prelude::Profiler,
     storage::{
@@ -63,7 +63,6 @@ use crate::{
         InterpreterStorage,
     },
 };
-
 use fuel_asm::PanicReason;
 use fuel_storage::{
     StorageInspect,
@@ -128,8 +127,10 @@ where
             memory: &mut self.memory,
             storage: &mut self.storage,
             contract_max_size: self.params.contract_max_size,
-            input_contracts: self.tx.input_contracts(),
-            panic_context: &mut self.panic_context,
+            input_contracts: InputContracts::new(
+                self.tx.input_contracts(),
+                &mut self.panic_context,
+            ),
             ssp,
             sp,
             fp: fp.as_ref(),
@@ -173,8 +174,10 @@ where
         let owner = self.ownership_registers();
         let input = CodeCopyCtx {
             memory: &mut self.memory,
-            input_contracts: self.tx.input_contracts(),
-            panic_context: &mut self.panic_context,
+            input_contracts: InputContracts::new(
+                self.tx.input_contracts(),
+                &mut self.panic_context,
+            ),
             storage: &mut self.storage,
             owner,
             pc: self.registers.pc_mut(),
@@ -213,14 +216,17 @@ where
 
     pub(crate) fn code_root(&mut self, a: Word, b: Word) -> Result<(), RuntimeError> {
         let owner = self.ownership_registers();
-        code_root(
-            &self.storage,
-            &mut self.memory,
+        CodeRootCtx {
+            memory: &mut self.memory,
+            input_contracts: InputContracts::new(
+                self.tx.input_contracts(),
+                &mut self.panic_context,
+            ),
+            storage: &self.storage,
             owner,
-            self.registers.pc_mut(),
-            a,
-            b,
-        )
+            pc: self.registers.pc_mut(),
+        }
+        .code_root(a, b)
     }
 
     pub(crate) fn code_size(
@@ -243,6 +249,10 @@ where
             storage: &mut self.storage,
             gas_cost: self.gas_costs.csiz,
             profiler: &mut self.profiler,
+            input_contracts: InputContracts::new(
+                self.tx.input_contracts(),
+                &mut self.panic_context,
+            ),
             current_contract,
             cgas,
             ggas,
@@ -419,8 +429,7 @@ where
 struct LoadContractCodeCtx<'vm, S, I> {
     contract_max_size: u64,
     memory: &'vm mut [u8; MEM_SIZE],
-    input_contracts: I,
-    panic_context: &'vm mut PanicContext,
+    input_contracts: InputContracts<'vm, I>,
     storage: &'vm S,
     ssp: RegMut<'vm, SSP>,
     sp: RegMut<'vm, SP>,
@@ -484,11 +493,7 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> {
         // Safety: Memory bounds are checked and consistent
         let contract_id = ContractId::from_bytes_ref(contract_id);
 
-        // the contract must be declared in the transaction inputs
-        if !self.input_contracts.any(|id| id == contract_id) {
-            *self.panic_context = PanicContext::ContractId(*contract_id);
-            return Err(PanicReason::ContractNotInInputs.into())
-        };
+        self.input_contracts.check(contract_id)?;
 
         // fetch the storage contract
         let contract = super::contract::contract(self.storage, contract_id)?;
@@ -603,8 +608,7 @@ where
 
 struct CodeCopyCtx<'vm, S, I> {
     memory: &'vm mut [u8; MEM_SIZE],
-    input_contracts: I,
-    panic_context: &'vm mut PanicContext,
+    input_contracts: InputContracts<'vm, I>,
     storage: &'vm S,
     owner: OwnershipRegisters,
     pc: RegMut<'vm, PC>,
@@ -637,10 +641,7 @@ impl<'vm, S, I> CodeCopyCtx<'vm, S, I> {
 
         let contract = ContractId::from_bytes_ref(contract.read(self.memory));
 
-        if !self.input_contracts.any(|input| input == contract) {
-            *self.panic_context = PanicContext::ContractId(*contract);
-            return Err(PanicReason::ContractNotInInputs.into())
-        }
+        self.input_contracts.check(contract)?;
 
         let contract = super::contract::contract(self.storage, contract)?.into_owned();
 
@@ -698,52 +699,63 @@ pub(crate) fn coinbase<S: InterpreterStorage>(
     inc_pc(pc)
 }
 
-pub(crate) fn code_root<S>(
-    storage: &S,
-    memory: &mut [u8; MEM_SIZE],
+struct CodeRootCtx<'vm, S, I> {
+    memory: &'vm mut [u8; MEM_SIZE],
+    input_contracts: InputContracts<'vm, I>,
+    storage: &'vm S,
     owner: OwnershipRegisters,
-    pc: RegMut<PC>,
-    a: Word,
-    b: Word,
-) -> Result<(), RuntimeError>
-where
-    S: InterpreterStorage,
-{
-    let ax = checked_add_word(a, Bytes32::LEN as Word)?;
-    let contract_id = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?;
+    pc: RegMut<'vm, PC>,
+}
 
-    if ax > VM_MAX_RAM {
-        return Err(PanicReason::MemoryOverflow.into())
-    }
+impl<'vm, S, I: Iterator<Item = &'vm ContractId>> CodeRootCtx<'vm, S, I> {
+    pub(crate) fn code_root(mut self, a: Word, b: Word) -> Result<(), RuntimeError>
+    where
+        S: InterpreterStorage,
+    {
+        let ax = checked_add_word(a, Bytes32::LEN as Word)?;
+        let contract_id = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?;
 
-    let contract_id = ContractId::from_bytes_ref(contract_id.read(memory));
+        if ax > VM_MAX_RAM {
+            return Err(PanicReason::MemoryOverflow.into())
+        }
 
-    let (_, root) = storage
-        .storage_contract_root(contract_id)
-        .transpose()
-        .ok_or(PanicReason::ContractNotFound)?
-        .map_err(RuntimeError::from_io)?
-        .into_owned();
+        let contract_id = ContractId::from_bytes_ref(contract_id.read(self.memory));
 
-    try_mem_write(a, root.as_ref(), owner, memory)?;
+        self.input_contracts.check(contract_id)?;
 
-    inc_pc(pc)
+        let (_, root) = self
+            .storage
+            .storage_contract_root(contract_id)
+            .transpose()
+            .ok_or(PanicReason::ContractNotFound)?
+            .map_err(RuntimeError::from_io)?
+            .into_owned();
+
+        try_mem_write(a, root.as_ref(), self.owner, self.memory)?;
+
+        inc_pc(self.pc)
+    }
 }
 
-struct CodeSizeCtx<'vm, S> {
+struct CodeSizeCtx<'vm, S, I> {
     storage: &'vm S,
     memory: &'vm mut [u8; MEM_SIZE],
     gas_cost: DependentCost,
     profiler: &'vm mut Profiler,
+    input_contracts: InputContracts<'vm, I>,
     current_contract: Option<ContractId>,
     cgas: RegMut<'vm, CGAS>,
     ggas: RegMut<'vm, GGAS>,
     pc: RegMut<'vm, PC>,
     is: Reg<'vm, IS>,
 }
 
-impl<'vm, S> CodeSizeCtx<'vm, S> {
-    pub(crate) fn code_size(self, result: &mut Word, b: Word) -> Result<(), RuntimeError>
+impl<'vm, S, I: Iterator<Item = &'vm ContractId>> CodeSizeCtx<'vm, S, I> {
+    pub(crate) fn code_size(
+        mut self,
+        result: &mut Word,
+        b: Word,
+    ) -> Result<(), RuntimeError>
     where
         S: StorageSize<ContractsRawCode>,
         <S as StorageInspect<ContractsRawCode>>::Error: Into<std::io::Error>,
@@ -752,6 +764,8 @@ impl<'vm, S> CodeSizeCtx<'vm, S> {
 
         let contract_id = ContractId::from_bytes_ref(contract_id.read(self.memory));
 
+        self.input_contracts.check(contract_id)?;
+
         let len = contract_size(self.storage, contract_id)?;
         let profiler = ProfileGas {
             pc: self.pc.as_ref(),

diff --git a/fuel-vm/src/interpreter/blockchain/code_tests.rs b/fuel-vm/src/interpreter/blockchain/code_tests.rs
@@ -1,9 +1,11 @@
+use super::*;
 use crate::{
-    interpreter::memory::Memory,
+    interpreter::{
+        memory::Memory,
+        PanicContext,
+    },
     storage::MemoryStorage,
 };
-
-use super::*;
 use fuel_tx::Contract;
 
 #[test]
@@ -29,13 +31,13 @@ fn test_load_contract() -> Result<(), RuntimeError> {
         .storage_contract_insert(&contract_id, &Contract::from(vec![5u8; 400]))
         .unwrap();
 
+    let mut panic_context = PanicContext::None;
     let input_contracts = vec![contract_id];
     let input = LoadContractCodeCtx {
         contract_max_size: 100,
         storage: &storage,
         memory: &mut memory,
-        panic_context: &mut PanicContext::None,
-        input_contracts: input_contracts.iter(),
+        input_contracts: InputContracts::new(input_contracts.iter(), &mut panic_context),
         ssp: RegMut::new(&mut ssp),
         sp: RegMut::new(&mut sp),
         fp: Reg::new(&fp),
@@ -69,11 +71,11 @@ fn test_code_copy() -> Result<(), RuntimeError> {
         .unwrap();
 
     let input_contracts = vec![contract_id];
+    let mut panic_context = PanicContext::None;
     let input = CodeCopyCtx {
         storage: &storage,
         memory: &mut memory,
-        panic_context: &mut PanicContext::None,
-        input_contracts: input_contracts.iter(),
+        input_contracts: InputContracts::new(input_contracts.iter(), &mut panic_context),
         pc: RegMut::new(&mut pc),
         owner: OwnershipRegisters {
             sp: 1000,