MF2-C02: bound ledger values to Solidity uint256/int256 ranges#764
Conversation
Reject states whose scaled ledger values fall outside the Solidity ABI ranges used for signing and onchain calls, so a caller cannot inflate offchain balances via a value that silently truncates to its low 256 bits when packed. - Add DecimalToUint256 / DecimalToInt256 wrappers around the scaling helper (now private decimalToBigInt) that enforce [0, 2^256-1] and [-2^255, 2^255-1] respectively. - Ledger.Validate now takes assetDecimals and bound-checks every scaled field. - StateAdvancerV1 looks up token decimals and passes them in so inflated state is rejected before DB write. - StatePackerV1 promotes contractLedger to package scope, runs Validate() on it before signing, and uses the bounded helpers when converting decimals. - coreLedgerToContractLedger and the remaining DepositToNode / Withdraw / Approve / native deposit Value paths swap to DecimalToUint256 so onchain submission is gated by the same check. Adds regression tests at the helper, packer, and advancer layers, including a home_deposit with amount = 2^256. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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: defaults 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 |
nksazonov
left a comment
There was a problem hiding this comment.
Good job! A few missing coverage spots in the new test suite worth addressing before landing.
Out-of-diff note — pkg/blockchain/evm/locking_client.go:229: The two call sites in this file were migrated to core.DecimalToUint256, but the original decimalToBigInt(amount decimal.Decimal, decimals int32) *big.Int was left in place with no callers. It is now dead code. Remove it to avoid accidental reuse — its different signature (int32 decimals, no error return) makes it visually distinct from the bounded helpers, but the potential for confusion remains.
Out-of-diff note — pkg/blockchain/evm/utils_test.go:139: TestCoreLedgerToContractLedger_Success covers the happy path after coreLedgerToContractLedger was updated to use DecimalToUint256/DecimalToInt256. There is no rejection test. A test asserting that a Ledger with UserBalance = 2^256 causes coreLedgerToContractLedger to return an error containing "uint256" would give the blockchain-layer conversion the same coverage as the core-layer packer and advancer tests.
Address review feedback on MF2-C02: - Add escrow-ledger overflow subtests to state_packer and state_advancer to cover the escrow branch of the uint256/int256 bound check. - Add coreLedgerToContractLedger rejection test for the blockchain-layer conversion wrapper. - Reword overflow-deposit test comment to drop the bug-number placeholder. - Remove the now-dead decimalToBigInt helper in locking_client.go; all callers were migrated to core.DecimalToUint256. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@nksazonov on the out-of-diff notes:
|
ihsraham
left a comment
There was a problem hiding this comment.
Approved for C-02. The latest head cleanly bounds ledger values before signing/storage and EVM conversion, and the added tests cover the important closure paths. Thanks for tightening this up.
- MF2-I02: fix: add SESSION_KEY_AUTH_TYPEHASH to session key authorization payload (#767) - MF2-M02: accept pre-finalize escrow version in ongoing check (#765) - MF2-L03: docs: document home chain migration as not yet active off-chain (#769) - MF2-I03: reject reused home channel ID explicitly (#766) - MF2-H03: guard challenge_rescue against post-Finalize close (#768) - MF2-C02: bound ledger values to Solidity uint256/int256 ranges (#764) - MF2-C01: preserve post-Finalize closing marker across challenge cycle (#762) - MF2-M01: fix(sdk): use keccak256(utf8) for session key application ID hashing (#761) - MF2-H01: harden receiver-state issuance and dispute resolution (#759) - MF2-L01: gate transfer_send during channel creation (#763)
Summary
Reject ledger states whose scaled values fall outside the Solidity ABI ranges used for signing and onchain calls. A caller could otherwise submit a
home_depositwhose raw amount is2^256 + X; the node would store the inflated offchain balance, but the signed EVM message and onchain calldata would only carry the low 256 bits, allowing the inflated balance to drain the node vault through a normaltransfer_send.core.DecimalToUint256/core.DecimalToInt256wrap the (now private) scaling helper and enforce[0, 2^256-1]and[-2^255, 2^255-1]respectively.Ledger.Validatenow takesassetDecimalsand bound-checks every scaled field.StateAdvancerV1looks up token decimals from the asset store and passes them in, so inflated state is rejected before the DB write.StatePackerV1promotescontractLedgerto package scope, runs aValidate()guard on it before signing, and uses the bounded helpers when converting decimals.coreLedgerToContractLedgerand the remainingDepositToNode/Withdraw/Approve/ native depositValuepaths swap toDecimalToUint256, so onchain submission is gated by the same check.Defense in depth: every layer that materialises a
*big.Intfrom a ledger decimal now refuses out-of-range values, instead of relying on a single chokepoint.Test plan
go vet ./...go test ./...pkg/core/utils_test.goforDecimalToUint256/DecimalToInt256boundaries (max, max+1, negative, scaling-induced overflow).pkg/core/state_packer_test.go::TestPackState_RejectsOutOfRangeValues— packer-level regression for both uint256 and int256 fields.pkg/core/state_advancer_test.go::TestValidateAdvancement_RejectsOverflowDeposit—home_depositwith amount2^256is rejected before DB write.pkg/core/state_advancer_test.go::TestValidateAdvancement_RejectsOverflowNetFlow— net flow2^255is rejected.app_session_v1deposit-state tests updated to stub the newGetTokenDecimalscall now made by the advancer.🤖 Generated with Claude Code