Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✅ Fix test to fail from old deadline not arithmetic underflow #340

Merged

Conversation

heyskylark
Copy link
Contributor

Description

In ERC20.t.sol it appears the test testFailPermitPastDeadline is failing due to an underflow error from subtracting block.timestamp of 0 by 1. The test fails successfully, but not from the expired deadline of the permit.

To fix the issue, I set the timestamp in the signature and the permit input to the original block.timestamp of 0. Then I used hevm.warp to increment the current block.timestamp by 1.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

@heyskylark heyskylark changed the title Fix permetPastDeadline test to fail from old deadline not arithmetic underflow Fix permitPastDeadline test to fail from old deadline not arithmetic underflow Nov 4, 2022
@transmissions11
Copy link
Owner

ah nice great find.

@transmissions11 transmissions11 changed the title Fix permitPastDeadline test to fail from old deadline not arithmetic underflow ✅ Fix permitPastDeadline test to fail from old deadline not arithmetic underflow Nov 4, 2022
@transmissions11 transmissions11 changed the title ✅ Fix permitPastDeadline test to fail from old deadline not arithmetic underflow ✅ Fix test to fail from old deadline not arithmetic underflow Nov 4, 2022
@transmissions11
Copy link
Owner

can you run forge snapshot?

@transmissions11 transmissions11 merged commit 8d910d8 into transmissions11:main Nov 4, 2022
@heyskylark heyskylark deleted the skylark/fix-erc20-permit-test branch November 4, 2022 21:50
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