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 zero address check in lookupAddress() #4086

Closed
wants to merge 1 commit into from

Conversation

mikeshultz
Copy link

Noticed earlier that we were sending name(bytes32) calls to the zero address. I think it's due to this invalid comparison preventing this from returning early and saving a bunch of unnecessary RPC calls.

@ricmoo
Copy link
Member

ricmoo commented Jun 1, 2023

Oh yes! You are correct!

This is due to previously using call directly, which did not decode the bytes32 result into an address. Now that we are using proper contracts, this needs to be changed.

Thanks! :)

@ricmoo ricmoo added bug Verified to be an issue. on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6 labels Jun 1, 2023
ricmoo added a commit that referenced this pull request Jun 1, 2023
@ricmoo
Copy link
Member

ricmoo commented Jun 2, 2023

Merged in v6.4.1.

I'll be sure to get you added into the contributors.md for your GitPoap too.

Thanks!

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Jun 2, 2023
@ricmoo ricmoo closed this Jun 2, 2023
@mikeshultz
Copy link
Author

Great, glad I could help and thanks for getting that change into a release!

For future reference, was there a problem with the PR that prevented it from being merged?

@ricmoo
Copy link
Member

ricmoo commented Jun 2, 2023

Oh, the PR was perfect. I just have crippling paranoia with just accepting a merge.

I highly recommend checking out this repo to really amp up your fear of how even simple documentation changes and such can use BIDI and joiner UTF-8 characters and such to hide malicious code in seemingly safe code or pull requests.

I am working on some CI workflows though, that will help make merging safe in the future.

@ricmoo
Copy link
Member

ricmoo commented Jun 2, 2023

(and with libraries like Ethers and my AES libraries, I have a whole new level of security I prefer to enforce ;))

@mikeshultz
Copy link
Author

Thanks for pointing out that repo. The paper was a good read. Something else to look out for, I guess...

Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this pull request Jan 14, 2024
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this pull request Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants