feat: audit #354
feat: audit #354
Conversation
…lance at init genesis
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adjusts several reward allocation and error handling routines across the distribution and move modules. In the distribution module, the token allocation now uses truncated division, and the genesis balance check has been relaxed. In the move module, the initialization no longer processes execution responses while dispatch and view functions receive improved error handling using type checks and an infinite gas context. The connector’s pool weight function is also modified to return valid weights without triggering an error for timestamp discrepancies. Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
x/distribution/keeper/genesis.go (1)
126-132: Relaxed module account balance validation.The validation check has been changed to only verify that the module account balance is greater than or equal to the expected holdings, rather than requiring an exact match. This accommodates potential external transfers into the module account.
This is a reasonable approach given the challenges with blocking fungible asset transfers. However, ensure there are regular audits to detect any unexpected discrepancies in balances that might indicate issues beyond normal operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/distribution/keeper/allocation.go(1 hunks)x/distribution/keeper/genesis.go(1 hunks)x/move/keeper/genesis.go(1 hunks)x/move/keeper/handler.go(2 hunks)x/move/types/connector.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Initiad
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
x/move/types/connector.go (1)
414-415:✅ Verification successful
Improved error handling for timestamp inconsistencies.
The code now returns valid weight values instead of an error when
timestampBeforeis greater thantimestamp. This change makes the function more robust by handling potential timestamp inconsistencies gracefully.Run this script to verify how these weights are used in practice:
🏁 Script executed:
#!/bin/bash # Check where GetPoolWeights is called and how errors are handled rg -A 3 -B 3 "GetPoolWeights" --type goLength of output: 1283
Notice: Timestamp Handling Update Verified
The changes atx/move/types/connector.go(lines 414–415) now return the pre-calculated weights (weightCoinABeforeandweightCoinBBefore) with a nil error whentimestampBeforeis greater thantimestamp. Verification shows that downstream calls (e.g., inTest_GetDexWeightinconnector_test.go) expect a valid weight value and handle the nil error appropriately. This update improves robustness by gracefully managing timestamp inconsistencies without propagating an error.x/move/keeper/handler.go (4)
394-400: Enhanced panic handling for gas-related errors.This change improves error handling by differentiating between out-of-gas errors and other panic types, allowing appropriate propagation of gas errors while safely handling other panic scenarios.
589-591: Account tracking added to view function execution.Adding account number tracking alongside execution counter provides better context for audit purposes. The variable naming change from
executionCountertoecalso improves code consistency.
599-601: Verify the implications of using an infinite gas meter for view functions.While delegating gas metering to the Move VM makes sense, using an infinite gas meter for view functions could potentially allow resource-intensive operations without proper constraints.
Can you confirm that the Move VM properly handles gas accounting internally for view functions? Otherwise, this approach could expose the system to potential DoS vulnerabilities if complex view functions consume excessive resources.
605-608: Context enrichment for better auditing.Passing account number and execution counter to the execution environment enhances the audit trail for view function calls, which aligns with the PR's audit feature objectives.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
- Coverage 41.21% 41.18% -0.03%
==========================================
Files 269 269
Lines 25761 25783 +22
==========================================
+ Hits 10617 10620 +3
- Misses 13498 13515 +17
- Partials 1646 1648 +2
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/ibc-hooks/move-hooks/receive.go (4)
24-29: Enhanced error handling for memo validation in ICS-20 packet processingThis change improves the conditional logic to properly handle error cases during memo validation. Now, the function only passes the packet to the standard processing path when either it's not Move-routed OR (the validation produced no error AND there's no Move message). This ensures that invalid memo data with errors won't be incorrectly processed.
While the current implementation is correct, consider extracting this flow control logic into a helper function since it's duplicated in both packet handlers:
+ func shouldUseDefaultHandler(isMoveRouted bool, err error, hookData *HookData) bool { + return !isMoveRouted || (err == nil && hookData != nil && hookData.Message == nil) + } func (h MoveHooks) onRecvIcs20Packet( // ...existing params... ) ibcexported.Acknowledgement { isMoveRouted, hookData, err := validateAndParseMemo(data.GetMemo()) - if !isMoveRouted || (err == nil && hookData.Message == nil) { + if shouldUseDefaultHandler(isMoveRouted, err, hookData) { return im.App.OnRecvPacket(ctx, packet, relayer) } else if err != nil { return newEmitErrorAcknowledgement(err) }
31-66: Consider adding audit logging for successful Move hook executionsGiven that this PR is focused on adding an audit feature (as mentioned in the PR objectives), and considering this is a critical path for interchain communications, it would be beneficial to add audit logging for successful executions of Move hooks when processing ICS-20 packets.
Consider adding audit logging after successful execution:
_, err = h.execMsg(ctx, msg) if err != nil { return newEmitErrorAcknowledgement(err) } + + // Add audit logging for successful hook execution + ctx.Logger().Info("ICS-20 packet processed with Move hook", + "sender", intermediateSender, + "module_address", msg.ModuleAddress, + "channel", packet.GetDestChannel()) return ack
112-117: Consider adding audit logging for successful Move hook executions in ICS-721 handlerSimilar to the ICS-20 handler, this function should also include audit logging for successful executions to maintain consistency and improve traceability of NFT-related operations.
Consider adding audit logging after successful execution:
_, err = h.execMsg(ctx, msg) if err != nil { return newEmitErrorAcknowledgement(err) } + + // Add audit logging for successful hook execution + ctx.Logger().Info("ICS-721 packet processed with Move hook", + "sender", intermediateSender, + "module_address", msg.ModuleAddress, + "channel", packet.GetDestChannel(), + "class_id", data.ClassId) return ack
121-133: Consider adding execution metrics for monitoring and auditingTo further enhance the audit capabilities of this system, consider instrumenting the
execMsgfunction with metrics that track execution counts, success rates, and execution times. This would be valuable for operational monitoring and auditing.func (h MoveHooks) execMsg(ctx sdk.Context, msg *movetypes.MsgExecute) (*movetypes.MsgExecuteResponse, error) { + start := time.Now() + defer func() { + h.metrics.ObserveExecutionTime(time.Since(start)) + }() if err := msg.Validate(h.ac); err != nil { + h.metrics.IncrementFailedExecutions(msg.ModuleAddress) return nil, err } moveMsgServer := movekeeper.NewMsgServerImpl(h.moveKeeper) res, err := moveMsgServer.Execute(ctx, msg) if err != nil { + h.metrics.IncrementFailedExecutions(msg.ModuleAddress) return nil, err } + h.metrics.IncrementSuccessfulExecutions(msg.ModuleAddress) return res, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/ibc-hooks/move-hooks/receive.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run test and upload codecov
- GitHub Check: golangci-lint
- GitHub Check: Initiad
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
x/ibc-hooks/move-hooks/receive.go (1)
76-81: Enhanced error handling for memo validation in ICS-721 packet processingThis change applies the same improved error handling logic to the non-fungible token packet processing path. The function now correctly ensures that errors from memo validation are properly handled before proceeding with any processing.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...