feat(evm): ante handler to prohibit authz grant evm messages #2032
Conversation
|
Warning Rate limit exceeded@onikonychev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 58 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a new authorization guard in the EVM module to prevent unauthorized authorization grant messages. This is achieved through the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
==========================================
- Coverage 65.58% 65.57% -0.02%
==========================================
Files 264 265 +1
Lines 16653 16689 +36
==========================================
+ Hits 10922 10943 +21
- Misses 4846 4858 +12
- Partials 885 888 +3
|
app/ante/authz_guard.go
Outdated
| var genericAuthTypeTurl = "/" + proto.MessageName(&authz.GenericAuthorization{}) | ||
|
|
||
| // AnteDecoratorAuthzGuard filters autz messages | ||
| type AnteDecoratorAuthzGuard struct{} | ||
|
|
||
| // AnteHandle rejects "authz grant generic --msg-type '/eth.evm.v1.MsgEthereumTx'" | ||
| func (rmd AnteDecoratorAuthzGuard) AnteHandle( | ||
| ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler, | ||
| ) (newCtx sdk.Context, err error) { | ||
| for _, msg := range tx.GetMsgs() { | ||
| if msgGrant, ok := msg.(*authz.MsgGrant); ok { |
There was a problem hiding this comment.
Revoke seems harmless. I would also remove Exec as well to address this vulnerability from Jump.
There was a problem hiding this comment.
Added rejection for exec of MsgEthereumTx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
app/ante.go (1)
65-65: Add documentation explaining the rationale for the guard.Please add comments or documentation explaining the necessity of the
ante.AnteDecoratorAuthzGuard{}to ensure that users understand the risks involved with the authz module. This was one of the objectives mentioned in the linked issue #1915.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- app/ante.go (1 hunks)
- app/ante/auth_grard_test.go (1 hunks)
- app/ante/authz_guard.go (1 hunks)
Additional comments not posted (4)
app/ante/authz_guard.go (1)
20-51: LGTM!The
AnteHandlefunction correctly rejects the granting of the message typeMsgEthereumTxwithin the authorization (authz) module. This change enhances security by preventing the generic permission granting for Ethereum transaction messages.app/ante/auth_grard_test.go (1)
18-98: LGTM!The test function
TestAnteDecoratorAuthzGuardis well-structured and covers important scenarios. The test cases are clearly defined and the expected behavior is specified. The test function uses good practices such as:
- Using
s.Runto run each test case as a sub-test for better isolation and reporting.- Using
s.Requireassertions to check the expected behavior.- Using
evmtest.NewTestDepsto create a new set of dependencies for each test case to ensure a clean state.- Using
sdkclienttx.Factoryto create a new unsigned transaction for each test case.- Using
app.MakeEncodingConfigto create a new encoding configuration for each test case to ensure a clean state.- Using
AnteHandlemethod of theAnteDecoratorAuthzGuardante decorator to handle the transaction, which is the main functionality being tested.- Checking the error returned by the
AnteHandlemethod and comparing it with the expected error.Overall, the test function is well-written and thoroughly tests the
AnteDecoratorAuthzGuardante decorator.app/ante.go (1)
65-65: LGTM!The
ante.AnteDecoratorAuthzGuard{}is correctly added to the ante handler chain for non-EVM transactions. This change aligns with the PR objective of implementing an "authz guard" to restrict the granting of certain message types, specifically Ethereum transactions, to mitigate potential security risks associated with the authz module.CHANGELOG.md (1)
122-122: LGTM, the ante handler feature looks good!The new ante handler to prohibit authz grant EVM messages enhances security as intended.
To confirm the ante handler is properly integrated, run this script to search for the handler registration:
Verification successful
Ante handler integration verified successfully.
The new ante handler to prohibit authz grant EVM messages is properly integrated into the application. The
AnteDecoratorAuthzGuardis included in theNewAnteHandlerNonEVMfunction, ensuring the intended functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the ante handler is properly wired in the app. # Expect: A call to `app.SetAnteHandler` with the new handler. rg --type go --context 5 --ignore-case 'SetAnteHandler\('Length of output: 518
Script:
#!/bin/bash # Description: Search for the definition or assignment of `anteHandler` to verify it includes the new ante handler. rg --type go --context 5 --ignore-case 'anteHandler'Length of output: 41220
app/ante/authz_guard.go
Outdated
| var genericAuthTypeTurl = "/" + proto.MessageName(&authz.GenericAuthorization{}) | ||
|
|
||
| // AnteDecoratorAuthzGuard filters autz messages | ||
| type AnteDecoratorAuthzGuard struct{} | ||
|
|
||
| // AnteHandle rejects "authz grant generic --msg-type '/eth.evm.v1.MsgEthereumTx'" | ||
| func (rmd AnteDecoratorAuthzGuard) AnteHandle( | ||
| ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler, | ||
| ) (newCtx sdk.Context, err error) { | ||
| for _, msg := range tx.GetMsgs() { | ||
| if msgGrant, ok := msg.(*authz.MsgGrant); ok { |
There was a problem hiding this comment.
Consider removing Exec as well to address the vulnerability.
The past review comments suggest removing Exec as well to address this vulnerability from Jump.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
123-124: Approved, but consider providing more context.The refactors to remove unnecessary code and mocks from the eth/rpc/backend are good changes that will help with maintainability. However, consider providing a bit more rationale in the future for why the code/mocks were unnecessary and how it improves things. More context can help the reader understand the motivations and impact better.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (2)
CHANGELOG.md (2)
122-122: LGTM!The changelog entry is clear and the feature makes sense from a security perspective.
125-125: Excellent!Adding more realistic backend tests using a test network and real transactions is a great way to expand coverage and catch bugs. Nice work.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
app/ante/authz_guard.go (4)
12-12: Typo in comment: 'autz' should be 'authz'There's a typo in the comment on line 12. It should be "filters authz messages" instead of "filters autz messages".
23-27: Use a more appropriate error type when authorization is missingWhen
msgGrant.Grant.Authorizationisnil, the code returns anErrInvalidTypeerror. Consider usingerrortypes.ErrInvalidRequestto more accurately represent a missing required field.
31-35: Clarify error message for unmarshaling authorizationThe error message "failed unmarshaling generic authorization %s" may be misleading if the authorization is not a
GenericAuthorization. Consider rephrasing it to "failed unmarshaling authorization: %s" for clarity.
58-60: UseErrNotSupportedwhen rejectingMsgEthereumTxinMsgExecCurrently, when rejecting a
MsgEthereumTxwithin aMsgExec, the code returns anErrInvalidTypeerror. Since this is a policy decision to not supportMsgEthereumTxinMsgExec, usingerrortypes.ErrNotSupportedwould be more appropriate.
| func (rmd AnteDecoratorAuthzGuard) AnteHandle( | ||
| ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler, | ||
| ) (newCtx sdk.Context, err error) { | ||
| for _, msg := range tx.GetMsgs() { | ||
| // Do not allow grant for MsgEthereumTx | ||
| if msgGrant, ok := msg.(*authz.MsgGrant); ok { | ||
| if msgGrant.Grant.Authorization == nil { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrInvalidType, | ||
| "grant authorization is missing", | ||
| ) | ||
| } | ||
| authorization, err := msgGrant.Grant.GetAuthorization() | ||
| if err != nil { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrInvalidType, | ||
| "failed unmarshaling generic authorization %s", err, | ||
| ) | ||
| } | ||
| if genericAuth, ok := authorization.(*authz.GenericAuthorization); ok { | ||
| if genericAuth.MsgTypeURL() == sdk.MsgTypeURL(&evm.MsgEthereumTx{}) { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrNotSupported, | ||
| "authz grant generic for msg type %s is not allowed", | ||
| genericAuth.MsgTypeURL(), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| // Also reject MsgEthereumTx in exec | ||
| if msgExec, ok := msg.(*authz.MsgExec); ok { | ||
| msgsInExec, err := msgExec.GetMessages() | ||
| if err != nil { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrInvalidType, | ||
| "failed getting exec messages %s", err, | ||
| ) | ||
| } | ||
| for _, msgInExec := range msgsInExec { | ||
| if _, ok := msgInExec.(*evm.MsgEthereumTx); ok { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrInvalidType, | ||
| "MsgEthereumTx needs to be contained within a tx with 'ExtensionOptionsEthereumTx' option", | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return next(ctx, tx, simulate) | ||
| } |
There was a problem hiding this comment.
Recommend adding unit tests for AnteDecoratorAuthzGuard
To ensure that AnteDecoratorAuthzGuard works as intended, please consider adding unit tests covering:
- Attempting to grant authorization for
MsgEthereumTxand expecting rejection. - Executing a
MsgExeccontainingMsgEthereumTxand verifying it is rejected. - Confirming that valid
authzmessages not involvingMsgEthereumTxare processed correctly.
| genericGrant, err := authz.NewGrant( | ||
| someTime, | ||
| authz.NewGenericAuthorization(sdk.MsgTypeURL(&evm.MsgEthereumTx{})), &expiryTime, | ||
| ) |
There was a problem hiding this comment.
Incorrect argument order in 'authz.NewGrant' function calls
The authz.NewGrant function expects the authorization as the first argument and expiration as the second argument. Currently, someTime (the current time) is being passed as the first argument, which is incorrect.
Apply these diffs to fix the argument order:
For lines 29-32:
genericGrant, err := authz.NewGrant(
- someTime,
authz.NewGenericAuthorization(sdk.MsgTypeURL(&evm.MsgEthereumTx{})), &expiryTime,
+)For lines 43-46:
genericGrant, err := authz.NewGrant(
- someTime,
authz.NewGenericAuthorization(sdk.MsgTypeURL(&stakingtypes.MsgCreateValidator{})), &expiryTime,
+)For lines 57-61:
genericGrant, err := authz.NewGrant(
- someTime,
&banktypes.SendAuthorization{},
&expiryTime,
+)Ensure that someTime is not passed to authz.NewGrant, as it is not a required parameter for this function.
Also applies to: 43-46, 57-61
| sdk.AccAddress("nibiuser"), | ||
| []sdk.Msg{ |
There was a problem hiding this comment.
Incorrect usage of sdk.AccAddress with string literals
Passing a string directly to sdk.AccAddress does not produce a valid address. sdk.AccAddress expects a byte slice. To create a valid address from a string, use sdk.AccAddressFromBech32 or convert the string to bytes.
Apply these diffs to correct the address usage:
For lines 78-79:
-msgExec := authz.NewMsgExec(
- sdk.AccAddress("nibiuser"),
+addr, err := sdk.AccAddressFromBech32("nibi1...") // Replace with a valid Bech32 address
+s.Require().NoError(err)
+msgExec := authz.NewMsgExec(
+ addr,For lines 90-91:
-msgExec := authz.NewMsgExec(
- sdk.AccAddress("nibiuser"),
+addr, err := sdk.AccAddressFromBech32("nibi1...") // Replace with a valid Bech32 address
+s.Require().NoError(err)
+msgExec := authz.NewMsgExec(
+ addr,For lines 104-105:
-msgExec := authz.NewMsgExec(
- sdk.AccAddress("nibiuser"),
+addr, err := sdk.AccAddressFromBech32("nibi1...") // Replace with a valid Bech32 address
+s.Require().NoError(err)
+msgExec := authz.NewMsgExec(
+ addr,Replace "nibi1..." with a valid Bech32 test address.
Also applies to: 90-91, 104-105
| for _, tc := range testCases { | ||
| s.Run(tc.name, func() { | ||
| deps := evmtest.NewTestDeps() | ||
| anteDec := ante.AnteDecoratorAuthzGuard{} | ||
|
|
||
| encCfg := app.MakeEncodingConfig() | ||
| txBuilder, err := sdkclienttx.Factory{}. | ||
| WithChainID(s.ctx.ChainID()). | ||
| WithTxConfig(encCfg.TxConfig). | ||
| BuildUnsignedTx(tc.txMsg()) | ||
| s.Require().NoError(err) | ||
|
|
||
| _, err = anteDec.AnteHandle( | ||
| deps.Ctx, txBuilder.GetTx(), false, evmtest.NextNoOpAnteHandler, | ||
| ) | ||
| if tc.wantErr != "" { | ||
| s.Require().ErrorContains(err, tc.wantErr) | ||
| return | ||
| } | ||
| s.Require().NoError(err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider refactoring to improve test readability and maintainability
The loop over test cases and the structure of the tests are good, but there's repetitive code within each test case, especially in the transaction building and ante handler invocation. Consider abstracting common setup steps into helper functions to reduce duplication and enhance readability.
Example:
-
Create a helper function to build transactions:
func buildTx(msg sdk.Msg) (sdk.Tx, error) { encCfg := app.MakeEncodingConfig() return sdkclienttx.Factory{}. WithChainID(s.ctx.ChainID()). WithTxConfig(encCfg.TxConfig). BuildUnsignedTx(msg) }
-
Use the helper function in your tests:
txBuilder, err := buildTx(tc.txMsg()) s.Require().NoError(err)
app/ante/auth_grard_test.go
Outdated
| "github.com/NibiruChain/nibiru/v2/x/evm/evmtest" | ||
| ) | ||
|
|
||
| func (s *AnteTestSuite) TestAnteDecoratorAuthzGuar() { |
There was a problem hiding this comment.
Typo in function name: Missing 'd' in 'AuthzGuard'
The function name TestAnteDecoratorAuthzGuar appears to have a typo. It should be TestAnteDecoratorAuthzGuard to accurately reflect the tested component and maintain consistency.
Apply this diff to correct the function name:
-func (s *AnteTestSuite) TestAnteDecoratorAuthzGuar() {
+func (s *AnteTestSuite) TestAnteDecoratorAuthzGuard() {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *AnteTestSuite) TestAnteDecoratorAuthzGuar() { | |
| func (s *AnteTestSuite) TestAnteDecoratorAuthzGuard() { |
Authz allows granting a generic permission for any message.
This ante handler disables granting the message type
MsgEthereumTx.So, now
nibid tx authz grant nibi1jle8khj3aennq24zx6g93aam9rt0fqhgyp4h52 generic --from validator --msg-type '/eth.evm.v1.MsgEthereumTx' --yesfails with:
Summary by CodeRabbit
New Features
Bug Fixes
Tests