Skip to content

Fix: add_new_entry converts block timestamps in local time instead of UTC - #1208

Open
pucedoteth wants to merge 1 commit into
SunWeb3Sec:mainfrom
pucedoteth:fix-utc-timestamp
Open

Fix: add_new_entry converts block timestamps in local time instead of UTC#1208
pucedoteth wants to merge 1 commit into
SunWeb3Sec:mainfrom
pucedoteth:fix-utc-timestamp

Conversation

@pucedoteth

Copy link
Copy Markdown

Problem

TransactionManager.get_timestamp_from_tx_hash converts the block timestamp with:

dt_object = datetime.fromtimestamp(unix_timestamp)

datetime.fromtimestamp() without a tz argument renders in the timezone of the machine running the script. Block timestamps are UTC seconds since epoch, so the result depends on who ran add_new_entry.py.

That timestamp is not cosmetic — it drives two things:

consumer code
README entry date timestamp.strftime("%Y%m%d")20250918 NGP
PoC directory timestamp.strftime("%Y-%m")src/test/2025-09/

So the same attack tx yields a different README date, and possibly a different month directory, for a contributor in Taipei vs. Berlin vs. New York.

The repo records UTC

Checked existing entries against chain data. The discriminating case is one where UTC and UTC+8 fall on different days:

entry attack tx block time (UTC) UTC date UTC+8 date README says
IdolsNFT 2025-01-14 17:31 20250114 20250115 20250114 ✅ UTC
NGP 2025-09-17 19:02 20250917 20250918 20250918

20250918 NGP is a day late — exactly the symptom of a UTC+8 machine having generated it.

The existing test already catches this

test_get_timestamp_from_tx_hash_success uses block timestamp 1647888693 and asserts assertIn("Mar-22-2022", result), with the comment # Tue Mar 22 2022 02:51:33 GMT+0000.

1647888693 is actually Mon Mar 21 2022 18:51:33 UTC. Mar 22 2022 02:51:33 is that instant in UTC+8 — the comment labels an author-local rendering as GMT. As a result the suite only passes from about UTC+6 eastward:

=== before this PR ===
TZ=UTC                 FAILED (failures=1)
TZ=Asia/Taipei         OK
TZ=America/New_York    FAILED (failures=1)
TZ=Europe/Berlin       FAILED (failures=1)

Fix

  • datetime.fromtimestamp(unix_timestamp, tz=timezone.utc) — block timestamps rendered as UTC.
  • The test's expectation corrected to the real UTC value (Mar-21-2022 06:51:33 PM) and tightened from assertIn to assertEqual, plus the misleading comment fixed.
  • The datetime.now() fallbacks (used when no tx hash / no timestamp is given) routed through a small utc_now() helper, for the same reason. It returns a naive datetime, so every existing strftime/strptime path is unchanged.
=== after this PR ===
TZ=UTC                 Ran 53 tests  OK
TZ=Asia/Taipei         Ran 53 tests  OK
TZ=America/New_York    Ran 53 tests  OK
TZ=Pacific/Kiritimati  Ran 53 tests  OK

The existing 20250918 NGP entry is left alone — happy to correct it in this PR if you'd like.

@SunWeb3Sec

Copy link
Copy Markdown
Owner

🤖 Automated PR review — outside automation scope, human review needed

This automated reviewer verifies PoC submissions by (1) running forge test on new src/test/**/_exp.sol files and (2) validating the corresponding README.md incident-list diff.

This PR doesn't fit that shape:

  • ❌ No new PoC (*_exp.sol) file is added.
  • ❌ No README.md changes.
  • ⚠️ The only way to verify the fix is to run the PR's own modified test.py — but this reviewer's guardrails restrict code execution to forge test/forge build against PoC contracts and the maintainer's own check_readme.py; it is never permitted to execute a script the PR itself adds or modifies (including via python/python3), regardless of how reasonable the PR's own test output claims look.

The change itself (switching datetime.fromtimestamp() to UTC-aware parsing in add_new_entry.py, and correcting the timezone-dependent test assertion) looks like a legitimate, well-documented bugfix based on the PR description — but a maintainer will need to review and merge it manually, since it falls outside what this bot is authorized to auto-verify.

No action needed from the contributor — this is not a rejection, just a routing note for a human maintainer.

