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

incorrect-equality: do not check addresses #1713

Merged
merged 1 commit into from
May 5, 2023
Merged

Conversation

ydm
Copy link
Contributor

@ydm ydm commented Mar 6, 2023

This commit adds a simple additional check to make sure no strict equality of addresses is flagged at all.

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2023

CLA assistant check
All committers have signed the CLA.

@ydm
Copy link
Contributor Author

ydm commented Mar 10, 2023

i really don't think these errors have anything to do with my changes, can you please rerun the ci?

@elopez
Copy link
Member

elopez commented Mar 10, 2023

Hi @ydm ! Those CI failures were fixed in #1723 and #1711. Can you merge the dev branch and push to resolve the issue?

@0xalpharush 0xalpharush changed the base branch from master to dev March 10, 2023 22:52
@ydm ydm force-pushed the master branch 3 times, most recently from 798e8e3 to 9dc5167 Compare March 10, 2023 23:37
@ydm
Copy link
Contributor Author

ydm commented Mar 13, 2023

should be good now?

Comparing addresses strictly should not be flagged.
"""
addr = ElementaryType("address")
return ir.variable_left.type != addr or ir.variable_right.type != addr
Copy link
Contributor

Choose a reason for hiding this comment

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

If something like Contract(address(0x0)) == Contract(address(0x0)) is used, is_not_comparing_addresses won't filter it out. We would need to add something like isinstance(ir.variable_left.type, UserDefinedType) and isinstance(ir.variable_left.type.type, Contract). It'd be helpful to add a test case for both the address and contract case so that we can be sure of the modifications and prevent regressions in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, will do that

@PaulRBerg
Copy link
Contributor

Casting my support of this PR.

The wiki about this detector mentions a use case that should never be relevant for address checks.

@montyly
Copy link
Member

montyly commented May 5, 2023

Thanks @ydm for the contribution. We merged this PR through #1890

@ydm
Copy link
Contributor Author

ydm commented May 5, 2023

Thanks @ydm for the contribution. We merged this PR through #1890

I'm truly sorry for taking so long to fix this PR!

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.

6 participants