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-11: Code root and code size opcodes do not check that contract is in tx inputs #558

Closed
xgreenx opened this issue Aug 28, 2023 · 1 comment
Labels
audit-report Issue from the audit report

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 28, 2023

Description

The code size (CSIZ) and code root (CROO) opcodes do not validate that the contract argument is in the transaction’s input contacts field. This check prevents operating on contracts which do not exist as the interpreter checks all transaction inputs prior to execution (see figure 11.1). Since code size charges a dynamic cost only if a contract exists, the node will perform computation without charging gas.

Figure 11.1: The interpreter validates the existence of all input contracts upfront in the run function

if self.transaction().inputs().iter().any(|input| {
    if let Input::Contract(contract) = input {
        !self
            .check_contract_exists(&contract.contract_id)
            .unwrap_or(false)
} else {
false
} }) {
    return Err(InterpreterError::Panic(PanicReason::ContractNotFound))
}

Rather than reverting upon seeing a contract that was not included in the transaction, the interpreter continues to execute and may revert later than it would otherwise. Notably, the call to contract_size reverts before charging gas when a contract does not exist and thus does not have a code size. This may make the node susceptible to denial of service attacks.

Figure 11.2: The contract size is looked up and may revert before charging gas in the code size

implementation

pub(crate) fn code_size(self, result: &mut Word, b: Word) -> Result<(),
RuntimeError>
where
    S: StorageSize<ContractsRawCode>,
    <S as StorageInspect<ContractsRawCode>>::Error: Into<std::io::Error>,
{
    let contract_id = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?;
    let contract_id = ContractId::from_bytes_ref(contract_id.read(self.memory));
    let len = contract_size(self.storage, contract_id)?;
    let profiler = ProfileGas {
        pc: self.pc.as_ref(),
        is: self.is,
        current_contract: self.current_contract,
        profiler: self.profiler,
    };
    dependent_gas_charge(self.cgas, self.ggas, profiler, self.gas_cost, len)?;

The code copy opcode correctly validates that all contract operands were specified in the transaction’s input contracts.

Figure 11.3: The correct validation performed by the code copy implementation

pub(crate) fn code_copy(
[...]
    if !self.input_contracts.any(|input| input == contract) {
        *self.panic_context = PanicContext::ContractId(*contract);
        return Err(PanicReason::ContractNotInInputs.into())
    }

Exploit Scenario

An attacker sends a large number of transactions that invoke code size opcodes for non-existent accounts and does not include them in the transaction’s input contracts. Because the opcodes cannot rely on the precondition of the interpreter validating that all input contracts exist, they unnecessarily perform lookups and revert with ContractNotFound prior to charging the attacker for gas.

Recommendations

Short term, validate that the contract is specified in the transaction input contracts prior to performing lookups.
Long term, consider charging a static gas cost for dynamically priced opcodes upfront or penalizing transactions that leave out contracts from the transaction input contracts.

@xgreenx xgreenx added the audit-report Issue from the audit report label Aug 28, 2023
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 28, 2023

Fixed as part of the #504

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Issue from the audit report
Projects
None yet
Development

No branches or pull requests

1 participant