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

Switch GasInt to uint64 #2292

Open
arnetheduck opened this issue Jun 4, 2024 · 2 comments
Open

Switch GasInt to uint64 #2292

arnetheduck opened this issue Jun 4, 2024 · 2 comments
Labels
Security Security vulnerability or security related

Comments

@arnetheduck
Copy link
Member

The initial choice in #35 (comment) is argued to simplify development by raising a Defect whenever an out-of-bounds computation happens.

While this might have been an appropriate choice in early-stages design, it is not a reliable choice for production code, also because types where GasInt is used are RLP-serialized and RLP doesn't support signed integers.

Since geth uses uint64, this will also ensure that our handling is similar to that of geth:

https://github.com/ethereum/go-ethereum/blob/682ae838b2312a4ec8e5581069039b567e33c7c2/core/types/block.go#L76C2-L76C10

As an alternative to plain uint64, we could also develop a special "checked uint64" in which all operations return an overflow flag that is checked where applicable.

@arnetheduck arnetheduck added the Security Security vulnerability or security related label Jun 4, 2024
@jangko
Copy link
Contributor

jangko commented Jun 4, 2024

A note: Some of the gas calculation depends on the int signedness, e.g. in evm/interpreter/gas_costs.nim and evm/interpreter/gas_meter.nim and in calculateAndPossiblyRefundGas of nimbus/transaction/call_common.nim are using equations that will result in negative number as intermediate value. We need to rewrite the equations to avoid negative number when fixing this issue.

@arnetheduck
Copy link
Member Author

Something to be careful about: code like

if tx.maxFee < baseFee.truncate(int64):
will allow negative values (!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security Security vulnerability or security related
Projects
None yet
Development

No branches or pull requests

2 participants