fix: verify entityHash matches metadata in on-chain third-party validation#349
Conversation
Test this pull request
|
…ation The on-chain third-party asset validator was using the user-provided entityHash directly without verifying it matched the actual metadata. This could allow an attacker to deploy unauthorized content by reusing a valid merkle proof from a different entity they own. Add entityHash verification matching the subgraph path's verifyHash().
- Add test: modified metadata with reused merkle proof must fail because the entityHash in the proof won't match the keccak256 hash of the tampered metadata - Fix existing wearable test to assert response.ok is false when the checker contract fails (was only checking call counts)
099eead to
6cef385
Compare
Include the actual hash values in the log messages so operators can diagnose proof mismatches without additional debugging. Matches the logging pattern used by the subgraph third-party-asset validator.
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: PR #349 — Security Fix for On-Chain Third-Party Entity Hash Verification
Verdict: ✅ APPROVE
This is a critical security fix that closes an exploitable gap in the on-chain third-party validation path. The fix is correct and well-scoped.
What the fix does
Adds the missing entityHash verification to verifyMerkleProofedEntity in the on-chain path, mirroring the subgraph path's existing verifyHash():
MerkleProof.validate(merkleProof)— validates proof structurekeccak256Hash(metadata, merkleProof.hashingKeys)— computes hash from actual metadata- Rejects if computed hash ≠
merkleProof.entityHash— binds proof to metadata
Without this, the on-chain path used the user-supplied entityHash directly in generateRoot() without verifying it corresponded to the deployed metadata — allowing merkle proof reuse from a different entity.
Findings
P3 — Nice-to-Have:
-
[Testing] The tampered-metadata test in
emotes.spec.tsassertsresponse.okis falsy but does not check the error message or verify thatvalidateThirdPartywas not called. Since other failure modes also produceok: false, the test could pass for the wrong reason. Consider addingexpect(components.L2.checker.validateThirdParty).not.toHaveBeenCalled()to confirm rejection happened at the hash check stage. -
[Testing] The existing wearables test fix (
wearables.spec.ts) now properly assertsresponse.okis falsy — good catch on the missing assertion.
CI Status
All checks passing. Coverage stable.
Summary
This fix is correct, necessary, and urgent. The emote path was fully exploitable via merkle proof reuse. Recommend merging promptly.
Requested by Lautaro Petaccio via Slack
The tampered-metadata test now verifies that validateThirdParty was never called, confirming the rejection happened at the hash verification stage. Without this assertion, the test could pass for the wrong reason since other failure modes also produce ok: false.
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review — Focus on Breaking Changes & Lowercasing
✅ No breaking change risk. For legitimate deployments, the entityHash in the merkle proof is computed from the actual metadata, so keccak256Hash(metadata, hashingKeys) will reproduce the same hash. Only tampered deployments (mismatched proof + metadata) will be rejected — which is the entire point.
✅ No lowercasing concern. This PR doesn't involve any string case operations.
Security Assessment
This is a critical security fix. The on-chain path was missing the entity hash verification that the subgraph path already has. The test correctly validates the attack scenario (reusing a valid proof with tampered metadata).
One note
The MerkleProof.validate() call is a good addition — it provides structural validation before attempting the hash computation, preventing unexpected errors from malformed proof objects.
Verdict: Approve. Merge with high priority.
Requested by Lautaro Petaccio via Slack
Summary
The on-chain third-party asset validator accepts deployments without verifying that the
entityHashin the merkle proof actually corresponds to the deployed metadata. This allows an attacker who owns any third-party entity to deploy arbitrary content by reusing a valid merkle proof from a different entity they own.How the vulnerability works
The third-party validation flow has two paths: subgraph and on-chain. Both receive a deployment containing metadata and a merkle proof (
entityHash,proof[],index). The correct verification is:keccak256Hash(metadata, hashingKeys)from the actual deployed metadatamerkleProof.entityHash(binding the proof to this specific metadata)The subgraph path (
subgraph/third-party-asset.ts) does all three steps correctly in itsverifyHash()function. The on-chain path (on-chain/third-party-asset.ts) skipped step 2 — it used the user-providedmerkleProof.entityHashdirectly ingenerateRoot()without checking it matched the metadata.Attack scenario
For wearables, this is independently caught by
thirdPartyWearableMerkleProofContentValidateFninitems/wearables.tswhich re-hashes the metadata and verifies. But emotes have no equivalent check, making third-party emotes on the on-chain path exploitable.What this PR does
Adds entity hash verification to
verifyMerkleProofedEntityin the on-chain path, mirroring the subgraph path'sverifyHash():MerkleProof.validate()keccak256Hash(metadata, merkleProof.hashingKeys)merkleProof.entityHashThis ensures the proof is cryptographically bound to the deployed metadata regardless of entity type.
Test plan
npm run buildcompiles cleanlynpm test— all existing tests pass (the fix is additive; valid deployments still pass because their entityHash already matches)entityHashis rejected on the on-chain path🤖 Generated with Claude Code