fix(privacy): enforce amount conservation in privacy tx CheckTx behind ForkPrivacyAmountCheck - #1309
Open
33cn wants to merge 3 commits into
Open
fix(privacy): enforce amount conservation in privacy tx CheckTx behind ForkPrivacyAmountCheck#130933cn wants to merge 3 commits into
33cn wants to merge 3 commits into
Conversation
added 3 commits
August 31, 2026 10:55
…d ForkPrivacyAmountCheck Fix three unlimited minting vulnerabilities in the privacy dapp: 1. ActionPublic2Privacy CheckTx returned nil directly, never verifying that the declared UTXO outputs sum to payload.Amount actually withdrawn from the public balance, allowing an attacker to deposit 1 coin and mint arbitrarily large UTXOs, then cash out via privacy2public. 2. No positivity/range check on KeyOutput.Amount anywhere: a negative output offset totalOutput and defeated the only implicit conservation check (fee = totalInput - totalOutput >= 1 coin). 3. The fee/conservation branch only applied to main-chain coins, excluding token assets and parallel chains from any conservation check. After ForkPrivacyAmountCheck activates (registered at height 0, gated via cfg.IsDappFork; pre-fork behavior is unchanged for running-chain consensus compatibility): - public2privacy requires sum(output.Amount) == payload.Amount - every KeyInput/KeyOutput amount must be > 0 and <= MaxCoin*precision - privacy2privacy/privacy2public enforce sum(input) >= sum(output) + fee (privacy2public: sum(input) >= sum(output) + amount + fee) for all asset types, including token and para chains Add vuln_fix_test.go regression tests covering the three attack scenarios (all rejected post-fork), legitimate flows (unaffected), and the fork gate (pre-fork behavior preserved).
RegisterDappFork requires every registered dapp fork to have an entry in the toml [fork.sub.<exec>] section for non-local configs, otherwise Chain33Config init panics with 'exec privacy name ForkPrivacyAmountCheck not config in config file', crashing node startup on parachain CI jobs (ci_paracross, ci_parachain_rollup, ci_rgbx, ci_cross2eth).
…ervation check The fork-gated conservation check required totalInput >= totalOutput + 1 coin for all asset types, but the wallet only burns a UTXO fee for main-chain coins (isMainetCoins); for token assets and para chains utxoBurnedAmount is 0 and legitimate txs conserve exactly (totalInput == totalOutput, fee paid via tx.Fee from the public account). This broke legitimate token/para privacy2privacy transfers (ci_paracross token GD priv2priv case). Make the required UTXO-burned fee conditional, mirroring both the wallet construction and the existing coins-only fee branch: 1 coin for main-chain coins, 0 otherwise. The core invariant is unchanged: outputs never exceed inputs for any asset type. Add a legit token privacy2privacy regression case (input sum == output sum, zero utxo fee) that must pass CheckTx+Exec.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vulnerabilities
Three unlimited-minting vulnerabilities in the privacy dapp, all in
plugin/dapp/privacy/executor/privacy.goCheckTx:privacy.go:216-218):CheckTxreturnednildirectly forActionPublic2Privacy.payload.Amount(actually withdrawn from the public balance) andoutput.Keyoutput[].Amount(the UTXO denominations written to state) were never checked for conservation. An attacker could deposit 1 coin, declare UTXOs of any denomination, then cash out via privacy2public — net minting of arbitrary coins.KeyOutput.Amount > 0check on any path: a negative output offsetstotalOutput, defeating the only implicit conservation check — the privacy2privacy fee check atprivacy.go:285-295(fee = totalInput - totalOutput >= 1 coin) — allowing huge UTXOs to be minted out of thin air.privacy.go:274): the!IsPara() && (assertExec == "" || assertExec == coinExec)condition means token assets and parallel chains skip the fee/conservation check entirely, so outputs can exceed inputs arbitrarily.Root cause
CheckTxnever validated amount conservation between inputs, outputs and the declared public deposit/withdrawal amounts, and the single fee-difference check was scoped to main-chain coins only and was bypassable with negative outputs.Fix
All new checks are gated behind a new dapp fork
ForkPrivacyAmountCheck(registered inplugin/dapp/privacy/typesat height 0 viaRegisterDappFork, gated withcfg.IsDappFork, following theForkEVMFixOverflowpattern). Pre-fork behavior is fully preserved for running-chain consensus compatibility.After the fork activates:
sum(output.Keyoutput[].Amount)must equalpayload.Amount, otherwisetypes.ErrAmount.KeyInput/KeyOutputamount must be> 0and<= types.MaxCoin * coinPrecision(with total-output overflow guard), otherwisetypes.ErrAmount.sum(input) >= sum(output) + fee; for privacy2publicsum(input) >= sum(output) + amount + fee, otherwisepty.ErrPrivacyTxFeeNotEnough. The privacy2public withdrawal amount itself must also be positive and within range.Tests
New
plugin/dapp/privacy/executor/vuln_fix_test.go:TestFixVuln_Public2Privacy_AmountInflation: deposit 1 coin / declare 1001 coin outputs — now rejected withErrAmount, no forged UTXO in state.TestFixVuln_Privacy2Privacy_NegativeOutput: negative-output offset attack rejected withErrAmount; oversized output without offset rejected withErrPrivacyTxFeeNotEnough.TestFixVuln_TokenPrivacy2Privacy_NoConservation: token privacy2privacy with 1-token input / 1,000,000-token output rejected.TestFix_LegitPrivacyFlows: legitimate public2privacy / privacy2privacy / privacy2public flows still passCheckTx+Exec, exact balances verified.TestFix_ForkGate: with the fork set to a future height, the old permissive behavior is preserved.go test -ldflags=-checklinkname=0 ./plugin/dapp/privacy/...— all packages pass, including the pre-existing executor/wallet/rpc regression tests.