Security remediation: x402 SDK / MCP audit 2026-08-06 - #12
Draft
coinsecuritiescompany wants to merge 84 commits into
Draft
Security remediation: x402 SDK / MCP audit 2026-08-06#12coinsecuritiescompany wants to merge 84 commits into
coinsecuritiescompany wants to merge 84 commits into
Conversation
Member
Review — contract and deployment assumptionsReviewed from the deployed-architecture side, as asked. The security work here is strong: runtime codehash pinning plus live One blocking issue, and it is a sequencing problem rather than a flaw in the logic. There is currently no payable route on any chain
if (target.version !== "1.3") reject("fee_inclusive_splitter_disabled");The only entry in There is exactly one The documentation added in this PR states "Only Polygon v1.2 is enabled", which contradicts the validator in the same change. One of the two needs to move:
Blocking fee-inclusive routes is the right call — paying a v1.1/v1.2 contract with fee-on-top amounts mis-settles. The issue is only that the current state is not what the docs describe. Release order is now load-bearingBecause the registry pins
Publishing the SDK before step 1 ships a package that cannot pay anyone. The evidence window expires on 3 September 2026
Governance note for the v1.3 deploymentThe registry records Optimism, BOT Chain and XRPL EVM as controlled by the single EOA v1.3 changes the function selectorv1.3 passes the merchant amount explicitly, so the signature becomes Verified on-chain, for the recordRe-read from each chain today: Polygon v1.2, Base v1.1, Optimism v1.2, Unichain v1.1, BOT Chain v1.2, XRPL EVM v1.2 — all six SDK addresses correct, every declared version matching the deployed contract. |
main carried the AIFP-1 client and a refactor that extracted the splitter settlement into settleSplitterNative(). This branch had removed the thing that refactor was extracting: settlement is v1.3 fee-on-top only, against a splitter resolved from the verified registry, with no server-steered version selection. Taking main's version back would have reinstated the v1.1/v1.2 dynamic ABI route, so it was not taken. AIFP-1 is preserved in full and now settles through the same validated path as call(). The gateway already quotes merchant/treasury/creator components, so those are forwarded verbatim and validateQuotedNativePayment checks them against the registry's fee-on-top model rather than the settling side trusting a total. The splitter address and version are not read from the gateway — it sends neither — so they come from the registry. That has a consequence worth stating plainly: Polygon's registered splitter is still v1.2, so EVM settlement — bridge and AIFP-1 alike — fails closed today with fee_inclusive_splitter_disabled. It stays closed until v1.3 is deployed and the registry entry is updated. A new test asserts the refusal against the real settlement path so it cannot be relaxed quietly. Also in this merge: - aifp1.test.ts adapted to the branch's in-memory budget caps; the reservation ledger it imported was deleted here. The property it tested — a failed settlement must not consume budget — is unchanged and still asserted. - splitterVersionDetect.test.ts stays deleted. It asserted the precedence between a declared version and on-chain detection, which is the mechanism that was removed; it cannot be updated to a contract that no longer exists. legacySplitterRouteRemoved.test.ts replaces it and asserts the removal. - Package metadata: versions from main (1.8.0 / 1.5.0, already published, so the candidate cannot start lower), descriptions from this branch, because main's advertise settlement on rails this branch deliberately gates. MCP now depends on ^1.8.0 rather than ^1.7.1. - Lockfiles regenerated from the resolved manifests. Production audit reproduced afterwards: agent 0 critical / 0 high (3 moderate), mcp 0/0 (6 moderate), so the high-severity fixes from main are still in. Node: build green, 162 tests. MCP: build green, 34 tests.
The five failures were old tests describing a protocol that no longer exists: they called the retired _sign_nonce primitive and drove a /nonce endpoint the v2 flow does not have. Restoring either would have meant reinstating unbound generic nonce signing, so neither was restored — the tests were rewritten against the AiFinPay-x402-v2 contract that replaced it. What they assert now: - Determinism over the whole bound payload rather than over a bare nonce. - Every bound field is actually covered by the signature: nonce, agent, method, resource, expiry, min_usd and agreement hash each change it. This is the property that stops a signature for one request authorising another. - A hostile origin is refused before the signer is reached, asserted by spying on _sign_message rather than on a network call the flow no longer makes. Lookalike hosts and a scheme downgrade are refused too. - Unbound and self-contradicting challenges fail closed: no signing block, no nonce, legacy message_version, a resource that does not match the response URL, unparseable expiry, non-numeric minimum, malformed agreement hash, and signing terms that disagree with the body. - Expired challenges are refused. - A 402 reached by a redirect off the trusted origin is refused on the origin check, so signing cannot follow a redirect. This replaces the old /nonce redirect test; the risk moved with the protocol rather than disappearing. Also removes the last active references to the retired agent_claim_self tool from the root README, which still advertised seven MCP tools. Six are registered. The notes in mcp/README.md and mcp/src/server.ts stay: they record that the tool was removed and why. The _sign_nonce / _fetch_nonce / auth_headers stubs in client.py are left alone — they raise on use, which is the fail-closed tombstone. Python: 105 passed, 0 failed (was 88 passed / 5 failed).
Two CI failures from the reconciliation, both mine. Lockfiles: I regenerated them with npm 11, which writes a tree npm 10 will not accept — CI runs Node 20, so `npm ci` failed on both packages with "Missing: utf-8-validate@5.0.10 from lock file". It passed locally, which is exactly how this got through. Regenerated with npm 10 and verified by running `npm ci` under npm 10 for both packages. Versions: the bump gate refuses published files changing under a published version, and it was right to. Set to 1.9.0-rc.1 and 1.6.0-rc.1 — release candidates, so no one can mistake either for a release. New feature (AIFP-1) plus a settlement path that now refuses where it previously succeeded; there is a fair argument that the second makes it a major, and that call belongs to the release step rather than to this branch. MCP's dependency stays at ^1.8.0 rather than the candidate: the RC is not published and must not be, so a range naming it cannot resolve. ^1.8.0 is satisfied by the eventual 1.9.0, and CI already builds MCP against this PR's local SDK in its own step, which is what §5.4 asks for. Verified after regeneration: npm ci (npm 10), build, test and `npm pack --dry-run` on both. Node 162 tests, MCP 34. Production audit unchanged — agent 0 critical / 0 high, mcp 0 critical / 0 high.
§6.2 wants proof that the SDK encodes the entrypoint the deployed contract actually has. The registry tests already refused v1.1/v1.2 targets, but nothing checked the bytes. The selector is asserted as the literal 0x894eb1f3 rather than derived from the ABI under test — deriving it from the same ABI would pass even if that ABI drifted away from the contract. It was read off the compiled B2BSplitterV13 artifact. The argument order is checked word by word, with merchantAmount as its own word, which is the thing that distinguishes v1.3 from v1.2 where the merchant's share was implied by the total. Also refuses a quote naming a legacy function_signature, and re-asserts that a target declaring version 1.1 or 1.2 is rejected before any calldata is built. SPLITTER_PAY_NATIVE_ABI is exported so the test encodes the same constant the signer uses rather than a copy that could drift. 167 tests passing (was 162).
The clean-machine check §14 asks for turned up something CI could not see. Installing both RC tarballs into an empty project resolved a nested @aifinpay/agent@1.8.0 under @aifinpay/mcp — the published, unremediated build — because MCP declares ^1.8.0 and npm does not admit a prerelease into a stable range. MCP resolves to that nested copy at runtime. Nothing failed. The install succeeded, CI was green, and MCP was quietly running against the SDK this candidate exists to replace. CI missed it because the local SDK was installed AFTER the build and test steps, so the tests ran against the registry copy and the override only affected a final rebuild. The swap now happens first, so the tests exercise the SDK that will actually ship. scripts/check-mcp-sdk-pin.mjs makes the manifest itself the thing that is checked, since that is what a consumer running plain `npm install` gets. While the SDK is an unpublished RC the manifest cannot name it — npm ci could not resolve it — so the check reports the blocker and exits 0, and fails hard as soon as the SDK version becomes stable. A stable release cannot ship the mismatch. It deliberately does not reimplement semver. It enforces the one narrow rule that covers this failure: while the SDK is a prerelease, MCP must name it exactly, because that is the only form npm resolves by default. If the versioning scheme outgrows that, add the real dependency rather than widening the guesswork. docs/CLEAN_MACHINE_EVIDENCE.md records the packing, secret scan, import test and the full §14 gate table, including the gates that cannot pass yet because v1.3, Solana v0.6 and Casper v2 are not deployed.
The SDK carried a hand-written address table. Every value in it turned out to be correct — the chain confirmed all six addresses, both treasuries and every fee split — but nothing checked that, and the August audit lost a day to the SDK and a stale deployments.json disagreeing with no way to tell which was right. SPLITTER_DEPLOYMENTS is now built from src/generated/splitter-table.json, vendored from AiFinPay/evm-contract where it is generated from the canonical registry and verified there against live chain state. Everything that decides where money goes comes from that file: address, version, runtime code hash, treasury, fee split, validity window and whether settlement is enabled. What stays local is what a chain cannot tell you — the viem chain object, a default RPC, the explorer, the local USDC address and which env var carries the native price. Those are kept in a separate CHAIN_METADATA map so the split is visible: getting one wrong degrades the experience, getting a registry value wrong sends money to the wrong place. Two import-time guards: a network with no registry entry throws rather than falling back to a hardcoded address, and a registry chainId that disagrees with the viem chain throws rather than building a target from mismatched chains. scripts/sync-registry.mjs vendors the canonical file and --check fails on drift, wired in as npm run registry:check. Demonstrated by editing the vendored polygon treasury to 0xdead… — exit 1, and re-syncing restores it. The fetch fails closed: an unreachable source is not a pass, because silently keeping the local copy would let the SDK drift for as long as the fetch kept failing. Versions set to 2.0.0 for both packages as approved. This also resolves the nested-SDK blocker from the clean-machine check: a prerelease could never satisfy a stable range, which is why MCP was pulling @aifinpay/agent 1.8.0. With both at 2.0.0 a clean install of the two tarballs resolves exactly one copy of the SDK, verified. check-mcp-sdk-pin.mjs is therefore a hard gate now rather than a report. MCP's CI builds the SDK and installs it by path, since 2.0.0 is not published yet and npm ci cannot resolve it. That edit is CI-only and never committed, and the pin check runs against the committed manifest before it happens. Node 169 tests, MCP 34, both green.
The verification line I added to the MCP job did
require('@aifinpay/agent/package.json'), which the package's exports map does
not expose, so the step failed with ERR_PACKAGE_PATH_NOT_EXPORTED and took the
build down. The diagnostic, not the thing being diagnosed.
npm ls @aifinpay/agent prints the same fact without going through exports, and
also shows when the resolution is a local link rather than a registry copy,
which is exactly what this step exists to confirm.
A stray "npm test" line was left under the restore step by an earlier edit. YAML folded it into the same scalar, so the command ran as "git checkout -- package.json npm test" and failed on two pathspecs that are not files. Build, install, tsc and the tests had all passed by then; only my cleanup step broke. Also restores package-lock.json alongside package.json. npm install rewrote both to point at the local SDK path, and leaving the lockfile modified would mean the tarball verified in the next step is not the publishable one.
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
2 participants
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.
Security and settlement candidate
solana|polygon;Evidence
Candidate SHA:
1f400fc84930842f1b93246e2f10f24817d069a3GitHub SDK CI run
31280955506— success.Clean local gates:
Release boundary
Nothing is published. Polygon, Solana and Casper settlement remain fail-closed until the registry is backed by a reviewed matching deployment and controlled paid E2E evidence. Address/balance inventory is not treated as settlement support.