fix(cronos): add contract authorization check to SendCroToIbcHandler#2099
Conversation
Any EVM contract emitting __CronosSendCroToIbc could drain CRO via IBC without being registered or authorized. Sister handlers (SendToIbc, SendToAccount) already gate on contract registration; this handler had no equivalent guard. Fix: - Add CroBridgeContractAddress string param to cronos module params - SendCroToIbcHandler rejects callers that don't match the authorized address; empty param (safe default) disables the hook entirely - validateIsEvmAddress rejects non-hex strings and the zero address - v1.8 upgrade handler migrates the param (mainnetCroBridgeContractAddress constant must be filled in before release) Tests: - Unit: two new rejection cases + existing success cases updated to set the authorized address before calling the handler - Go upgrade test: validates constant format + SetParams round-trip - Integration upgrade test: asserts param is "" after v1.8 handler runs - Integration IBC test: authorizes deployed CroBridge via gov proposal before bridge call (fixes test breakage from the new guard)
This comment has been minimized.
This comment has been minimized.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a repeated ChangesCroBridge Contract Authorization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @JayT106's task in 2m 24s —— View job PR Review: fix(cronos): add contract authorization check to SendCroToIbcHandler
SummaryThis PR closes a real authorization gap: any EVM contract emitting State / Consensus ImpactThis is a state-breaking change requiring a coordinated upgrade. The Issues FoundCritical:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
app/upgrades.go (1)
22-25: Add a release-time guard for the mainnet bridge address.
mainnetCroBridgeContractAddressstill defaults to"", and the current upgrade tests explicitly accept that value. If this is missed during a mainnet cut, v1.8 will silently leaveSendCroToIbcdisabled chain-wide after upgrade. Consider making CI or release packaging fail when this constant is empty for the mainnet build.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/upgrades.go` around lines 22 - 25, The constant mainnetCroBridgeContractAddress is left as an empty string which silently disables SendCroToIbc; add a release-time guard that fails fast when building or running a mainnet release: implement a validation check (e.g. in an init() or an upgrade validation function) that asserts mainnetCroBridgeContractAddress is non-empty for mainnet builds and triggers a clear panic/exit or returns an error during startup/packaging, or add a CI/release packaging step to reject builds where mainnetCroBridgeContractAddress == ""; reference the symbol mainnetCroBridgeContractAddress and the SendCroToIbc hook to locate where to enforce the guard.x/cronos/keeper/evmhandlers_test.go (1)
368-373: ⚡ Quick winUse
SendCroToIbcEventABI inTestSendCroToIbcHandlerpayload setup.These cases currently pack data with
SendToIbcEvent. It works only while both event input layouts stay identical, which weakens this test’s contract-specific coverage.Proposed patch
- input, _ := evmhandlers.SendToIbcEvent.Inputs.NonIndexed().Pack( + input, _ := evmhandlers.SendCroToIbcEvent.Inputs.NonIndexed().Pack( sender, "recipient", coin.Amount.BigInt(), )- input, _ := evmhandlers.SendToIbcEvent.Inputs.NonIndexed().Pack( + input, _ := evmhandlers.SendCroToIbcEvent.Inputs.NonIndexed().Pack( sender, "recipient", coin.Amount.BigInt(), )- input, _ := evmhandlers.SendToIbcEvent.Inputs.NonIndexed().Pack( + input, _ := evmhandlers.SendCroToIbcEvent.Inputs.NonIndexed().Pack( sender, "recipient", coin.Amount.BigInt(), )Also applies to: 392-397, 446-450
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cronos/keeper/evmhandlers_test.go` around lines 368 - 373, The test is packing event payloads with evmhandlers.SendToIbcEvent but the test is specifically for SendCroToIbcHandler; change the payload packing to use the SendCroToIbcEvent ABI (e.g. evmhandlers.SendCroToIbcEvent.Inputs.NonIndexed().Pack(...)) wherever SendToIbcEvent is used in TestSendCroToIbcHandler (and the other occurrences noted), so the packed input uses the contract-specific SendCroToIbcEvent layout instead of SendToIbcEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/cronos/simulation/genesis.go`:
- Line 84: The GetOrGenerate block that populates maxCallbackGas is mistakenly
writing into ibcTimeout instead of the maxCallbackGas variable, so when
AppParams supplies max_callback_gas it gets ignored and 0 is persisted; update
the GetOrGenerate usage to read/write the correct key into maxCallbackGas (not
ibcTimeout) so that the later call to types.NewParams(ibcCroDenom, ibcTimeout,
cronosAdmin, enableAutoDeployment, maxCallbackGas, "") receives the intended
value. Locate the GetOrGenerate invocation and replace the ibcTimeout target
with maxCallbackGas and ensure the AppParams key name matches
"max_callback_gas".
---
Nitpick comments:
In `@app/upgrades.go`:
- Around line 22-25: The constant mainnetCroBridgeContractAddress is left as an
empty string which silently disables SendCroToIbc; add a release-time guard that
fails fast when building or running a mainnet release: implement a validation
check (e.g. in an init() or an upgrade validation function) that asserts
mainnetCroBridgeContractAddress is non-empty for mainnet builds and triggers a
clear panic/exit or returns an error during startup/packaging, or add a
CI/release packaging step to reject builds where mainnetCroBridgeContractAddress
== ""; reference the symbol mainnetCroBridgeContractAddress and the SendCroToIbc
hook to locate where to enforce the guard.
In `@x/cronos/keeper/evmhandlers_test.go`:
- Around line 368-373: The test is packing event payloads with
evmhandlers.SendToIbcEvent but the test is specifically for SendCroToIbcHandler;
change the payload packing to use the SendCroToIbcEvent ABI (e.g.
evmhandlers.SendCroToIbcEvent.Inputs.NonIndexed().Pack(...)) wherever
SendToIbcEvent is used in TestSendCroToIbcHandler (and the other occurrences
noted), so the packed input uses the contract-specific SendCroToIbcEvent layout
instead of SendToIbcEvent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1555cfe1-e7fd-4784-9b88-2a9f6611cba7
⛔ Files ignored due to path filters (1)
x/cronos/types/cronos.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
app/upgrades.goapp/upgrades_test.gointegration_tests/test_ibc.pyintegration_tests/test_upgrade.pyproto/cronos/cronos.protox/cronos/keeper/evmhandlers/send_cro_to_ibc.gox/cronos/keeper/evmhandlers_test.gox/cronos/simulation/genesis.gox/cronos/types/params.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2099 +/- ##
==========================================
- Coverage 16.87% 14.01% -2.87%
==========================================
Files 72 75 +3
Lines 6163 5146 -1017
==========================================
- Hits 1040 721 -319
+ Misses 5000 4343 -657
+ Partials 123 82 -41
🚀 New features to boost your workflow:
|
- Make validateIsEvmAddress error message generic (not param-name-specific) - Explicitly reset CroBridgeContractAddress in test malleate for clarity - Add round-trip assertion in integration test for cro_bridge_contract_address
…able ibcTimeout was being used as the target pointer instead of maxCallbackGas, causing max_callback_gas from AppParams to be ignored and 0 persisted.
…est env Port nix/testenv.nix fixes from crypto-org-chain#2091 to unblock integration_tests CI: - Add preferWheels = true to mkPoetryEnv - Clear postPatch for ckzg wheel builds (no src/Makefile in wheel) - Clear preConfigure for eth-hash/rlp/eth-keyfile/eth-keys/web3 wheel builds - Guard typing-extensions/types-requests postPatch to source-only builds
Lines 211-213 exceeded 88-char limit and failed Black formatting check. Extract assertion variables to shorten lines.
- Fix import ordering in x/cronos/types/params.go (golangci-lint --fix) - Add missing blank line in app/upgrades_test.go (golangci-lint --fix) - Add cro_bridge_contract_address field to test_gov_update_params expected params
Supports multiple bridge implementations and testnets by changing CroBridgeContractAddress (string) to CroBridgeContractAddresses ([]string) in params, proto, and the SendCroToIbc authorization check.
Rename cro_bridge_contract_address -> cro_bridge_contract_addresses and update all integration test references to use []string instead of string.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
x/cronos/types/params.go (2)
143-154: ⚡ Quick winEmpty strings in the address slice create validation inconsistency.
validateIsEvmAddressesallows empty strings within the slice because it delegates tovalidateIsEvmAddress, which returnsnilfor empty strings (line 161-163). However, the zero-address check (lines 167-169) rejects the explicit zero address hex string. This creates an inconsistency:""is allowed but converts to the zero address viacommon.HexToAddress(""), while"0x0000000000000000000000000000000000000000"is explicitly rejected.For a slice parameter, the "not set" state should be
[]string{}(empty slice), not a slice containing empty strings like[]string{"", ""}. Allowing empty strings within the slice serves no purpose and may confuse operators who might mistakenly believe adding""to the list has some effect.🛡️ Proposed fix to reject empty strings in the slice
Reject empty strings in
validateIsEvmAddressesbefore delegating to the single-address validator:func validateIsEvmAddresses(i interface{}) error { addrs, ok := i.([]string) if !ok { return fmt.Errorf("invalid parameter type: %T", i) } for _, addr := range addrs { + if len(addr) == 0 { + return fmt.Errorf("address list must not contain empty strings") + } if err := validateIsEvmAddress(addr); err != nil { return err } } return nil }Alternatively, remove the empty-string early return from
validateIsEvmAddressif it's only used for slice elements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cronos/types/params.go` around lines 143 - 154, validateIsEvmAddresses currently lets empty strings pass through because validateIsEvmAddress treats "" as valid; update validateIsEvmAddresses to explicitly reject empty elements before calling validateIsEvmAddress by checking each addr == "" and returning a descriptive error (e.g., "empty address in slice"), so slices must use []string{} for "not set"; alternatively, remove the empty-string early-return in validateIsEvmAddress but prefer the slice-level check in validateIsEvmAddresses to catch this case.
161-163: 💤 Low valueDocument the rationale for allowing empty strings in single-address validation.
The early return for empty strings in
validateIsEvmAddressis appropriate for optional single-address parameters likeCronosAdmin(where""means "not set"), but when reused for slice validation invalidateIsEvmAddresses, it produces the confusing behavior flagged above.Consider adding a comment here explaining that empty strings are permitted to support optional single-address fields, or split the validators into separate single-address and slice-element variants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/cronos/types/params.go` around lines 161 - 163, The empty-string early return in validateIsEvmAddress currently permits "" for optional single-address params (e.g., CronosAdmin) but causes confusing behavior when reused by validateIsEvmAddresses; either add a clear comment above validateIsEvmAddress explaining that the empty string is intentionally allowed to represent "not set" for single-value params, and note that callers validating slices should handle empties separately, or refactor by creating two validators: validateIsEvmAddressAllowEmpty (for single optional fields) and validateIsEvmAddressNonEmpty (used by validateIsEvmAddresses for slice elements), then update validateIsEvmAddresses to call the non-empty variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/upgrades_test.go`:
- Around line 69-73: The test TestUpgradeV18CroBridgeContractAddresses currently
only uses common.IsHexAddress to validate mainnetCroBridgeContractAddresses, but
production validation uses validateIsEvmAddresses → validateIsEvmAddress which
also rejects the zero address; update the test to mirror production validation
by either invoking validateIsEvmAddresses (or validateIsEvmAddress for each
entry) on each addr or add an explicit non-zero-address assertion (addr !=
common.Address{} / not "0x000...") so the test will fail with the same rule as
SetParams; reference the test function TestUpgradeV18CroBridgeContractAddresses
and the validators validateIsEvmAddresses / validateIsEvmAddress when making the
change.
In `@integration_tests/test_gov_update_params.py`:
- Line 55: The test is using the wrong parameter name and type: replace the
singular string field "cro_bridge_contract_address" with the plural slice field
matching the proto/params definition (CroBridgeContractAddresses) and provide a
list (e.g., [] or ["..."]) instead of an empty string; update the test data in
integration_tests/test_gov_update_params.py to set CroBridgeContractAddresses
(matching the x/cronos/types/params.go CroBridgeContractAddresses []string) so
the governance proposal uses the correct field and type.
---
Nitpick comments:
In `@x/cronos/types/params.go`:
- Around line 143-154: validateIsEvmAddresses currently lets empty strings pass
through because validateIsEvmAddress treats "" as valid; update
validateIsEvmAddresses to explicitly reject empty elements before calling
validateIsEvmAddress by checking each addr == "" and returning a descriptive
error (e.g., "empty address in slice"), so slices must use []string{} for "not
set"; alternatively, remove the empty-string early-return in
validateIsEvmAddress but prefer the slice-level check in validateIsEvmAddresses
to catch this case.
- Around line 161-163: The empty-string early return in validateIsEvmAddress
currently permits "" for optional single-address params (e.g., CronosAdmin) but
causes confusing behavior when reused by validateIsEvmAddresses; either add a
clear comment above validateIsEvmAddress explaining that the empty string is
intentionally allowed to represent "not set" for single-value params, and note
that callers validating slices should handle empties separately, or refactor by
creating two validators: validateIsEvmAddressAllowEmpty (for single optional
fields) and validateIsEvmAddressNonEmpty (used by validateIsEvmAddresses for
slice elements), then update validateIsEvmAddresses to call the non-empty
variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c41fdbb9-73b6-43c7-8a88-fc9721aced87
⛔ Files ignored due to path filters (1)
x/cronos/types/cronos.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
app/upgrades.goapp/upgrades_test.gointegration_tests/test_gov_update_params.pyproto/cronos/cronos.protox/cronos/keeper/evmhandlers/send_cro_to_ibc.gox/cronos/keeper/evmhandlers_test.gox/cronos/simulation/genesis.gox/cronos/types/params.go
🚧 Files skipped from review as they are similar to previous changes (4)
- proto/cronos/cronos.proto
- x/cronos/simulation/genesis.go
- x/cronos/keeper/evmhandlers_test.go
- app/upgrades.go
Panic early in RegisterUpgradeHandlers if any address is not a valid non-zero EVM hex address, catching misconfiguration before upgrade height.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration_tests/test_ibc.py`:
- Around line 211-214: The comprehension and long assertion message in the test
are too long for lint; reformat the list comprehension and the assert into
multiple lines so Black/flake8 pass. For example, split the stored_addresses
assignment that uses stored_params.get("cro_bridge_contract_addresses", [])
across lines and break the assert into a short condition line and a separate
indented tuple/message expression referencing contract.address and stored_params
so the lines are under the max length; make changes around stored_addresses,
stored_params and contract.address.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba252717-38cf-4ca5-a665-8740c7a273d7
📒 Files selected for processing (3)
integration_tests/test_gov_update_params.pyintegration_tests/test_ibc.pyintegration_tests/test_upgrade.py
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/test_upgrade.py
Proto marshal/unmarshal round-trips []string{} as nil; TestMigrate
compares DefaultParams() against the deserialized value and failed.
Remove the explicit empty slice so both sides are nil after round-trip.
Also reverts gofumpt struct-field alignment to match the longest field
(EnableAutoDeployment) now that CroBridgeContractAddresses is omitted
from the literal.
Add the two deployed CroBridge contract addresses and rename variable from mainnetCroBridgeContractAddresses to croBridgeContractAddresses.
…ress Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2669a65
…rypto-org-chain#2099) * fix(cronos): add contract authorization check to SendCroToIbcHandler Any EVM contract emitting __CronosSendCroToIbc could drain CRO via IBC without being registered or authorized. Sister handlers (SendToIbc, SendToAccount) already gate on contract registration; this handler had no equivalent guard. Fix: - Add CroBridgeContractAddress string param to cronos module params - SendCroToIbcHandler rejects callers that don't match the authorized address; empty param (safe default) disables the hook entirely - validateIsEvmAddress rejects non-hex strings and the zero address - v1.8 upgrade handler migrates the param (mainnetCroBridgeContractAddress constant must be filled in before release) Tests: - Unit: two new rejection cases + existing success cases updated to set the authorized address before calling the handler - Go upgrade test: validates constant format + SetParams round-trip - Integration upgrade test: asserts param is "" after v1.8 handler runs - Integration IBC test: authorizes deployed CroBridge via gov proposal before bridge call (fixes test breakage from the new guard) * update changelog * fix(cronos): address review findings from PR crypto-org-chain#2099 - Make validateIsEvmAddress error message generic (not param-name-specific) - Explicitly reset CroBridgeContractAddress in test malleate for clarity - Add round-trip assertion in integration test for cro_bridge_contract_address * fix(cronos): fix maxCallbackGas GetOrGenerate writing into wrong variable ibcTimeout was being used as the target pointer instead of maxCallbackGas, causing max_callback_gas from AppParams to be ignored and 0 persisted. * fix(nix): add preferWheels and wheel-safe overrides for integration test env Port nix/testenv.nix fixes from crypto-org-chain#2091 to unblock integration_tests CI: - Add preferWheels = true to mkPoetryEnv - Clear postPatch for ckzg wheel builds (no src/Makefile in wheel) - Clear preConfigure for eth-hash/rlp/eth-keyfile/eth-keys/web3 wheel builds - Guard typing-extensions/types-requests postPatch to source-only builds * fix(test): fix python lint errors in test_ibc.py Lines 211-213 exceeded 88-char limit and failed Black formatting check. Extract assertion variables to shorten lines. * fix(ci): fix golangci-lint and gov param test failures - Fix import ordering in x/cronos/types/params.go (golangci-lint --fix) - Add missing blank line in app/upgrades_test.go (golangci-lint --fix) - Add cro_bridge_contract_address field to test_gov_update_params expected params * fix(cronos): expand CroBridgeContractAddress to a list Supports multiple bridge implementations and testnets by changing CroBridgeContractAddress (string) to CroBridgeContractAddresses ([]string) in params, proto, and the SendCroToIbc authorization check. * fix(integration-tests): update CroBridge param to address list Rename cro_bridge_contract_address -> cro_bridge_contract_addresses and update all integration test references to use []string instead of string. * fix(cronos): validate mainnetCroBridgeContractAddresses at startup Panic early in RegisterUpgradeHandlers if any address is not a valid non-zero EVM hex address, catching misconfiguration before upgrade height. * fix(ci): fix gci import order in app/upgrades.go * fix(test): fix python lint line-length in test_ibc.py * fix(cronos): use nil default for CroBridgeContractAddresses Proto marshal/unmarshal round-trips []string{} as nil; TestMigrate compares DefaultParams() against the deserialized value and failed. Remove the explicit empty slice so both sides are nil after round-trip. Also reverts gofumpt struct-field alignment to match the longest field (EnableAutoDeployment) now that CroBridgeContractAddresses is omitted from the literal. * feat(upgrade): set mainnet CroBridge contract addresses for v1.8 Add the two deployed CroBridge contract addresses and rename variable from mainnetCroBridgeContractAddresses to croBridgeContractAddresses. * fix(upgrade): update test to use renamed croBridgeContractAddresses * fix(test): update upgrade test to expect populated croBridge addresses * fix: inline evm address validation and remove unused validateIsEvmAddress Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Thomas Nguy <thomas.nguy@crypto.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Any EVM contract emitting
__CronosSendCroToIbccould drain CRO via IBC without being registered or authorized. Sister handlers (SendToIbc,SendToAccount) already gate on contract registration; this handler had no equivalent guard.Changes
CroBridgeContractAddresses([]string) to support multiple bridge implementations and testnetsSendCroToIbcHandlerrejects callers not in the authorized list; empty list (safe default) disables the hook entirelyRegisterUpgradeHandlerspanics at startup if any entry inmainnetCroBridgeContractAddressesis invalid — catches misconfiguration before upgrade heightmainnetCroBridgeContractAddressesmust be filled in before release)