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

Fix underflow in modexp gas calculation #883

Conversation

guidovranken
Copy link
Contributor

Description

In calc_iter_count(), which is part of the modexp precompile gas calculation, the most-significant bit of the exponent is added to the intermediate result. The MSB is computed using U256::from(exp.bits()) - U256::from(1)) in:

Ok(U256::from(8) * U256::from(exp_len - 32) + U256::from(exp.bits()) - U256::from(1))

This works only if the exponent is non-zero; subtracting 1 from zero results in an underflow, leading to inaccurate gas requirements for specific inputs.

Geth computes the MSB as follows; notice the condition bitlen > 0:

	var msb int
	if bitlen := expHead.BitLen(); bitlen > 0 {
		msb = bitlen - 1
	}

This PR changes the underflowing subtraction into a saturating_sub so that an underflow never occurs.

Performance / NEAR gas cost considerations

Testing

Fuzzing.

How should this be reviewed

A test has been added to modexp.rs which asserts that the required gas for the specified input is 65536; an equivalent Go program which uses Geth to demonstrate this is provided below:

package main

import (
    "github.com/ethereum/go-ethereum/core/vm"
    "github.com/ethereum/go-ethereum/common"
    "fmt"
)

func main() {
    input := []byte{
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x26, 0x00}
    modexp_address := common.BytesToAddress([]byte{0x05})
    gas := vm.PrecompiledContractsBerlin[modexp_address].RequiredGas(input)
    fmt.Println(gas)
}

Additional information

@aleksuss aleksuss added this pull request to the merge queue Dec 11, 2023
Merged via the queue into aurora-is-near:develop with commit c7771cb Dec 11, 2023
22 checks passed
aleksuss added a commit that referenced this pull request Feb 6, 2024
## Description

In `calc_iter_count()`, which is part of the modexp precompile gas
calculation, the most-significant bit of the exponent is added to the
intermediate result. The MSB is computed using `U256::from(exp.bits()) -
U256::from(1))` in:

```rust
Ok(U256::from(8) * U256::from(exp_len - 32) + U256::from(exp.bits()) - U256::from(1))
```

This works only if the exponent is non-zero; subtracting `1` from zero
results in an underflow, leading to inaccurate gas requirements for
specific inputs.

Geth computes the MSB [as
follows](https://github.com/ethereum/go-ethereum/blob/fa0df76f3cfd186a1f06f2b80aa5dbb89555b009/core/vm/contracts.go#L340);
notice the condition `bitlen > 0`:

```go
	var msb int
	if bitlen := expHead.BitLen(); bitlen > 0 {
		msb = bitlen - 1
	}
```

This PR changes the underflowing subtraction into a `saturating_sub` so
that an underflow never occurs.

## Performance / NEAR gas cost considerations

## Testing

Fuzzing.

## How should this be reviewed

A test has been added to `modexp.rs` which asserts that the required gas
for the specified input is `65536`; an equivalent Go program which uses
Geth to demonstrate this is provided below:

```go
package main

import (
    "github.com/ethereum/go-ethereum/core/vm"
    "github.com/ethereum/go-ethereum/common"
    "fmt"
)

func main() {
    input := []byte{
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x26, 0x00}
    modexp_address := common.BytesToAddress([]byte{0x05})
    gas := vm.PrecompiledContractsBerlin[modexp_address].RequiredGas(input)
    fmt.Println(gas)
}
```

## Additional information

Co-authored-by: Oleksandr Anyshchenko <oleksandr.anyshchenko@aurora.dev>
@aleksuss aleksuss mentioned this pull request Feb 6, 2024
aleksuss added a commit that referenced this pull request Feb 6, 2024
## 3.6.0 2024-02-06

### Fixes

- Fixed underflow in the modexp gas calculation by [@guidovranken].
([#883])
- Prevented subtraction underflow in th xcc module by [@guidovranken].
([#888])
- Fixed balance and gas overflows in the xcc module by [@guidovranken].
([#889])

### Changes

- CI was updated by changing self-hosted runner to the GitHub heavy by
[@aleksuss]. ([#881])
- Removed a logic of fee calculation in the eth-connector by
[@karim-en]. ([#882])
- Version of the rust nightly was updated to 2023-12-15 by
[@RomanHodulak]. ([#885])

[#881]: #881
[#882]: #882
[#883]: #883
[#885]: #885
[#888]: #888
[#889]: #889

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Guido Vranken <guidovranken@users.noreply.github.com>
Co-authored-by: Karim <karim@aurora.dev>
Co-authored-by: Roman Hodulák <roman.hodulak@aurora.dev>
Co-authored-by: Michael Birch <birchmd8@gmail.com>
Co-authored-by: Michael Birch <michael.birch@aurora.dev>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants