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-35: Gas for hashing is not dependent on amount of words hashed #569

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

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 28, 2023

Description

The K256 and S256 instructions use constant gas even though they iterate over the memory range b..b + c. Therefore, the actual cost of these instructions should depend on the parameter c.

Figure 36.1: Code for charging gas for hashing. (fuel-vm/fuel-vm/src/interpreter/executors/instruction.rs#808–818)

Instruction::K256(k256) => {
    self.gas_charge(self.gas_costs.k256)?;
    let (a, b, c) = k256.unpack();
    self.keccak256(r!(a), r!(b), r!(c))?;
}
Instruction::S256(s256) => {
    self.gas_charge(self.gas_costs.s256)?;
    let (a, b, c) = s256.unpack();
    self.sha256(r!(a), r!(b), r!(c))?;
}

The fuzzer from the fuzzing appendix (see appendix E) uncovered this issue, as input execution of the hashing functions with a large C, caused a timeout.

Exploit Scenario

An attacker deploys a contract which heavily uses the above hashing functions. With very little gas consumption the attack can put a lot of stress on the network. Depending on how much gas the attacker invests the whole network could be blocked.

Recommendations

Short term, switch to a dependent gas calculation depending on c. We verified this fix by fuzzing. After applying a dependent gas charge, the VM no longer caused a timeout.
Long term, deploy the fuzzer described in the fuzzing appendix (see appendix E). By using a reasonably low timeout of 100ms to 1s it is possible to catch bugs like this.

@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 with #537, requires follow-up PR on the fuel-core side FuelLabs/fuel-core#1325

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