v6 #573
v6 #573
Conversation
WalkthroughThis pull request represents a comprehensive upgrade of the Gravity Bridge module from version 5 to version 6. The changes primarily involve updating import paths across multiple files in the project, introducing a new upgrade handler for version 6, and making some minor modifications to error handling and fee calculation. The upgrade includes security fixes identified through a bug bounty program and introduces enhanced migration capabilities for handling Ethereum event vote records. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
module/x/gravity/keeper/migrations.go (2)
32-69: Validate usage oflowestNonce.
lowestNonceis tracked but never used after being set. Ensure there’s no forgotten logic or remove it if it’s no longer necessary.
80-88: Deduplicate logic is fine but consider factoring to a helper.
The loop to remove duplicate validators is sound. For maintainability, you could move deduplication into a small helper function and reuse it elsewhere if needed.module/x/gravity/keeper/batch.go (1)
55-68: Batch profitability check is sound.
Rejecting a new batch if the old batch has higher or equal fees is sensible. Consider optional logging to clarify why the new batch was not created.if lastBatchFees.GTE(totalFees) { + k.Logger(ctx).Info("Skipping new batch creation due to lower or equal fee profitability", "new_fees", totalFees.String(), "old_fees", lastBatchFees.String()) return nil }module/app/upgrades/v2/upgrades.go (1)
Line range hint
1-10: Review the necessity of v2 upgrade handler in v6.This file raises architectural concerns:
- It's marked with
excludebuild tag but still maintained- Historical v2 upgrade handler importing v6 types could cause confusion
- Consider moving historical upgrade handlers to a separate archive package
Consider either:
- Removing this file if it's no longer needed
- Moving it to an archive package with frozen v5 imports
- Adding clear documentation explaining why it's maintained with
excludetagmodule/x/gravity/module.go (1)
130-133: Consider simplifying the migration handler.The migration handler can be simplified by directly passing the migrator's
MigrateStoremethod.Apply this diff to simplify the code:
- if err := cfg.RegisterMigration(types.ModuleName, 5, func(ctx sdk.Context) error { - return migrator.MigrateStore(ctx) - }); err != nil { + if err := cfg.RegisterMigration(types.ModuleName, 5, migrator.MigrateStore); err != nil {🧰 Tools
🪛 golangci-lint (1.62.2)
132-132: unlambda: replace
func(ctx sdk.Context) error { return migrator.MigrateStore(ctx) }withmigrator.MigrateStore(gocritic)
module/x/gravity/keeper/migrations_test.go (2)
11-131: Consider refactoring test setup code.The test setup code for creating events and setting up vote records is duplicated across multiple test functions. Consider extracting this into helper functions to improve maintainability.
Example helper functions:
func createTestEvent(t *testing.T, nonce uint64, scope []byte) types.ContractCallExecutedEvent { return types.ContractCallExecutedEvent{ EventNonce: nonce, EthereumHeight: 99, InvalidationScope: scope, InvalidationNonce: 1, } } func setupVoteRecord(t *testing.T, gk Keeper, ctx sdk.Context, event types.ContractCallExecutedEvent, votes []string, accepted bool) { packedEvent, err := types.PackEvent(&event) require.NoError(t, err) gk.setEthereumEventVoteRecord(ctx, event.EventNonce, OldHash(&event), &types.EthereumEventVoteRecord{ Event: packedEvent, Votes: votes, Accepted: accepted, }) }
145-230: Consider using table-driven tests.The test cases for different voting patterns could be structured as a table-driven test to make it easier to add new test cases.
Example refactor:
func TestDeletePendingEventVoteRecordsPartialVotes(t *testing.T) { tests := []struct { name string setupVotes func(ctx sdk.Context, gk Keeper) expectedNonce uint64 expectDeleted []uint64 }{ { name: "approved event only", setupVotes: func(ctx sdk.Context, gk Keeper) { // setup approved event }, expectedNonce: 5, expectDeleted: []uint64{}, }, { name: "partial votes", setupVotes: func(ctx sdk.Context, gk Keeper) { // setup partial votes }, expectedNonce: 5, expectDeleted: []uint64{6, 7}, }, // Add more test cases } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { env := CreateTestEnv(t) gk := env.GravityKeeper tc.setupVotes(env.Context, gk) migrator := NewMigrator(gk) err := migrator.DeletePendingEventVoteRecords(env.Context) require.NoError(t, err) require.Equal(t, tc.expectedNonce, gk.GetLastObservedEventNonce(env.Context)) for _, nonce := range tc.expectDeleted { require.Nil(t, gk.GetEthereumEventVoteRecordMapping(env.Context)[nonce]) } }) } }integration_tests/ethereum/Dockerfile (1)
1-1: Consider using an LTS version of Node.js.The update to
node:23.6-alpine3.21uses a non-LTS version of Node.js. Consider using an LTS version (like Node.js 20) for better long-term stability and support.-FROM node:23.6-alpine3.21 +FROM node:20-alpine3.21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
go.work.sumis excluded by!**/*.sum,!**/*.sumintegration_tests/ethereum/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lockmodule/go.modis excluded by!**/*.modmodule/go.sumis excluded by!**/*.sum,!**/*.summodule/proto/buf.lockis excluded by!**/*.lock,!**/*.lockmodule/x/gravity/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gomodule/x/gravity/types/gravity.pb.gois excluded by!**/*.pb.go,!**/*.pb.gomodule/x/gravity/types/msgs.pb.gois excluded by!**/*.pb.go,!**/*.pb.gomodule/x/gravity/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (61)
integration_tests/chain.go(1 hunks)integration_tests/ethereum.go(1 hunks)integration_tests/ethereum/Dockerfile(1 hunks)integration_tests/happy_path_test.go(1 hunks)integration_tests/setup_test.go(1 hunks)integration_tests/transaction_stress_test.go(1 hunks)integration_tests/validator.go(1 hunks)integration_tests/validator_out.go(1 hunks)integration_tests/valset_stress_test.go(1 hunks)module/app/app.go(2 hunks)module/app/encoding.go(1 hunks)module/app/upgrades/v2/upgrades.go(1 hunks)module/app/upgrades/v2/upgrades_test.go(1 hunks)module/app/upgrades/v6/README.md(1 hunks)module/app/upgrades/v6/constants.go(1 hunks)module/app/upgrades/v6/upgrades.go(1 hunks)module/cmd/gravity/cmd/gentx.go(1 hunks)module/cmd/gravity/cmd/root.go(1 hunks)module/cmd/gravity/cmd/root_test.go(1 hunks)module/cmd/gravity/main.go(1 hunks)module/proto/gravity/v1/genesis.proto(1 hunks)module/proto/gravity/v1/gravity.proto(1 hunks)module/proto/gravity/v1/msgs.proto(1 hunks)module/proto/gravity/v1/query.proto(1 hunks)module/x/gravity/abci.go(1 hunks)module/x/gravity/abci_test.go(3 hunks)module/x/gravity/client/cli/query.go(1 hunks)module/x/gravity/client/cli/tx.go(1 hunks)module/x/gravity/client/cli/utils.go(1 hunks)module/x/gravity/client/proposal_handler.go(1 hunks)module/x/gravity/cosmos_originated_test.go(1 hunks)module/x/gravity/handler.go(1 hunks)module/x/gravity/handler_test.go(1 hunks)module/x/gravity/keeper/batch.go(2 hunks)module/x/gravity/keeper/batch_test.go(4 hunks)module/x/gravity/keeper/contract_call.go(1 hunks)module/x/gravity/keeper/contract_call_test.go(1 hunks)module/x/gravity/keeper/cosmos_originated.go(1 hunks)module/x/gravity/keeper/ethereum_event_handler.go(1 hunks)module/x/gravity/keeper/ethereum_event_vote.go(1 hunks)module/x/gravity/keeper/genesis.go(2 hunks)module/x/gravity/keeper/grpc_query.go(1 hunks)module/x/gravity/keeper/grpc_query_test.go(1 hunks)module/x/gravity/keeper/hooks.go(1 hunks)module/x/gravity/keeper/keeper.go(1 hunks)module/x/gravity/keeper/keeper_test.go(1 hunks)module/x/gravity/keeper/migrations.go(2 hunks)module/x/gravity/keeper/migrations_test.go(1 hunks)module/x/gravity/keeper/msg_server.go(1 hunks)module/x/gravity/keeper/msg_server_test.go(1 hunks)module/x/gravity/keeper/pool.go(1 hunks)module/x/gravity/keeper/pool_test.go(1 hunks)module/x/gravity/keeper/proposal_handler.go(1 hunks)module/x/gravity/keeper/signer_set.go(1 hunks)module/x/gravity/keeper/signer_set_test.go(2 hunks)module/x/gravity/keeper/test_common.go(2 hunks)module/x/gravity/module.go(3 hunks)module/x/gravity/types/ethereum_event.go(5 hunks)module/x/gravity/types/msgs_test.go(1 hunks)module/x/gravity/types/types.go(1 hunks)orchestrator/Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (40)
- module/app/upgrades/v6/README.md
- module/app/upgrades/v6/constants.go
- module/cmd/gravity/cmd/root_test.go
- module/x/gravity/keeper/ethereum_event_handler.go
- module/proto/gravity/v1/gravity.proto
- module/app/encoding.go
- module/x/gravity/keeper/grpc_query.go
- integration_tests/ethereum.go
- module/x/gravity/client/cli/tx.go
- module/x/gravity/keeper/contract_call_test.go
- module/x/gravity/keeper/grpc_query_test.go
- module/x/gravity/keeper/msg_server.go
- integration_tests/setup_test.go
- module/x/gravity/keeper/signer_set.go
- orchestrator/Dockerfile
- integration_tests/valset_stress_test.go
- module/x/gravity/keeper/ethereum_event_vote.go
- module/x/gravity/keeper/msg_server_test.go
- module/proto/gravity/v1/genesis.proto
- module/x/gravity/keeper/hooks.go
- module/x/gravity/keeper/cosmos_originated.go
- module/x/gravity/keeper/proposal_handler.go
- module/cmd/gravity/main.go
- module/x/gravity/keeper/keeper_test.go
- module/x/gravity/keeper/pool.go
- module/app/upgrades/v2/upgrades_test.go
- module/x/gravity/types/msgs_test.go
- integration_tests/validator_out.go
- module/cmd/gravity/cmd/root.go
- integration_tests/validator.go
- module/x/gravity/keeper/keeper.go
- module/proto/gravity/v1/msgs.proto
- integration_tests/happy_path_test.go
- module/x/gravity/abci.go
- module/x/gravity/handler.go
- module/proto/gravity/v1/query.proto
- module/x/gravity/client/cli/utils.go
- module/x/gravity/client/cli/query.go
- module/x/gravity/cosmos_originated_test.go
- module/x/gravity/handler_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
module/x/gravity/module.go
132-132: unlambda: replace func(ctx sdk.Context) error { return migrator.MigrateStore(ctx) } with migrator.MigrateStore
(gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: rust-test
- GitHub Check: rust-build
- GitHub Check: local-tests (16.x)
- GitHub Check: go-build
🔇 Additional comments (30)
module/x/gravity/keeper/migrations.go (2)
3-12: Imports updated to v6 successfully.
No issues spotted with these import statements. The addition ofv6references is consistent with the PR’s upgrade theme.
90-108: Confirm validity ofValidatoraddresses before panicking.
Currently, any Bech32 parsing error results in a panic. Consider more graceful handling (e.g., skipping or logging).module/x/gravity/keeper/batch.go (5)
13-13: Import path update aligns with v6 transition.
No concerns here; the path change is consistent with the Gravity Bridge upgrade.
21-23: Overflow prevention comment is clearly stated.
The added doc lines clarify the new logic to handle potential overflows.
38-53: Sorting by fee ensures maximum profitability.
This approach to prioritize higher fees is correct. The integer overflow check withSafeAddis a good safeguard.
71-74: Deleting newly batched transactions is correct.
Removing these from the unbatched pool prevents duplication. The approach looks well-structured.
79-79: Batch creation and persistence.
Storing the finalized transactions and incrementing the batch nonce is logically consistent with the new flow.module/x/gravity/client/proposal_handler.go (1)
5-5: Import path upgraded successfully.
Switching from v5 to v6 for thecliimport is aligned with the rest of the PR updates.module/x/gravity/keeper/pool_test.go (1)
10-10: LGTM!The import path update is consistent with the v6 upgrade, and test cases remain valid.
module/x/gravity/keeper/contract_call.go (1)
11-11: Verify security fixes mentioned in PR description.The PR description mentions security fixes from a bug bounty program, but this security-sensitive file only has import path updates. Please verify:
- If any security fixes were needed in contract call handling
- If the fixes are implemented elsewhere
- If this code was audited for v6
Run this script to locate security-related changes:
✅ Verification successful
Contract call handling remains secure and unchanged
The verification shows that contract_call.go only has an import path update from v5 to v6, with no functional changes needed. The codebase maintains existing security measures including overflow protections in related components. The version bump appears to be for API compatibility rather than security fixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for security-related changes in v6 echo "Searching for security fixes..." rg -l "v5" . | xargs -I {} git diff origin/main -- {} | grep -B2 -A2 "security" echo "Checking for new security tests..." rg -l "test.*security|security.*test" . echo "Checking for security-related commits..." git log --grep="security" --since="1 month ago"Length of output: 575
Script:
#!/bin/bash # Search for contract call related changes echo "=== Checking contract call changes ===" git diff origin/main -- module/x/gravity/keeper/contract_call.go echo -e "\n=== Checking PR description ===" gh pr view echo -e "\n=== Searching for vulnerability terms ===" rg -i "vulnerability|exploit|attack|overflow|underflow" module/x/gravity/keeper/Length of output: 1541
module/x/gravity/types/types.go (1)
126-136: LGTM! Security improvement in fee calculation.The changes enhance security by properly handling potential integer overflow in fee calculations using
SafeAdd. The added error handling ensures that any overflow is caught and propagated.module/x/gravity/keeper/signer_set_test.go (1)
87-142: LGTM! Comprehensive test coverage for signer set ordering.The test suite thoroughly covers all scenarios including edge cases:
- Normal ordering of different nonces
- Empty and nil slice handling
- Single element case
- Duplicate nonces
module/x/gravity/types/ethereum_event.go (1)
38-38: LGTM! Enhanced event hash uniqueness.Added unique string identifiers to each event type's hash computation, preventing potential hash collisions between different event types. The change is consistently applied across all event types:
- SendToCosmosEvent
- BatchExecutedEvent
- ContractCallExecutedEvent
- ERC20DeployedEvent
- SignerSetTxExecutedEvent
Also applies to: 55-55, 70-70, 85-85, 103-103
integration_tests/transaction_stress_test.go (1)
13-13: LGTM! Module version update.Import path updated from v5 to v6 as part of the module version upgrade. Test logic remains unchanged, indicating backward compatibility.
module/x/gravity/module.go (2)
21-23: LGTM!Import paths correctly updated to v6, maintaining consistency with the module version upgrade.
111-111: LGTM!ConsensusVersion correctly incremented to match the module version upgrade.
module/x/gravity/keeper/genesis.go (2)
10-10: LGTM!Import path correctly updated to v6, maintaining consistency with the module version upgrade.
31-31: LGTM!Added informative logging that helps track event processing during genesis initialization.
integration_tests/chain.go (1)
42-45: LGTM!Import paths correctly updated to v6, maintaining consistency with the module version upgrade.
module/x/gravity/keeper/migrations_test.go (2)
133-143: LGTM!Good edge case coverage testing empty state scenario.
232-335: LGTM!Comprehensive test coverage for handling active vs inactive validators, with proper verification of state changes.
module/cmd/gravity/cmd/gentx.go (1)
40-40: LGTM!The import path has been correctly updated to use version 6 of the Gravity Bridge module.
module/x/gravity/keeper/batch_test.go (2)
12-12: LGTM!The import path has been correctly updated to use version 6 of the Gravity Bridge module.
441-497: Great addition of comprehensive test cases!The new test function thoroughly covers batch ordering scenarios including:
- Normal ordering
- Empty slice handling
- Nil slice handling
- Single element case
- Duplicate nonce handling
This enhances the test coverage and helps ensure the robustness of the batch ordering functionality.
module/x/gravity/abci_test.go (2)
15-17: LGTM!The import paths have been correctly updated to use version 6 of the Gravity Bridge module.
407-407: Test refactoring improves clarity.The test modifications:
- Use the new
AddSendToEthTxsToPoolWithFeehelper for cleaner test setup- Adjust batch sizes to test different scenarios more explicitly
These changes maintain test coverage while making the test more maintainable.
Also applies to: 413-414, 433-434
module/x/gravity/keeper/test_common.go (2)
62-62: LGTM!The import paths have been correctly updated to use version 6 of the Gravity Bridge module.
Also applies to: 65-65
237-244: Well-structured test helper addition.The new
AddSendToEthTxsToPoolWithFeemethod provides a cleaner way to create test transactions with consistent fees. This improves test maintainability and readability.module/app/app.go (2)
97-103: LGTM! Import paths correctly updated to v6.All gravity-related imports have been consistently updated from v5 to v6, aligning with the module upgrade objective.
934-941: LGTM! Upgrade handler for v6 properly configured.The v6 upgrade handler is correctly registered while preserving the v5 handler, ensuring proper upgrade path management.
| func (m Migrator) MigrateStore(ctx sdk.Context) error { | ||
| ctx.Logger().Info("gravity: Migrating store") | ||
|
|
||
| m.DeletePendingEventVoteRecords(ctx) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Consider returning or handling DeletePendingEventVoteRecords error.
DeletePendingEventVoteRecords returns an error, but it is not captured here. It may be safer to handle or propagate the returned error instead of ignoring it.
func (m Migrator) MigrateStore(ctx sdk.Context) error {
ctx.Logger().Info("gravity: Migrating store")
- m.DeletePendingEventVoteRecords(ctx)
+ if err := m.DeletePendingEventVoteRecords(ctx); err != nil {
+ return err
+ }
return nil
}📝 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 (m Migrator) MigrateStore(ctx sdk.Context) error { | |
| ctx.Logger().Info("gravity: Migrating store") | |
| m.DeletePendingEventVoteRecords(ctx) | |
| return nil | |
| } | |
| func (m Migrator) MigrateStore(ctx sdk.Context) error { | |
| ctx.Logger().Info("gravity: Migrating store") | |
| if err := m.DeletePendingEventVoteRecords(ctx); err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
| func OldHash(event types.EthereumEvent) tmbytes.HexBytes { | ||
| switch e := event.(type) { | ||
| case *types.SendToCosmosEvent: | ||
| return HashSendToCosmosEvent(event.(*types.SendToCosmosEvent)) | ||
| case *types.BatchExecutedEvent: | ||
| return HashBatchExecutedEvent(event.(*types.BatchExecutedEvent)) | ||
| case *types.ContractCallExecutedEvent: | ||
| return HashContractCallExecutedEvent(event.(*types.ContractCallExecutedEvent)) | ||
| case *types.SignerSetTxExecutedEvent: | ||
| return HashSignerSetTxExecutedEvent(event.(*types.SignerSetTxExecutedEvent)) | ||
| case *types.ERC20DeployedEvent: | ||
| return HashERC20DeployedEvent(event.(*types.ERC20DeployedEvent)) | ||
| default: | ||
| panic(fmt.Sprintf("unknown event type: %T", e)) | ||
| } | ||
| } | ||
|
|
||
| func HashSendToCosmosEvent(event *types.SendToCosmosEvent) tmbytes.HexBytes { | ||
| rcv, _ := sdk.AccAddressFromBech32(event.CosmosReceiver) | ||
| path := bytes.Join( | ||
| [][]byte{ | ||
| sdk.Uint64ToBigEndian(event.EventNonce), | ||
| common.HexToAddress(event.TokenContract).Bytes(), | ||
| event.Amount.BigInt().Bytes(), | ||
| common.Hex2Bytes(event.EthereumSender), | ||
| rcv.Bytes(), | ||
| sdk.Uint64ToBigEndian(event.EthereumHeight), | ||
| }, | ||
| []byte{}, | ||
| ) | ||
| hash := sha256.Sum256([]byte(path)) | ||
| return hash[:] | ||
| } | ||
|
|
||
| func HashBatchExecutedEvent(event *types.BatchExecutedEvent) tmbytes.HexBytes { | ||
| path := bytes.Join( | ||
| [][]byte{ | ||
| common.HexToAddress(event.TokenContract).Bytes(), | ||
| sdk.Uint64ToBigEndian(event.EventNonce), | ||
| sdk.Uint64ToBigEndian(event.BatchNonce), | ||
| sdk.Uint64ToBigEndian(event.EthereumHeight), | ||
| }, | ||
| []byte{}, | ||
| ) | ||
| hash := sha256.Sum256([]byte(path)) | ||
| return hash[:] | ||
| } | ||
|
|
||
| func HashContractCallExecutedEvent(event *types.ContractCallExecutedEvent) tmbytes.HexBytes { | ||
| path := bytes.Join( | ||
| [][]byte{ | ||
| sdk.Uint64ToBigEndian(event.EventNonce), | ||
| event.InvalidationScope, | ||
| sdk.Uint64ToBigEndian(event.InvalidationNonce), | ||
| sdk.Uint64ToBigEndian(event.EthereumHeight), | ||
| }, | ||
| []byte{}, | ||
| ) | ||
| hash := sha256.Sum256([]byte(path)) | ||
| return hash[:] | ||
| } | ||
|
|
||
| func HashERC20DeployedEvent(event *types.ERC20DeployedEvent) tmbytes.HexBytes { | ||
| path := bytes.Join( | ||
| [][]byte{ | ||
| sdk.Uint64ToBigEndian(event.EventNonce), | ||
| []byte(event.CosmosDenom), | ||
| common.HexToAddress(event.TokenContract).Bytes(), | ||
| []byte(event.Erc20Name), | ||
| []byte(event.Erc20Symbol), | ||
| sdk.Uint64ToBigEndian(event.Erc20Decimals), | ||
| sdk.Uint64ToBigEndian(event.EthereumHeight), | ||
| }, | ||
| []byte{}, | ||
| ) | ||
| hash := sha256.Sum256([]byte(path)) | ||
| return hash[:] | ||
| } | ||
|
|
||
| func HashSignerSetTxExecutedEvent(event *types.SignerSetTxExecutedEvent) tmbytes.HexBytes { | ||
| path := bytes.Join( | ||
| [][]byte{ | ||
| sdk.Uint64ToBigEndian(event.EventNonce), | ||
| sdk.Uint64ToBigEndian(event.SignerSetTxNonce), | ||
| sdk.Uint64ToBigEndian(event.EthereumHeight), | ||
| types.EthereumSigners(event.Members).Hash(), | ||
| }, | ||
| []byte{}, | ||
| ) | ||
| hash := sha256.Sum256(([]byte(path))) | ||
| return hash[:] | ||
| } |
There was a problem hiding this comment.
Check for errors when parsing CosmosReceiver.
Here, sdk.AccAddressFromBech32(event.CosmosReceiver) silently ignores potential errors. Invalid addresses may result in unexpected zero addresses in the final hash.
func HashSendToCosmosEvent(event *types.SendToCosmosEvent) tmbytes.HexBytes {
- rcv, _ := sdk.AccAddressFromBech32(event.CosmosReceiver)
+ rcv, err := sdk.AccAddressFromBech32(event.CosmosReceiver)
+ if err != nil {
+ // Log or handle error as needed
+ rcv = nil
+ }
path := bytes.Join(
...
)
...
}Committable suggestion skipped: line range outside the PR's diff.
| func CreateUpgradeHandler( | ||
| mm *module.Manager, | ||
| configurator module.Configurator, | ||
| ) upgradetypes.UpgradeHandler { | ||
| return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) { | ||
| ctx.Logger().Info("v6 upgrade: entering handler") | ||
|
|
||
| return mm.RunMigrations(ctx, configurator, fromVM) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add version validation and specific v6 migration steps.
The upgrade handler should include:
- Version validation to ensure upgrade from v5
- Specific v6 migration steps for security fixes mentioned in the PR description
- Error handling for migration failures
Consider this implementation:
func CreateUpgradeHandler(
mm *module.Manager,
configurator module.Configurator,
) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
ctx.Logger().Info("v6 upgrade: entering handler")
+
+ // Validate upgrade from v5
+ if fromVM[gravitytypes.ModuleName] != 5 {
+ return nil, fmt.Errorf("cannot upgrade from version %d, expected version 5", fromVM[gravitytypes.ModuleName])
+ }
+
+ // Apply v6-specific migrations (security fixes, etc.)
+ if err := applyV6Migrations(ctx, mm); err != nil {
+ return nil, fmt.Errorf("failed to apply v6 migrations: %w", err)
+ }
return mm.RunMigrations(ctx, configurator, fromVM)
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
integration_tests/ethereum/contracts/MaliciousERC20.sol (1)
15-21: **Custom transfer rejects zero-value transfers **Requiring
amount > 0breaks certain default behaviors (for example, zero-value transfers often serve as a means of setting approvals in other tokens). For your malicious use case, it’s fine; just note that this deviates from standard ERC20 norms and may raise unexpected errors in some tooling.integration_tests/fee_overflow_test.go (2)
69-71: **Consider partial error logging or retry logic **If
SendEthTransactionfails, returning the error is sufficient, but consider logging or capturing relevant transaction details for better test diagnostics.
97-114: **Use ephemeral addresses for short tests **Sending large sums of minted tokens to a single validator address is acceptable, but ephemeral addresses or distinct addresses might help isolate test flows more effectively in a complex test environment.
integration_tests/ethereum/hardhat.config.ts (1)
46-50: **Store deployed contract address for reference **After logging the deployed contract address, consider storing it in a test configuration or environment variable (like
constants.ts). This helps other scripts or tests automatically reference the newly deployedMaliciousERC20contract without manually copying addresses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Makefile(1 hunks)integration_tests/ethereum/Dockerfile(1 hunks)integration_tests/ethereum/contracts/MaliciousERC20.sol(1 hunks)integration_tests/ethereum/hardhat.config.ts(1 hunks)integration_tests/fee_overflow_test.go(1 hunks)integration_tests/setup_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- integration_tests/ethereum/Dockerfile
- integration_tests/setup_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: go-build
- GitHub Check: local-tests (16.x)
🔇 Additional comments (10)
Makefile (2)
21-22: LGTM! The new e2e test target follows established patterns.The
e2e_fee_overflowtarget correctly:
- Depends on
e2e_clean_slatefor test isolation- Uses consistent naming convention with other e2e targets
- Handles failures via the existing
failtarget
22-22: Verify test coverage for fee overflow scenarios.The test specifically targets
TestUnbatchedTxsTotalFeeOverflow, which appears to be a security-related test case based on the PR summary mentioning bug bounty fixes.Let's verify the test implementation:
✅ Verification successful
Fee overflow scenarios are properly tested ✅
The test implementation in
integration_tests/fee_overflow_test.gocomprehensively verifies fee overflow protection by attempting to create transactions with fees that would cause uint256 overflow. The codebase includes multiple safeguards against fee overflow in both the batch creation logic and the orchestrator.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of fee overflow test # Look for the test implementation and any related fee calculation logic # Find the test implementation echo "Looking for fee overflow test implementation:" rg -A 10 "TestUnbatchedTxsTotalFeeOverflow" # Look for related fee calculation logic echo -e "\nLooking for fee calculation code:" rg -A 5 "fee.*overflow|overflow.*fee"Length of output: 5046
integration_tests/ethereum/contracts/MaliciousERC20.sol (2)
5-6: **Constructor lacks role-based access control checks **Currently, anyone who can create this contract can deploy it. This is presumably intentional for a "malicious" test contract; however, if you ever consider reusing this code in a non-testing capacity, ensure that role-based checks or other ACL measures are implemented to prevent unauthorized deployments.
11-13: **forceTransfer bypasses standard ERC20 checks **By design, this function deliberately allows arbitrary token transfers without the sender’s approval. Although it is malicious, it correctly illustrates a scenario for testing forced transfers. No changes needed for the intended malicious behavior.
integration_tests/fee_overflow_test.go (6)
14-33: **ABI definitions are well structured **The JSON ABI for the mint function is clear and self-contained, which simplifies reading and future updates.
35-37: **Pack function usage ensures type consistency **Using
packCallto generate ABI-encoded data helps avoid manual encoding errors and ensures correct type matching. This is good practice.
39-67: **ForceTransfer ABI extends your malicious flow **This confirms that force transfers are properly integrated into the test. No issues found here.
120-137: **Clearing Gravity contract balance is well-handled **The approach ensures multiple test iterations without leftover state. This is a robust design choice for repeated malicious exploit scenarios.
139-161: **Retriable message sending ensures reliability **Using
Eventuallyfis a solid approach to handle timing uncertainties in integration tests.
213-315: **Exploit scenario thoroughly covered **This test method systematically walks through the steps to trigger an integer overflow. Logging and step validation are clear. The scenario is well laid out for diagnosing security flaws in unbatched fee calculations.
| function mint(address recipient, uint256 amount) external { | ||
| _mint(recipient, amount); | ||
| } |
There was a problem hiding this comment.
**Mint function can be called by any external account **
The mint function grants unlimited minting power to any caller, which is highly insecure in a production setting. While this is presumably intentional for testing malicious scenarios, highlight explicitly in documentation or comments that this function should never be deployed unchanged to production.
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
MaliciousERC20for testing vulnerabilitiesImprovements
Security
Chores