Harden signed-message domains and clarify pay-clearance semantics#3
Conversation
…ine rename - PayResolver: require pay.payResolver == address(this) so a pay can only be resolved by the resolver its body designates - Document plain-ERC20-only support in README, contracts.md, and openChannel NatSpec (fee-on-transfer / rebasing tokens are out of scope) - Rename SimplexPaymentChannel.last_pay_resolve_deadline → pay_clear_deadline for clearer semantics (= max(pay.resolveDeadline) + clearMargin); rename PayRegistry.getPayAmounts parameter to _maxResolveDeadline so the registry describes its own domain; add multi-segment confirmSettle regression tests
There was a problem hiding this comment.
Pull request overview
This PR hardens replay protection for signed, standalone messages by binding them to their intended execution domain (chain + contract address where applicable), and clarifies settlement/pay-clearance semantics by renaming and tightening the “last pay resolve deadline” concept into an explicit pay_clear_deadline.
Changes:
- Add domain-binding fields to protobuf messages (
chain_id,ledger_address) and enforce them at runtime inCelerLedger.openChannelandPayResolver. - Rename
last_pay_resolve_deadline→pay_clear_deadlineacross proto/Solidity/tests and update settlement gating semantics/docs accordingly. - Update interfaces, docs, and tests (including gas report snapshots) to reflect the new fields and naming.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/Proto.t.sol | Updates proto round-trip tests for new fields and renamed payClearDeadline. |
| test/utils/LedgerTestBase.t.sol | Updates deterministic channel-id derivation to include block.chainid; adds initializer domain fields in fixtures. |
| test/utils/Fixtures.sol | Extends fixture structs/encoders for chainId, ledgerAddress, and payClearDeadline (wire tags preserved). |
| test/gas_logs/fine_granularity/IntendSettle-OneState.txt | Refreshes gas snapshots after additional checks/fields. |
| test/gas_logs/fine_granularity/DepositEthInBatch.txt | Refreshes gas snapshots after additional checks/fields. |
| test/gas_logs/fine_granularity/ClearPays.txt | Refreshes gas snapshots after additional checks/fields. |
| test/gas_logs/PayResolver.txt | Refreshes PayResolver gas snapshots after added domain checks. |
| test/gas_logs/CelerLedger-ETH.txt | Refreshes ETH-path deployment/call gas snapshots after changes. |
| test/gas_logs/CelerLedger-ERC20.txt | Refreshes ERC20-path call gas snapshots after changes. |
| test/PayResolver.t.sol | Adds regression tests for wrong-chain and wrong-resolver replays; updates pay building to include chainId. |
| test/PayRegistry.t.sol | Aligns tests/comments with payClearDeadline terminology. |
| test/GasReport.t.sol | Updates gas report harness to include chainId and renamed payClearDeadline. |
| test/CelerWallet.t.sol | Updates wallet-id derivation expectation to include block.chainid. |
| test/CelerLedger.ETH.t.sol | Adds open-channel replay protection tests (wrong chain / wrong ledger) and multi-segment clearance regression tests. |
| src/lib/ledgerlib/LedgerStruct.sol | Renames peer state field to payClearDeadline. |
| src/lib/ledgerlib/LedgerOperation.sol | Enforces initializer domain binding; updates settlement gating and registry calls to use payClearDeadline. |
| src/lib/ledgerlib/LedgerChannel.sol | Renames getter to getPayClearDeadlineMap. |
| src/lib/interface/IPayResolver.sol | Documents new domain-binding requirements for pay resolution. |
| src/lib/interface/IPayRegistry.sol | Renames parameter to _maxResolveDeadline and clarifies semantics in NatSpec. |
| src/lib/interface/ICelerWallet.sol | Updates wallet-id derivation NatSpec to include chainid. |
| src/lib/interface/ICelerLedger.sol | Updates channel-id derivation NatSpec; documents initializer domain binding + token constraints. |
| src/lib/data/proto/entity.proto | Adds chain_id / ledger_address fields; renames last_pay_resolve_deadline to pay_clear_deadline (same tag). |
| src/lib/data/PbEntity.sol | Updates protobuf decoders/structs for new/renamed fields. |
| src/PayResolver.sol | Enforces pay.chainId == block.chainid and pay.payResolver == address(this) before resolving. |
| src/PayRegistry.sol | Renames getPayAmounts param and updates gating logic/comments accordingly. |
| src/CelerWallet.sol | Changes wallet-id derivation to include block.chainid. |
| src/CelerLedgerMock.sol | Renames mock getter/setter plumbing to payClearDeadline. |
| src/CelerLedger.sol | Renames public getter to getPayClearDeadlineMap. |
| docs/contracts.md | Documents supported-token constraints, wallet/channel id derivations, resolver rules, and renamed getter. |
| README.md | Adds “Supported tokens” section clarifying plain ERC-20-only assumption. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
… of operatorAddr Agent-Logs-Url: https://github.com/celer-network/agent-pay-contracts/sessions/e41b91f0-25b6-4807-84af-fd6617976e76 Co-authored-by: hhl42 <708956+hhl42@users.noreply.github.com>
Done in commit 8669201. Both the NatSpec in |
Summary
Two hardening themes:
PaymentChannelInitializercarrieschain_idandledger_address;ConditionalPaycarrieschain_id. Both validated on entry.walletIdprefixes
block.chainid, so downstreamchannel_id-anchored messagesbind transitively. Closes cross-chain and same-chain wrong-ledger replay
surfaces.
PayResolverand clarify pay-clearance semantics._resolvePaymentnow requirespay.payResolver == address(this).SimplexPaymentChannel.last_pay_resolve_deadlinerenamed topay_clear_deadline(=max(pay.resolveDeadline) + clearMargin) — themargin reserves wall-clock time for
clearPaysto land on multi-segmentpay lists.
PayRegistry.getPayAmounts's second parameter renamed to_maxResolveDeadlineso the registry's API describes its own domain.Notes
walletId = keccak256(chainid, walletAddr, creatorAddr, nonce).pay_clear_deadlinerename is not a wire-format break: same prototag (6), same
uint64. Off-chain producers regenerate bindings to pickup the new symbol; signed bytes are bit-identical.
docs/contracts.md, and relevant interface NatSpec updated. PlainERC-20 only; fee-on-transfer / rebasing / ERC-777 hooks are out of scope
(now stated in public docs).