feat: update token handling to improve security #914
Conversation
WalkthroughThis pull request introduces enhancements to the upgrade process and testing framework across multiple files. The changes focus on improving token migration, metadata validation, and upgrade testing capabilities. Key modifications include adding new validation functions in the test suite, introducing migration functions for ERC20 tokens and metadata, and standardizing denomination handling. The updates aim to strengthen the robustness of the upgrade process for both testnet and mainnet environments. Changes
Sequence DiagramsequenceDiagram
participant Upgrade as Upgrade Process
participant Metadata as Metadata Migration
participant ERC20 as ERC20 Token Migration
participant Validator as Test Validator
Upgrade->>Metadata: Migrate Metadata Display
Upgrade->>ERC20: Migrate FX to Pundi AI Tokens
Upgrade->>Validator: Perform Upgrade Checks
Validator->>Metadata: Validate Metadata
Validator->>ERC20: Verify Token Existence
Possibly related PRs
Poem
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 (5)
x/erc20/migrations/v8/keys.go (1)
67-67: LGTM! Consider extracting the token pair migration logic.The introduction of
fxDenomimproves code maintainability. However, the token pair migration logic could be extracted into a separate helper function to improve readability and testability.Consider extracting the token pair migration logic into a separate function:
func (m Migrator) migrateTokenPair(ctx sdk.Context, store storetypes.KVStore) error { fxDenom := strings.ToUpper(fxtypes.FXDenom) iterator := storetypes.KVStorePrefixIterator(store, KeyPrefixTokenPair) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { - var tokenPair types.ERC20Token - m.cdc.MustUnmarshal(iterator.Value(), &tokenPair) - md, found := m.bankKeeper.GetDenomMetaData(ctx, tokenPair.GetDenom()) - if !found { - return sdkerrors.ErrKeyNotFound.Wrapf("metadata not found: %s", tokenPair.GetDenom()) - } - if md.Base == fxDenom || md.Base == strings.ToLower(md.Symbol) { - if err := m.keeper.ERC20Token.Set(ctx, md.Base, tokenPair); err != nil { - return err - } - if err := m.keeper.DenomIndex.Set(ctx, tokenPair.Erc20Address, md.Base); err != nil { - return err - } - continue - } - // ... rest of the migration logic + if err := m.migrateTokenPairEntry(ctx, iterator.Value(), fxDenom); err != nil { + return err + } } return nil } +func (m Migrator) migrateTokenPairEntry(ctx sdk.Context, value []byte, fxDenom string) error { + var tokenPair types.ERC20Token + m.cdc.MustUnmarshal(value, &tokenPair) + md, found := m.bankKeeper.GetDenomMetaData(ctx, tokenPair.GetDenom()) + if !found { + return sdkerrors.ErrKeyNotFound.Wrapf("metadata not found: %s", tokenPair.GetDenom()) + } + if md.Base == fxDenom || md.Base == strings.ToLower(md.Symbol) { + if err := m.keeper.ERC20Token.Set(ctx, md.Base, tokenPair); err != nil { + return err + } + if err := m.keeper.DenomIndex.Set(ctx, tokenPair.Erc20Address, md.Base); err != nil { + return err + } + return nil + } + // ... rest of the migration logic + return nil +}Also applies to: 77-77
x/erc20/migrations/v8/migrate.go (1)
77-82: Improve error handling and readability in bridge token logic.While the introduction of
fxDenomimproves maintainability, the bridge token logic could benefit from better error handling and readability improvements.Consider these improvements:
- Extract the bridge token validation logic into a separate function.
- Add detailed error messages for each failure case.
- Use early returns to reduce nesting.
func (m Migrator) addBridgeToken(ctx sdk.Context, base, alias string) error { fxDenom := strings.ToUpper(fxtypes.FXDenom) if getExcludeBridgeToken(ctx, alias) { return nil } + // Extract validation into a separate function + if err := m.validateBridgeToken(ctx, base, alias); err != nil { + return fmt.Errorf("bridge token validation failed: %w", err) + } for _, ck := range m.crosschainKeepers { - canAddFxBridgeToken := base == fxDenom && ck.ModuleName() == ethtypes.ModuleName - canAddBridgeToken := strings.HasPrefix(alias, ck.ModuleName()) + if !m.canAddBridgeToken(ctx, ck, base, alias, fxDenom) { + continue + } // ... rest of the function } return nil }app/upgrade_test.go (2)
576-587: Enhance metadata validation test coverage.While the test validates basic metadata properties, it could benefit from additional test cases.
Consider adding these test cases:
func checkMetadataValidate(t *testing.T, ctx sdk.Context, myApp *app.App) { t.Helper() myApp.BankKeeper.IterateAllDenomMetaData(ctx, func(metadata banktypes.Metadata) bool { if len(metadata.DenomUnits) <= 1 { return false } require.NoError(t, metadata.Validate()) require.NotEqual(t, metadata.Display, metadata.Base) + // Add more assertions + require.NotEmpty(t, metadata.Description, "metadata description should not be empty") + require.NotEmpty(t, metadata.Symbol, "metadata symbol should not be empty") + require.True(t, len(metadata.DenomUnits) >= 2, "metadata should have at least 2 denom units") return false }) }
589-598: Enhance ERC20 token validation test coverage.The test could be more comprehensive in validating token properties.
Consider expanding the test:
func checkPundiAIFXERC20Token(t *testing.T, ctx sdk.Context, myApp *app.App) { t.Helper() has, err := myApp.Erc20Keeper.ERC20Token.Has(ctx, fxtypes.DefaultDenom) require.NoError(t, err) require.True(t, has) + // Add token property validations + token, err := myApp.Erc20Keeper.ERC20Token.Get(ctx, fxtypes.DefaultDenom) + require.NoError(t, err) + require.NotEmpty(t, token.Erc20Address, "ERC20 address should not be empty") + require.Equal(t, fxtypes.DefaultDenom, token.Denom, "token denom should match") has, err = myApp.Erc20Keeper.ERC20Token.Has(ctx, strings.ToUpper(fxtypes.FXDenom)) require.NoError(t, err) require.False(t, has) }scripts/linter.sh (1)
Line range hint
4-11: Consider enhancing pattern tracking for security context.The pattern limits array could be improved to better track and manage security-critical suppressions:
Consider these improvements:
patternLimits=( - "nolint:23" + # Format: "pattern:limit:category:description" + "nolint:23:general:Generic code suppressions" + "nolint-security:0:security:Security-related suppressions" "#nosec:5" "CrossChain:4" "cross chain:0" "GetERC1967Proxy:4" )This would:
- Separate security-critical suppressions from general ones
- Document the rationale for each limit
- Enable stricter enforcement of security-related suppressions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/upgrade_test.go(5 hunks)app/upgrades/v8/upgrade.go(3 hunks)scripts/linter.sh(1 hunks)x/erc20/migrations/v8/keys.go(2 hunks)x/erc20/migrations/v8/migrate.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
scripts/linter.sh (1)
6-6: Verify the necessity of increasing nolint suppressions.Increasing the allowed count of linter suppressions in a security-focused PR requires careful consideration. Each "nolint" directive potentially bypasses important checks that could catch security issues.
Let's identify the new suppression and verify its necessity:
Could you please:
- Justify why this additional suppression is necessary
- Confirm that the suppressed check doesn't impact security
- Consider if there's an alternative approach that doesn't require suppressing the linter
✅ Verification successful
Increasing nolint limit is reasonable and doesn't impact security.
The additional suppression allowance is justified as:
- Most existing suppressions are for cyclomatic complexity in core Cosmos SDK code
- No security-critical linter rules are being suppressed
- Complex functions are either well-tested SDK code or marked for future refactoring
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the new nolint suppression and its context # Get the diff that introduced the new nolint git log -p --full-diff -S "nolint" . # List all current nolint suppressions for review rg "nolint" --type go --glob '!*.pb.go' --glob '!*.pulsar.go' --glob '!*.sol.go' --glob '!legacy.go' -C 2Length of output: 7335
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/upgrades/v8/upgrade.go (1)
590-608: Add validation for metadata updates.The function correctly validates metadata after modification but should also:
- Validate metadata before modification
- Log the changes being made
Apply this diff to improve the implementation:
func migrateMetadataDisplay(ctx sdk.Context, bankKeeper bankkeeper.Keeper) error { mds := bankKeeper.GetAllDenomMetaData(ctx) for _, md := range mds { + if err := md.Validate(); err != nil { + return fmt.Errorf("invalid metadata before migration for denom %s: %w", md.Base, err) + } if md.Display != md.Base || len(md.DenomUnits) <= 1 { continue } + oldDisplay := md.Display for _, dus := range md.DenomUnits { if dus.Denom != md.Base { md.Display = dus.Denom break } } if err := md.Validate(); err != nil { return err } + ctx.Logger().Info("migrated metadata display", + "denom", md.Base, + "old_display", oldDisplay, + "new_display", md.Display) bankKeeper.SetDenomMetaData(ctx, md) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v8/upgrade.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (4)
app/upgrades/v8/upgrade.go (4)
88-90: LGTM! Error handling improvement.Good addition of explicit error handling for the redeployTestnetContract call.
610-621: Add validation for token migration.The function should validate the token state before and after migration.
623-634: Add validation for metadata migration.The function should validate the metadata state before and after migration.
94-100: Verify the migration order.The order of migrations is important. Please verify that:
- migrateMetadataDisplay must run before token migrations
- migrateErc20FXToPundiAI must run before migrateMetadataFXToPundiAI
✅ Verification successful
Migration order is correctly implemented
The current implementation follows the required order:
migrateMetadataDisplayruns first (independent display field updates)migrateErc20FXToPundiAIruns before metadata migration (token mapping)migrateMetadataFXToPundiAIruns last (depends on new token mapping)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any dependencies between these functions in the codebase # Look for any shared state or resources between these functions # Check if any function modifies metadata that another function reads rg -A 5 "SetDenomMetaData|GetAllDenomMetaData" # Check if any function modifies ERC20 tokens that another function reads rg -A 5 "GetERC20Token|ERC20Token.Set|ERC20Token.Remove"Length of output: 13242
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/upgrades/v8/upgrade.go (1)
590-608: Add context to validation errors.While the function includes validation, error context would help debugging issues.
if err := md.Validate(); err != nil { - return err + return fmt.Errorf("invalid metadata for denom %s: %w", md.Base, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v8/upgrade.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (5)
app/upgrades/v8/upgrade.go (5)
88-100: LGTM! Error handling and migration sequence look good.The changes properly handle errors from contract redeployment and add token migration steps in a logical order.
183-191: LGTM! Consistent error handling and migration sequence.The changes maintain consistent error handling by returning toVM and add token migration steps in the same order as testnet.
610-621: Add validation for ERC20 token migration.
623-634: Add validation for metadata migration.
Line range hint
88-191: Verify upgrade safety against potential exploits.The upgrade process involves sensitive token migration operations. Please ensure:
- The upgrade can't be front-run or exploited
- Token balances are preserved during migration
- No double-spending possibilities exist
✅ Verification successful
Upgrade implementation follows security best practices
The verification shows the upgrade process is properly secured:
- All token operations use proper keeper methods with balance checks
- Operations are atomic with proper error handling
- Module-scoped operations prevent unauthorized access
- Proper validation checks are in place for metadata updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential security issues in the upgrade process # Look for balance checks before migrations rg -A 2 "GetBalance.*ModuleName" # Check for proper event emission during migrations rg "EmitEvent.*migration" # Verify no direct balance modifications without proper checks ast-grep --pattern 'MintCoins($_, $_, $_)'Length of output: 791
Script:
#!/bin/bash # Check for token burning and minting operations rg -A 3 "BurnCoins|SendCoins|TransferFrom" app/upgrades/v8/ # Check for access controls and authorization ast-grep --pattern 'func $_(ctx sdk.Context, $$$) { $$$ RequireAuth($$$) $$$ }' # Look for broader event patterns rg -A 2 "ctx\.EventManager\(\)\.EmitEvent" app/upgrades/v8/Length of output: 1517
Script:
#!/bin/bash # Check upgrade handler access controls ast-grep --pattern 'func upgrade$_(ctx sdk.Context, $$$) { $$$ }' # Look for validation checks rg -A 3 "ValidateBasic|Validate|require\." app/upgrades/v8/ # Check for transaction boundaries rg -A 3 "WithTransient|WithCache" app/upgrades/v8/Length of output: 389
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores