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

Priority big int conversion should not causing underflow #1787

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yzang2019
Copy link
Contributor

@yzang2019 yzang2019 commented Jul 26, 2024

Describe your changes and provide context

This PR fixes the immunify bug report: https://bugs.immunefi.com/dashboard/submission/32602?resolvedFilter=unresolved

The actual problem is:
this section of code:

	// calculate the priority by dividing the total fee with the native gas limit (i.e. the effective native gas price)
	priority := fc.CalculatePriority(ctx, txData)
	ctx = ctx.WithPriority(priority.Int64())

priority is a big.Int that's converted to an int64 without first checking if such a conversion is possible with IsInt64(). If priority is negative, converting to int64 causes the final value to underflow and wrap around to a very large integer (for example MaxInt64 – max priority).

The code that sets priority for non-EVM transactions correctly calls IsInt64() (https://github.com/sei-protocol/sei-cosmos/blob/10546b70331d5f13e52b38acbf366a527566c3f1/x/auth/ante/validator_tx_fee.go#L77-L79), but the EVM version is missing this check.

Testing performed to validate your change

ctx = ctx.WithPriority(priority.Int64())

// only set priority if it is valid
if priority.IsInt64() {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if priority is not int64? should we set some default priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be default to 0 if not set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But maybe we can explicitly set it to 0 again

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 61.42%. Comparing base (aefdcf8) to head (0b6e9a5).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1787      +/-   ##
==========================================
- Coverage   61.47%   61.42%   -0.05%     
==========================================
  Files         257      257              
  Lines       22284    22289       +5     
==========================================
- Hits        13698    13690       -8     
- Misses       7629     7631       +2     
- Partials      957      968      +11     
Files Coverage Δ
x/evm/ante/fee.go 59.75% <44.44%> (-4.72%) ⬇️

... and 7 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants