-
-
Notifications
You must be signed in to change notification settings - Fork 810
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
Check calldatasize before loading func selector #1606
Check calldatasize before loading func selector #1606
Conversation
8839113
to
f9155a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding my own approval, and referencing OP's approval from #1603 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. Left a couple of suggestions for adding comments.
STORE_CALLDATA: List[Any] = ['seq', ['mstore', 28, ['calldataload', 0]]] | ||
STORE_CALLDATA: List[Any] = \ | ||
['seq', | ||
['if', ['lt', 'calldatasize', 4], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to add a note about why this is here. To someone reading the code who's not familiar with the discussion, it might look mysterious.
@@ -57,13 +61,6 @@ | |||
) | |||
|
|||
|
|||
# Header code | |||
INITIALIZER_LIST = ['seq', ['mstore', 28, ['calldataload', 0]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include the deletion of this constant in a separate commit with a message that explains why it was removed? I had to do a bit of poking around to figure out why this was taken out. It looks like it just wasn't used anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i discovered it was dead code. i can split it into a separate commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I remember why I put it in the same commit, it's because it had duplicated code with STORE_CALLDATA and I didn't want the commit to change one of the calldataload
without changing the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note to the commit message to clarify.
- delete INITIALIZER_LIST since it was unused.
f9155a5
to
8531af2
Compare
What I did
Fix #1603
How I did it
Implemented the solution from #1603 (comment)
How to verify it
Check tests, also look at IR of following code:
Description for the changelog
Fix edge case in function selector with 0 calldata
Cute Animal Picture