-
Notifications
You must be signed in to change notification settings - Fork 12.3k
Audit Fixes for RLP library on Broadcaster Audit #6106
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
Open
luiz-lvj
wants to merge
16
commits into
OpenZeppelin:master
Choose a base branch
from
luiz-lvj:audit-fixes/RLP
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+59
−14
Open
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
5800339
fix L-03
luiz-lvj 47c8048
fix N-06
luiz-lvj 78f643d
fix N-07
luiz-lvj 55b33a0
fix N-09
luiz-lvj e0a6e6b
add changeset
luiz-lvj f437257
Test encoding of addresses with leading zeros
Amxx 0ac8a92
Merge branch 'testing/rlp-encoding-addresses' into audit-fixes/RLP
Amxx 60b70b7
Update test/utils/RLP.test.js
Amxx 6a86e31
add fuzzing test
Amxx 08d217f
Merge branch 'testing/rlp-encoding-addresses' of https://github.com/A…
Amxx 5f11e08
Merge branch 'testing/rlp-encoding-addresses' into audit-fixes/RLP
Amxx 1992bed
revert back to address
luiz-lvj be65eb8
remove changeset
luiz-lvj 8f2dd16
update changeset
luiz-lvj 61b695f
Solve: Inconsistent Integer Base in Inline Assembly When Setting RLP …
luiz-lvj fe917b7
test string qith leading zeros
luiz-lvj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This encoding is not consistent with how the ethereum ecosystem does things.
If you try to use ethers.js to do encode an address that had zeros, you'll get them encoded
Note that this difference is important in the context of operations like
getCreateAddress. If the address prefix zeros are removed, you'd get an invalid address prediction.I'll add unit tests to show these issues.
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 mentioned to the auditors that it makes sense to include leading zeroes in addresses because there's no definition of an address in RLP. My thinking about this is that leading zeroes in an address are not necessarily "leading" because they constitute relevant information for the address.