Repository navigation
[WNS-63] Authex: test the SVM createlink sweep and document import-before-link - #695
Conversation
CertiK WNS-63 (WIRE-413) is fixed on master by #594 (links sweep unmapped credits), #639 (credits never expire) and #670 (onreward retired). The SVM createlink sweep had no unit test; the new one also checks that a credit sharing the key's 8-byte bychainad prefix stays put. A link sweeps once and createlink does not check that the DClaim import is sealed, so the upgrade-order doc now says to finish the import before the first link. Change-Id: I50e69a120633181dd3d9e0dd100c3e5a663af844
| as part of the deploy, so there is no separate privilege step. The durable | ||
| precondition is ordering: deploy DClaim before any link action can execute. | ||
|
|
||
| Finish the pre-launch import (`importseed`, then `importdone`) before the first |
There was a problem hiding this comment.
High — make this prerequisite executable and fail closed. The numbered rollout below skips funding, importing, reconciliation, and sealing and proceeds directly from DClaim deployment to AuthX deployment and link smoke tests. The only converter shipped in this repository cannot safely fill that gap: it emits CHAIN_KIND_ETHEREUM / CHAIN_KIND_SOLANA while the DClaim ABI accepts CHAIN_KIND_EVM / CHAIN_KIND_SVM, and it treats absent purchasers / stakers arrays as an empty import. Running it against the current Solana frozen-export shape used by Wire-Network/wire-tools-ts#151 exits successfully with [] and zero credits. If importdone then seals that result, later importseed calls revert and the omitted credits remain stranded absent a migration. Please update and fixture-test the converter and add explicit fund/import/reconcile/seal steps before links, or explicitly block rollout on a landed and current-head-validated wire-tools-ts#151.
There was a problem hiding this comment.
Agreed on the converter — removed in bb79c42. It emitted CHAIN_KIND_ETHEREUM/CHAIN_KIND_SOLANA, scaled Solana pretokens as 9-decimal, and returned an empty import for the current Solana export. The numbered WIRE-352 steps are the upgrade procedure for a chain already running pre-#594 contracts; launch is a fresh deployment, which the paragraph now covers: fund → importseed → reconcile → importdone before the first link. The cluster bootstrap's LCO import performs it, and launch depends on wire-tools-ts#151: its new reconcile-lco-claims step (74436eb2) checks sysio.dclaim::unmapped against the plan and stops before importdone on any missing, extra or wrong row.
| `unmapped`, and its holder cannot trigger the sweep again: `createlink` accepts | ||
| one link per account and chain, and only `sysio.authex` may send `linkswept`. A | ||
| credit imported after its key is linked stays in `unmapped`, still owed, until | ||
| an operator sends `linkswept` for it as `sysio.authex`. |
There was a problem hiding this comment.
Medium/security — reserve this recovery for governance. sysio.dclaim::linkswept requires sysio.authex authorization but does not verify that the supplied Wire account matches the AuthX link; it moves the full matching unmapped balance to the supplied account. An ordinary operator cannot authorize this action, while delegating sysio.authex@active would let that delegate redirect parked credits. For a trusted recordlink, use the identical replay path already documented above. For other stragglers, please state that governance / sysio.msig submits linkswept as sysio.authex@active, with the tuple derived and reconciled against the existing AuthX link.
There was a problem hiding this comment.
Agreed — reworded in bb79c42: governance submits linkswept as sysio.authex@active through sysio.msig, with the account, chain and address of the existing link; a node-owner link can instead replay its identical recordlink. The recordlink table row and the authex comments now say governance too.
…ep-hardening Change-Id: Ic4b83f5b5ab4130e8ab9631c1241f6f182135b0f
…ink recovery convert_import.py emitted CHAIN_KIND_ETHEREUM/SOLANA, which the DClaim ABI rejects, scaled Solana pretokens as 9-decimal (they are 8), and produced an empty import from the current Solana export. The LCO import in wire-tools-ts replaces it. The upgrade-order doc now orders fund, import, reconcile and seal before the first link. A credit imported after its key was linked is recovered by governance through sysio.msig, because linkswept does not check the account against the link; the recordlink row and the authex comments say the same. Change-Id: I0616c8d8f35928ac2acd95c92ad1f411b9d14281
| `linkswept`. `linkswept` does not check the account against the link, so | ||
| recovering a credit imported after its key was linked is for governance: it | ||
| submits `linkswept` as `sysio.authex@active` through `sysio.msig`, with the | ||
| account, chain and address taken from the existing link. For a node-owner link, |
There was a problem hiding this comment.
[P2] Derive and verify the address before recovery
The links row cannot supply this address: links_s stores only username, chain_kind, and pub_key. Since dclaim::linkswept looks up the caller-supplied (chain, address) and credits the caller-supplied account without consulting AuthX, a proposal built with the wrong 20/32 bytes can move a parked credit belonging to another holder into this account. Please spell out reconstruction from the stored key (SVM: raw ED bytes; EVM: the canonical 20-byte address derived from the stored EM key) and require governance to verify the link tuple and exact target unmapped row before proposing linkswept or replaying recordlink; the link row does not retain the original native address.
There was a problem hiding this comment.
Agreed — the link row keeps only the public key. Reworded in 2a36e12: governance derives the address from the stored key (SVM: the raw 32-byte ED key; EVM: the canonical 20-byte address of the stored EM key, the last 20 bytes of keccak256 of its uncompressed form) and checks the link and the exact target unmapped row before proposing linkswept or replaying recordlink.
…ep-hardening Change-Id: I7290c8a1717e4c993335ad0a637f5c1ce9c040cb
…covery The links row keeps only the account, chain and public key, so the recovery paragraph now has governance derive the address from the stored key (SVM: the raw ED key; EVM: the 20-byte address of the EM key) and check the link and the exact unmapped row before proposing linkswept or replaying recordlink. Master is merged in. Change-Id: I23ce4dd13217de54b7864b3b5cb1a0677be3eeda
| as `sysio.authex@active`. The `links` row keeps only the account, chain and | ||
| public key, not the native address, so governance derives the address from the | ||
| stored key (SVM: the raw 32-byte ED key; EVM: the canonical 20-byte address of | ||
| the stored EM key, the last 20 bytes of keccak256 of its uncompressed form) and |
There was a problem hiding this comment.
[P2] Exclude the SEC1 prefix from the EVM hash
In this tree an uncompressed EM key is the 65-byte SEC1 serialization 0x04 || X || Y, but both evm_address_from_uncompressed_key and fc::crypto::ethereum::to_address deliberately hash only the 64-byte X || Y payload. Following “keccak256 of its uncompressed form” literally and hashing all 65 bytes yields a different address, so this recovery will not find the intended unmapped row. Please specify “the trailing 20 bytes of keccak256(X || Y), excluding the leading SEC1 0x04,” or require a standard EVM-address helper.
There was a problem hiding this comment.
Agreed — fixed in 2944e90: the doc now specifies the trailing 20 bytes of keccak256(X || Y) over the 64-byte coordinates, without the leading SEC1 0x04, and names opp::evm_address_from_uncompressed_key.
…ep-hardening Change-Id: I016a42c1278ddc3236f09c8c3e067462ee84c4fc
…dress The EVM address is the trailing 20 bytes of keccak256 over the 64-byte coordinates; hashing the 65-byte key with its SEC1 0x04 prefix gives another address, and the recovery would miss its unmapped row. The doc now says so and names opp::evm_address_from_uncompressed_key. Master is merged in. Change-Id: Icbea2d71f4e36f141a074228ed259e535a6225ae
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed current head 2944e90. All prior findings are resolved, including the fail-closed import sequence, governance-only recovery, native-address reconstruction, and explicit EVM keccak256(X || Y) derivation excluding the SEC1 prefix. The full diff and current-master merge are clean; retained local Clang/JIT validation and the companion current-head LCO E2E are applicable. No actionable findings.
Summary
Closes the open items of WIRE-413 (CertiK WNS-63). The finding, audited at
697c35a986, is already fixed on master:ef4bc92048(WIRE-352: sweep linked rewards into dclaim #594, WIRE-352):createlinkandrecordlinksendsysio.dclaim::linksweptwith the native address (EVM: the last 20 bytes of keccak256 of the recovered key; SVM: the 32-byte key).createlinkrefuses to link untilsysio.dclaimis deployed and privileged.f3a887e0dd(WIRE-339/WIRE-353: keep all four claim ledgers claimable indefinitely #639, WIRE-339/353): DClaim balances never expire;flushexpiredis gone.9e7bb2e0b2(Retire reward and reserve creation carriers #670):onrewardis retired, soimportseedis the only writer ofunmapped.The code grant CertiK recommends is not needed:
sysio.authexdeploys privileged throughsetsyscode, and privileged contracts skip inline authorization.This PR closes the gaps those changes left:
createlinksweep had no unit test; only EVMcreatelinkand SVMrecordlinkdid.createlinkdoes not check that the DClaim import is sealed.docs/contract-upgrade-order.mdnow orders the fresh-deployment import before the first link: fundsysio.dclaim, push everyimportseedbatch, reconcile theunmappedrows against the export, thenimportdone. The cluster bootstrap's LCO import runs these steps; wire-tools-ts#151 adds the reconcile before the seal.linksweptmoves theunmappedrow matching the supplied chain and address without consulting the link, so recovering a credit imported after its key was linked is governance's, throughsysio.msigassysio.authex@active. The link row keeps only the public key, so governance derives the native address from it (SVM: the raw ED key; EVM: the 20-byte address of the EM key) and checks the link and the exactunmappedrow before proposinglinksweptor replayingrecordlink. Therecordlinktable row and the authex comments now say governance too.contracts/sysio.dclaim/tools/convert_import.pyis removed. It emittedCHAIN_KIND_ETHEREUM/CHAIN_KIND_SOLANA, which the DClaim ABI rejects (it takesCHAIN_KIND_EVM/CHAIN_KIND_SVM), scaled Solana pretokens as 9-decimal (they are 8), and produced an empty import from the current Solana export. The LCO import in wire-tools-ts replaced it.Letting an identical
createlinkre-send the sweep is not needed:importdoneseals the only writer ofunmapped, so no credit can arrive behind a link once the bootstrap import is done.Tests
createlink_svm_sweeps_prelink_dclaim_rewards: imports a credit for the linking ED key and one for an address with the same first 8 bytes, links, and checks that only the first moves topclaims. It fails against the auditedsysio.authex.wasm, whosecreatelinksends no sweep.contracts_unit_test -- --sys-vmpasses 922/922 with master merged in. Import, link and claim run end to end inflow-lco-import(wire-tools-ts#151).