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

Process all basic blocks to make the sighash checks 'greedy' #267

Open
wants to merge 76 commits into
base: develop
Choose a base branch
from

Conversation

andrewmccall
Copy link

@andrewmccall andrewmccall commented Aug 4, 2021

Instead of processing just basic_block[0] I've looped through all the basic blocks this finds the sighashes for many more ERC20 contracts.

I think this also removed the limitation around proxies. I'm still building my knowledge around ETH so it may do so at the expense of introducing false positives.

@andrewmccall
Copy link
Author

Sorry, I missed the actual fix I wanted to implement in my commit. This now contains a loop through the basic blocks adding all the hashes.

This will may add more sighashes than there really are (I'm not sure everything that's a PUSH4 is actually a function) but it doesn't seem to miss any which works for the matching on contracts if the function comes through a proxy, which lots of contracts seem to implement.

@medvedev1088
Copy link
Member

Thanks for the PR!

Looking only at the 1st block could produce false negatives (some function sighashes will be missing), while looking at all blocks could product false positives (some function sighashes will be added mistakenly). The PR needs some test cases and comparison of results before and after to understand the impact

I kindly request to add test cases that could surface false positives.

@yongchand
Copy link
Contributor

Any updates relating this issue/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.

None yet

8 participants