@SunWeb3Sec

Copy link
Copy Markdown
Owner

🤖 Automated PR review — out of scope for auto-verification

Thanks for the detailed writeup and the UTC timezone fix! This PR isn't something the automated reviewer can validate or auto-merge, for reasons specific to how it operates rather than any problem found in the change itself:

  • This bot's job is to verify PoC contributions: run forge test on a newly added src/test/**/_exp.sol file and validate the corresponding README.md incident-count/list update. This PR touches neither — it modifies add_new_entry.py and test.py (tooling/test-harness code), so there's no PoC or README diff for the automated checks to run against.
  • To actually verify the timezone fix, the bot would need to execute test.py as modified by this PR. Its guardrails only allow it to execute forge test/forge build against PoC .sol files and the reviewer's own bundled check_readme.py — it's explicitly barred from running any script that a PR itself adds or modifies. So it can't independently confirm the TZ=UTC/TZ=America/New_York/etc. results quoted in the PR description.

This looks like a solid, well-reasoned fix (converting datetime.fromtimestamp() to explicit UTC, correcting the mislabeled test fixture) — it just needs a human maintainer to review the Python change and run the test suite directly rather than going through this automated PoC pipeline. Leaving open for manual review.

get_timestamp_from_tx_hash rendered the block timestamp with
datetime.fromtimestamp(unix_timestamp), which uses the timezone of the
machine running the script. That timestamp decides the README entry date
(%Y%m%d) and the src/test/YYYY-MM directory a new PoC lands in, so the
same transaction produces different dates for different contributors.

Block timestamps are UTC and the repo records hack dates in UTC: the
IdolsNFT attack landed 2025-01-14 17:31 UTC and is listed as 20250114,
not 20250115. The 20250918 NGP entry is off by a day for the same
reason - that attack was 2025-09-17 19:02 UTC.

The existing unit test already caught this but only passes at UTC+6 and
east; its expected value was the author's UTC+8 rendering, labelled
GMT+0000. Corrected to the real UTC value so the test is deterministic.

The datetime.now() fallbacks are routed through a utc_now() helper for
the same reason; it stays naive so all existing formatting is unchanged.
@SunWeb3Sec

Copy link
Copy Markdown
Owner

🤖 Automated PR review — outside automated scope, needs manual review

Thanks for the detailed writeup and the fix! This PR doesn't fit the pattern this automated reviewer is built to verify, so I'm leaving it open for a maintainer to review by hand rather than merging it automatically.

Why:

  • This reviewer's two checks are (1) forge test on a new PoC .sol file, and (2) validating a README.md incident-list diff. This PR touches neither — it changes add_new_entry.py and test.py only (tooling used to generate future incident entries), with no new PoC and no README edit.
  • Per this reviewer's operating guardrails, the only code it's permitted to execute is forge test/forge build against a submitted PoC and the reviewer's own check_readme.py validator. Running the PR's own test.py (a script the PR modifies) to verify the timezone-handling claims is explicitly out of bounds for an automated pass, no matter how reasonable the fix looks.

The technical argument (UTC vs. local-time in datetime.fromtimestamp, and the mislabeled GMT+0000 test comment) reads as sound on inspection, but a human should confirm the test.py results and decide whether to also correct the existing 20250918 NGP README date as offered in the PR description.

No action needed from you — a maintainer will take it from here.

@SunWeb3Sec

Copy link
Copy Markdown
Owner

🤖 Automated PR review — outside automated scope

This automated reviewer verifies PoC contributions (forge test on a new src/test/**/*_exp.sol, plus the corresponding README.md incident-list update) and auto-merges on a full pass.

This PR doesn't fit that shape:

  • No new _exp.sol PoC file is added — nothing for forge test to verify.
  • README.md is not modified.
  • The change is to repo tooling (add_new_entry.py, test.py), including modifying the existing test suite's expectations.

Since this reviewer's command allow-list only permits running forge test/forge build against a PR's Solidity PoC and this skill's own check_readme.py validator, it does not execute PR-supplied Python (e.g. the updated test.py) to verify this change.

Leaving this open for manual maintainer review rather than merging or rejecting automatically — the analysis in the PR description (UTC vs local-time timestamp handling) looks reasonable on read-through, but should be confirmed by a human before merging a change to the entry-generation tooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants