Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Problem: Some Web3 RPC Handlers could panic #702

Merged
merged 3 commits into from
Oct 26, 2021
Merged

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Oct 25, 2021

Description

Closes: #701

Solution:

  • return error rather than panic when decoding invalid tx

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@yihuang
Copy link
Contributor Author

yihuang commented Oct 25, 2021

With this patch, the response of the tx mentioned in issue is:

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"SafeNewIntFromBigInt() out of bound"}}

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #702 (190b041) into main (08a8191) will decrease coverage by 0.24%.
The diff coverage is 53.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
- Coverage   57.36%   57.11%   -0.25%     
==========================================
  Files          63       63              
  Lines        5505     5557      +52     
==========================================
+ Hits         3158     3174      +16     
- Misses       2180     2201      +21     
- Partials      167      182      +15     
Impacted Files Coverage Δ
rpc/ethereum/types/utils.go 0.00% <ø> (ø)
x/evm/types/errors.go 100.00% <ø> (ø)
x/evm/types/dynamic_fee_tx.go 78.61% <19.23%> (-11.07%) ⬇️
x/evm/types/access_list_tx.go 68.91% <21.05%> (-6.64%) ⬇️
server/config/config.go 21.48% <36.36%> (-0.12%) ⬇️
encoding/config.go 88.09% <40.00%> (-6.91%) ⬇️
x/evm/types/legacy_tx.go 95.20% <68.42%> (-4.80%) ⬇️
x/evm/types/msg.go 70.86% <87.50%> (-0.09%) ⬇️
x/evm/types/tx_data.go 85.41% <87.50%> (+1.32%) ⬆️
x/evm/types/chain_config.go 100.00% <100.00%> (ø)
... and 7 more

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks all right -- there are also a few direct calls in the RPC handlers themselves, e.g.: https://github.com/yihuang/ethermint/blob/invalid-tx/rpc/ethereum/namespaces/eth/api.go#L487
are those safe?

@yihuang
Copy link
Contributor Author

yihuang commented Oct 25, 2021

looks all right -- there are also a few direct calls in the RPC handlers themselves, e.g.: https://github.com/yihuang/ethermint/blob/invalid-tx/rpc/ethereum/namespaces/eth/api.go#L487 are those safe?

it seems not, the fee is gasPrice * gasLimit, so it could still overflow the 256bits. I guess we can check this in ValidateBasic.

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Can you add a changelog entry?

Closes: evmos#701

Solution:
- return error rather than panic when decoding invalid tx
@yihuang
Copy link
Contributor Author

yihuang commented Oct 26, 2021

ACK. Can you add a changelog entry?

done, also added validations into the Validate methods, and unit test cases.

@fedekunze fedekunze merged commit bc1d81c into evmos:main Oct 26, 2021
@yihuang yihuang deleted the invalid-tx branch October 26, 2021 11:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Web3 RPC Handlers could panic
3 participants