Skip to content

fix: reject future timestamps for Lock/Decrease/Unlock modes#64

Open
re1ro wants to merge 1 commit into
mainfrom
fix/lock-future-timestamp-guard
Open

fix: reject future timestamps for Lock/Decrease/Unlock modes#64
re1ro wants to merge 1 commit into
mainfrom
fix/lock-future-timestamp-guard

Conversation

@re1ro

@re1ro re1ro commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

The only future-timestamp guard lived inside _processIncreaseOrUpdate, so only the Increase mode (modeOrExpiration > 3) rejected future timestamps. The Lock, Decrease, and Unlock modes flow through _processAllowanceOperation and never hit that check.

Because _lockAllowance stored the user-supplied timestamp verbatim, and Unlock requires a timestamp strictly greater than the stored lock timestamp (_validateLockStatus), a Lock signed with timestamp = type(uint48).max produced a permanently un-unlockable allowance — a footgun for any owner (or relayer) that signs a Lock with a bad timestamp.

Fix

  • Move the future-timestamp guard up into _processAllowanceOperation (after the ZeroToken/ZeroAccount checks, before _validateLockStatus) so all timestamp-storing modes reject future timestamps.
  • Remove the now-redundant identical check from _processIncreaseOrUpdate, which no longer reads block.timestamp and is now pure.
  • The TransferERC20 mode (0) skips _processAllowanceOperation and carries no timestamp, so it is unaffected.

Tests

  • Adds test_lockTimestampInFuture: signs a Lock permit with a future timestamp and asserts it reverts with InvalidTimestamp.
  • Updates existing Lock/Decrease/Unlock tests that incidentally passed a future timestamp (block.timestamp + 100 / + 1). These now either advance the chain clock with vm.warp (where the test legitimately needs an unlock timestamp newer than the lock timestamp) or use the current block time (where the timestamp value was immaterial to what the test verifies). The production guard was not weakened.

Full suite: 214 passed, 0 failed.

🤖 Generated with Claude Code

The only future-timestamp guard lived inside _processIncreaseOrUpdate, so
just the Increase mode rejected future timestamps. Lock, Decrease, and
Unlock flowed through _processAllowanceOperation and never hit the check.

Because _lockAllowance stored the user-supplied timestamp verbatim and
Unlock requires a timestamp strictly greater than the stored lock
timestamp, a Lock signed with timestamp = type(uint48).max produced a
permanently un-unlockable allowance.

Move the guard up into _processAllowanceOperation (after the zero-checks,
before _validateLockStatus) so all timestamp-storing modes reject future
timestamps, and remove the now-redundant check from _processIncreaseOrUpdate
(which becomes pure). The TransferERC20 mode skips _processAllowanceOperation
and carries no timestamp, so it is unaffected.

Adds test_lockTimestampInFuture; updates existing Lock/Decrease/Unlock tests
that incidentally passed future timestamps to advance the clock (vm.warp) or
use the current block time instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 23, 2026 18:08
@eco-ai-app

eco-ai-app Bot commented Jun 23, 2026

Copy link
Copy Markdown

Overview

This PR fixes an issue where Lock, Decrease, or Unlock operations could incorrectly store future timestamps. By moving the future-timestamp guard up to _processAllowanceOperation, the validation now correctly applies to all modes. The team verified that this change is secure, introduces no performance regressions, allows the _processIncreaseOrUpdate helper to become pure, and is properly covered by the updated tests.

QA & Test Coverage

No issues found in this domain.

Web Security

Skipped: not applicable to this PR — No web-specific security concerns; this is a smart contract timestamp validation fix.

Blockchain Security

No issues found in this domain.

Performance

No issues found in this domain.

Stats

Metric Value
Reviewers 4
Successful 3
Skipped 1
Failed 0
Total findings 0
Cross-domain 0
Tokens (in / out) 94599 / 2826

Automated review by Eco's specialized review team. Architecture, business logic, and correctness remain with the human reviewer.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR closes a correctness gap in Permit3’s timestamp validation by rejecting future timestamps not only for Increase permits, but also for Lock/Decrease/Unlock flows, preventing “permanently un-unlockable” locked allowances caused by user-supplied future timestamps.

Changes:

  • Enforces a future-timestamp guard in _processAllowanceOperation so Lock/Decrease/Unlock cannot supply future timestamps.
  • Removes the redundant future-timestamp guard from _processIncreaseOrUpdate and updates it to pure.
  • Adds/updates edge tests to cover future lock timestamps and adjusts existing tests to avoid unintentionally using future timestamps (via vm.warp where appropriate).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Permit3.sol Moves the future-timestamp validation to the common allowance-operation path; simplifies _processIncreaseOrUpdate accordingly.
test/Permit3Edge.t.sol Adds a regression test for future Lock timestamps and updates related tests to use non-future timestamps or warp time when required.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Permit3.sol
revert ZeroAccount();
}

// Prevent setting timestamps in the future for all timestamp-storing modes
@MSevey MSevey requested a review from fish-sammy June 24, 2026 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants