Problem: improve permission checking in a few messages #1256
Conversation
yihuang
requested review from
mmsqe and
thomas-nguy
and removed request for
a team
WalkthroughThe recent updates focus on enhancing security by improving permission checks across various message processing functions. The changes include the removal of certain imports, reorganization of functions, and the expansion of permission checks to handle multiple accounts. These modifications aim to bolster the robustness of the system against unauthorized access and actions. Changes
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 X ? TipsChat with CodeRabbit Bot (
|
Signed-off-by: yihuang <huang@crypto.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- app/ante/ante.go (2 hunks)
- app/app.go (3 hunks)
- x/cronos/keeper/msg_server.go (2 hunks)
- x/cronos/keeper/permissions.go (1 hunks)
Additional comments: 8
app/ante/ante.go (1)
- 15-15: Clarify the use of
evmante.Recoverand confirm if it's the appropriate way to handle errors in this context. It's important to ensure that this recovery pattern is consistent with the rest of the codebase and aligns with the error handling strategy.app/app.go (1)
- 953-960: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [935-960]
The changes in the
setAnteHandlerfunction include the addition of a newoptionsstruct for theevmante.HandlerOptionsand the creation of a newanteHandlerusing theevmantepackage. This aligns with the PR's objective to enhance permission checking mechanisms by potentially using a more robust ante handler from theevmantepackage.However, there is a potential issue with the
blockAddressDecoratorbeing added to theExtraDecoratorsslice. If theblockAddressDecoratoris intended to block certain addresses from sending transactions, this logic should be thoroughly tested to ensure it does not inadvertently block addresses that should be allowed to transact.Additionally, the
optionsstruct is populated with various keepers and handlers, which suggests a significant change in how the ante handler processes transactions. It is crucial to ensure that all new logic introduced here is covered by unit tests to prevent regressions and unintended side effects.x/cronos/keeper/msg_server.go (3)
66-77: The addition of permission checks in the
UpdateTokenMappingmethod aligns with the PR's objective to enhance permission checking mechanisms. This is a critical security improvement ensuring that only authorized users can update token mappings.82-93: The addition of permission checks in the
TurnBridgemethod is consistent with the PR's objective to enhance permission checking mechanisms. This ensures that only authorized users can enable or disable the bridge.71-71: Verify that the constants
CanChangeTokenMappingandCanTurnBridgeused for permission checks are correctly defined and consistently used throughout the codebase.x/cronos/keeper/permissions.go (3)
31-47: The updated
HasPermissionfunction correctly iterates over the provided slice ofsdk.AccAddressand checks if any of the accounts have the specified permissions. This change enhances the flexibility of permission checking by allowing multiple accounts to be verified in a single call. Ensure that all calls to this function have been updated to pass a slice of addresses where necessary.36-39: The check for the
adminaccount within the provided accounts is a critical security operation. It is important to ensure that the comparison between theadminaccount and each account in the slice is secure and accurate. The current implementation usesaccount.String()for comparison, which should be consistent with how theCronosAdminis stored and compared throughout the codebase.47-47: The function correctly returns
falseif none of the provided accounts have the required permissions, which is the expected behavior for a permission check function.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/cronos/keeper/msg_server.go (3 hunks)
- x/cronos/keeper/permissions_test.go (1 hunks)
- x/cronos/simulation/operations.go (1 hunks)
Additional comments: 8
x/cronos/keeper/msg_server.go (3)
66-77: The addition of permission checks in the
UpdateTokenMappingmethod aligns with the PR's objective to enhance security by refining permission checking mechanisms. The use ofHasPermissionwithCanChangeTokenMappingis appropriate for the context.82-93: The addition of permission checks in the
TurnBridgemethod is consistent with the PR's goal to improve security. The use ofHasPermissionwithCanTurnBridgeensures that only authorized entities can execute the action.113-119: The permission check in the
UpdatePermissionsmethod uses a direct address comparison instead of theHasPermissionfunction. This is a deviation from the pattern used in other methods. Confirm if this is intentional and aligns with the desired permission model.x/cronos/keeper/permissions_test.go (4)
44-44: The update to
cosmosAddressfrom a singlesdk.AccAddressto a slice ofsdk.AccAddressis consistent with the PR's objective to enhance security by refining permission checks.52-52: The
SetPermissionsmethod call has been correctly updated to passcosmosAddress[0]instead ofcosmosAddress, aligning with the method's expectation of a singlesdk.AccAddress.53-53: The
HasPermissionmethod call correctly passescosmosAddressas a slice, reflecting the updated function signature that now accepts a slice ofsdk.AccAddress.57-57: This assertion seems incorrect. After setting the
CanTurnBridgepermission, the test should still expectCanChangeTokenMappingto be true if theAllpermission includes it. This needs to be verified against the permission logic to ensure the test reflects the intended behavior.x/cronos/simulation/operations.go (1)
- 94-95: The error handling logic appears to be correct, but verify that the error message "msg sender is not authorized" is consistent with the rest of the codebase.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1256 +/- ##
===========================================
+ Coverage 16.04% 35.83% +19.78%
===========================================
Files 80 115 +35
Lines 6196 10647 +4451
===========================================
+ Hits 994 3815 +2821
- Misses 5122 6456 +1334
- Partials 80 376 +296
|
Signed-off-by: mmsqe <mavis@crypto.com>
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Security Enhancements
Bug Fixes
TransferTokens,UpdateTokenMapping, andTurnBridgemethods to ensure proper authorization.Refactor
HasPermissionfunction to handle multiple accounts, improving flexibility in permission checks.Documentation