Skip to content

fix(x/callback): audit remidiations #539

Merged
spoo-bar merged 12 commits into
mainfrom
spoorthi/audit-remediations
Feb 5, 2024
Merged

fix(x/callback): audit remidiations #539
spoo-bar merged 12 commits into
mainfrom
spoorthi/audit-remediations

Conversation

@spoo-bar

@spoo-bar spoo-bar commented Jan 31, 2024

Copy link
Copy Markdown
Contributor

Addressed + Added the following:

  • When a callback is registered and after that the price of gas increases so much that the tx fees paid are not enough to cover the execution, the module would attempt to send extra amount to the fee collector and would panic. Fix ensures at max the txFees sent by the user are what is send to the fee collector
  • isAuthorizedToModify func which was used to perform checks on who can create/update callbacks was case sensitive. The fix ensures even case insensitive inputs match
  • Estimate callback fees would not error when input height was current height. Fix throws error when input height is in the past or the present
  • Update ValidateBasic for MsgRegisterCallback to check the denom of the payment to be the sdk.DefaultBondDenom
  • Modify the CancelCallback logic to prevent redundancy of logic in checking if the callback already exists
  • Use prop values instead of getters at various places
  • Some additional code comments
  • Prevent blocked addresses from registering callbacks. During refund of callbacks, ensure the recipient is not blocked addr. If they are, send the amount to the fee collector instead. (Usually module accounts are blocked accounts. So they wont be creating callbacks. but added the checks to prevent panics and chain halts)

@codecov

codecov Bot commented Jan 31, 2024

Copy link
Copy Markdown

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@7e01661). Click here to learn what that means.

Files Patch % Lines
x/callback/types/msg.go 0.00% 5 Missing ⚠️
x/callback/keeper/keeper.go 0.00% 2 Missing and 1 partial ⚠️
x/callback/keeper/grpc_query.go 66.66% 1 Missing ⚠️
x/callback/keeper/msg_server.go 93.75% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #539   +/-   ##
=======================================
  Coverage        ?   64.62%           
=======================================
  Files           ?       88           
  Lines           ?     4851           
  Branches        ?        0           
=======================================
  Hits            ?     3135           
  Misses          ?     1550           
  Partials        ?      166           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@spoo-bar spoo-bar marked this pull request as ready for review February 1, 2024 10:33
@spoo-bar spoo-bar requested review from a team and fdymylja February 1, 2024 10:33
Comment thread CHANGELOG.md Outdated
spoo-bar and others added 2 commits February 1, 2024 12:24
Co-authored-by: Augusto Elesbão <aelesbao@users.noreply.github.com>
Signed-off-by: Spoorthi <9302666+spoo-bar@users.noreply.github.com>
@spoo-bar spoo-bar changed the title fix(x/callback): audit remediations fix(x/callback): audit remidiations Feb 1, 2024

@fdymylja fdymylja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 🙏

@spoo-bar spoo-bar merged commit 83424ff into main Feb 5, 2024
@spoo-bar spoo-bar deleted the spoorthi/audit-remediations branch February 5, 2024 09:50
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.

3 participants