Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TOB-FUEL-23: Consensus parameters are potentially incompatible on 32bit platforms #1261

Closed
Dentosal opened this issue Jul 25, 2023 · 0 comments · Fixed by #1464
Closed

TOB-FUEL-23: Consensus parameters are potentially incompatible on 32bit platforms #1261

Dentosal opened this issue Jul 25, 2023 · 0 comments · Fixed by #1464
Assignees
Labels
audit-report Somehow related to the audit report bug Something isn't working

Comments

@Dentosal
Copy link
Member

Dentosal commented Jul 25, 2023

Description

The consensus parameters define the maximum length of vectors such as max_inputs, max_outputs, max_witnesses to be 64-bit values.These values are cast to usize when validating that a transaction’s field does not contain a vector which exceeds these maximums (see figure 23.2). On 32-bit architectures, usize will truncate consensus parameter values larger than 32 bits and reject different transactions than 64-bit platforms.

Figure 23.1: The ConsensusParameters struct definition (fuel-core/crates/client/src/client/schema/chain.rs#11–27)

pub struct ConsensusParameters {
    pub contract_max_size: U64,
    pub max_inputs: U64,
    pub max_outputs: U64,
    pub max_witnesses: U64,
    pub max_gas_per_tx: U64,
    pub max_script_length: U64,
    pub max_script_data_length: U64,
    pub max_storage_slots: U64,
    pub max_predicate_length: U64,
    pub max_predicate_data_length: U64,
    pub max_gas_per_predicate: U64,
    pub gas_price_factor: U64,
    pub gas_per_byte: U64,
    pub max_message_data_length: U64,
    pub chain_id: U64,
}

Figure 23.2: Validation of transactions against consensus parameters (fuel-vm/fuel-tx/src/transaction/validity.rs#325–335)

if tx.inputs().len() > parameters.max_inputs as usize {
    Err(CheckError::TransactionInputsMax)?
}
if tx.outputs().len() > parameters.max_outputs as usize {
    Err(CheckError::TransactionOutputsMax)?
}
if tx.witnesses().len() > parameters.max_witnesses as usize {
    Err(CheckError::TransactionWitnessesMax)?
}

Recommendations

Short term, define the consensus parameters as 32-bit unsigned integers.
Long term, review platform-dependent behavior such as casts to usize and ensure that code is compatible across architectures.

Our thoughts

The actual use occurs here:
https://github.com/FuelLabs/fuel-vm/blob/20f1e9b888e359b9d1ac45214673e97277370543/fuel-tx/src/transaction/validity.rs#L325-L335

Either change the data types to be u32, or remove usize casts from the fuel-vm side. Or maybe specify that fuel-vm/core only work on 64bit platforms and add test to that effect.

@Dentosal Dentosal added the bug Something isn't working label Jul 25, 2023
@xgreenx xgreenx changed the title Consensus parameters are potentially incompatible on 32bit platforms TOB-FUEL-23: Consensus parameters are potentially incompatible on 32bit platforms Aug 28, 2023
@xgreenx xgreenx added the audit-report Somehow related to the audit report label Aug 28, 2023
xgreenx added a commit that referenced this issue Oct 31, 2023
Closes #1261

Awaits reelasing of the FuelLabs/fuel-vm#619.

---------

Co-authored-by: Hannes Karppila <hannes.karppila@gmail.com>
@xgreenx xgreenx self-assigned this Nov 1, 2023
crypto523 pushed a commit to crypto523/fuel-core that referenced this issue Oct 7, 2024
Closes FuelLabs/fuel-core#1261

Awaits reelasing of the FuelLabs/fuel-vm#619.

---------

Co-authored-by: Hannes Karppila <hannes.karppila@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Somehow related to the audit report bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants