fix: distribution precompile 32-byte withdraw address inflates native…#16
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
11ac5c6 to
a7e1839
Compare
e52c33e to
cc5caea
Compare
cc5caea to
c8f7153
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a native-supply inflation bug in the distribution precompile’s balance mirroring by ensuring only SDK accounts with a bijective 20-byte EVM mapping are mirrored into the EVM StateDB.
Changes:
- Add an
isMirrorableEVMAddressguard to skip mirroring balance events for non-20-byte SDK addresses (preventscommon.BytesToAddresstruncation collisions). - Add a regression test covering the 32-byte address mirroring case.
- Update system/solidity test infrastructure (systemtest helper usage, consensus-timeout defaults, truffle/solc configuration, and a targeted test skip).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/systemtests/upgrade_test.go | Updates systemtest helper invocation for awaiting upgrade height. |
| tests/systemtests/suite/test_suite.go | Ensures ModifyConsensusTimeout uses default node args when none are provided. |
| tests/systemtests/mempool/test_replacement.go | Skips a known-flaky DynamicFeeTx replacement scenario across nodes. |
| tests/systemtests/mempool/test_broadcast.go | Changes per-case setup in tx broadcasting tests (removes prior “no blocks produced” assertion). |
| tests/solidity/suites/opcode/truffle-config.js | Pins solc to a local soljson.js path for deterministic compilation. |
| tests/solidity/suites/opcode/package.json | Adds solc@0.5.17 for the opcode suite. |
| tests/solidity/suites/exception/truffle-config.js | Pins solc to a local soljson.js path. |
| tests/solidity/suites/eip1559/truffle-config.js | Pins solc to a local soljson.js path. |
| tests/solidity/suites/basic/truffle-config.js | Pins solc to a local soljson.js path. |
| tests/solidity/package.json | Adds solc@0.8.18 to the solidity test harness dependencies. |
| precompiles/common/balance_handler.go | Skips mirroring for non-20-byte SDK addresses to prevent truncated-address duplication in EVM state. |
| precompiles/common/balance_handler_test.go | Adds a regression test for skipping 32-byte address mirroring. |
| Makefile | Updates the legacy binary build checkout ref used by system tests. |
| CHANGELOG.md | Documents the bug fix in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
203ea0c to
cc72173
Compare
cc72173 to
ceddeaf
Compare
… supply (#340) # Description Fixes native token supply inflation triggered through the stateful EVM precompiles (distribution/staking/gov) when an account address is not exactly 20 bytes. The precompiles accept an account address (e.g. the distribution withdraw address) and mirror the resulting bank transfer into the EVM StateDB via `common.BytesToAddress`, which is keyed by a 20-byte address. A longer account (e.g. a 32-byte bech32 account, module account, or CosmWasm contract) is silently truncated to its trailing 20 bytes during mirroring, causing the StateDB commit to mint a duplicate balance to that trailing-20-byte account and inflate native supply. This PR introduces an `evmAddressCodec` wrapper (`app/keepers/precompiles.go`) around the account `AddressCodec` used by the gov/staking precompiles. Its `StringToBytes` rejects any decoded address that is not exactly 20 bytes, so non-EVM accounts can never enter a balance-mirroring precompile flow. NOTE: This is an in-kiichain fix and does NOT bump the `cosmos/evm` fork; the `go.mod` replace directive remains at `v0.6.0-fork.1`. The related fork-side fix (KiiChain/evm#16) is an alternative/defense-in-depth approach and is not required by this PR. ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) # How Has This Been Tested? - [x] Test A — Unit test for `evmAddressCodec.StringToBytes`: a 20-byte account decodes successfully; a 32-byte account is rejected with a clear error. - [x] Test B — `make test-unit` and `make build` pass. # PR Checklist: - [x] Updated changelog with PR's intent - [x] Lint with `make lint-fix`
Description
Fixes the distribution-precompile 32-byte withdraw address supply-inflation bug. When a balance event's account is not exactly 20 bytes (e.g. a 32-byte bech32 withdraw, module, or CosmWasm contract account),
common.BytesToAddresstruncates it to the trailing 20 bytes, causing the StateDB commit to mint a duplicate balance and inflate native supply.AfterBalanceChangenow skips mirroring any non-20-byte account (isMirrorableEVMAddressguard) in theCoinSpent,CoinReceived, andFractionalBalanceChangecases, so only bijective SDK↔EVM accounts are mirrored.Most critical to review:
precompiles/common/balance_handler.goAuthor 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...
mainbranch — N/A: targetsfeat/v0.6.0(thev0.6.0-fork.1release lineage kiichain pins); av0.6.0-fork.2tag will follow