From 7e921e506cd72c0cbf5e5dc852047f03426cc345 Mon Sep 17 00:00:00 2001 From: satyakwok Date: Thu, 21 May 2026 14:21:09 +0200 Subject: [PATCH] fix(audit): tighten ERC-721 decode to require empty data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit M-4 follow-up against #46. The Transfer selector `0xddf252ad…` is shared by: - ERC-20 `Transfer(address indexed, address indexed, uint256)` — three topics + 32-byte unindexed data carrying amount. - ERC-721 `Transfer(address indexed, address indexed, uint256 indexed)` — four topics, no unindexed data. Custom events with three indexed args + unindexed data (eg. `Transfer(address indexed, address indexed, address indexed, uint256)`) hit the same selector. The original decoder distinguished only on topic count, so any such custom event got recorded as an ERC-721 with a fabricated `amount = 1` and a token_id taken from whatever the third indexed argument happened to be. That polluted `token_transfers` with false NFT rows and broke balance aggregation on the indexer's `/accounts/{addr}/tokens` endpoint. Per ERC-721 spec the Transfer event has NO unindexed data. Add a data-must-be-empty gate to the ERC-721 path; return `None` if data is present so the row drops out of the index entirely. Real NFT contracts emit empty data and continue to decode correctly. Real custom events with three indexed args fall through silently — the cost is missing them from `token_transfers`, but they weren't meaningfully decoded before either. Adds a regression test (`skips_three_indexed_args_with_nonempty_data`). --- crates/sync/src/token_decode.rs | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/crates/sync/src/token_decode.rs b/crates/sync/src/token_decode.rs index 361aa0d..203b31b 100644 --- a/crates/sync/src/token_decode.rs +++ b/crates/sync/src/token_decode.rs @@ -35,7 +35,23 @@ pub fn decode_transfer(log: &Log) -> Option { let (standard, token_id, amount) = match log.topic3.as_deref() { Some(id_topic) => { - // ERC-721: token_id in topic3, data empty (or padding only). + // ERC-721: per spec the Transfer event has exactly three + // indexed args (from, to, tokenId) and NO unindexed data. + // 2026-05-21 (audit M-4): also require data to be absent / + // empty. Custom events with three indexed args + non-empty + // data SHARE topic0 with ERC-721 Transfer but are not NFT + // transfers — decoding them as ERC-721 produced spurious + // token_transfers rows with a fabricated `amount = 1`. + // Drop those by returning None; if the contract turns out + // to be a real NFT we can revisit with a registry probe. + let data_empty = log + .data + .as_deref() + .map(|d| d.trim_start_matches("0x").is_empty()) + .unwrap_or(true); + if !data_empty { + return None; + } let token_id = topic_to_u256(id_topic)?; ( TokenStandard::Erc721, @@ -152,4 +168,17 @@ mod tests { log.topic1 = Some("0xshort".into()); assert!(decode_transfer(&log).is_none()); } + + #[test] + fn skips_three_indexed_args_with_nonempty_data() { + // Custom event Transfer(address,address,address,uint256) emits the + // same topic0 selector as ERC-721 Transfer but carries unindexed + // data. Audit M-4 requires we NOT decode this as ERC-721 — the + // resulting `amount = 1` row would be a fabrication. + let mut log = base_log(); + log.topic3 = + Some("0x0000000000000000000000000000000000000000000000000000000000000007".into()); + // Same data as ERC-20 base_log: non-empty 32-byte payload. + assert!(decode_transfer(&log).is_none()); + } }