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

Handle potential overflow when calculating fee to be substracted #1181

Merged

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Jan 14, 2022

What does it do?

Probably not exploitable because the queue would prevent a message with that much weight from beign executed, but still

plus weight is u64

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@girazoki girazoki added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 14, 2022
Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One other thing I would consider in cases like this is whether or not the max int size is sufficient to prevent spam. Obviously, it's better than an int overflow, but ideally the max int size is going to exhaust block space in all cases (in other words, be invalid).

@librelois librelois mentioned this pull request Jan 14, 2022
32 tasks
@girazoki
Copy link
Collaborator Author

In this case they would need to pay a lot of money to even try to get this message processed. So in that sense I think we are good with this particular case

@girazoki girazoki merged commit 1f536e7 into master Jan 14, 2022
@girazoki girazoki deleted the girazoki-potential-overflow-when-calculating-substracted-fee branch January 14, 2022 18:00
@crystalin crystalin added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Jan 30, 2022
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants