Skip to content
This repository has been archived by the owner on Mar 3, 2021. It is now read-only.

Fix library bytecode comparison #790

Merged
merged 3 commits into from
May 24, 2018
Merged

Fix library bytecode comparison #790

merged 3 commits into from
May 24, 2018

Conversation

yann300
Copy link
Collaborator

@yann300 yann300 commented May 9, 2018

In the context of a library, deployed bytecode contains the address of the library (pushed by the compiler to avoid calling library other than with a DELEGATECALL)

If code2 is not a library, we still suppose that the comparison remains relevant even if we remove some information from code1

@yann300 yann300 changed the title fix library comparison Fix library bytecode comparison May 9, 2018
@yann300 yann300 self-assigned this May 9, 2018
@yann300 yann300 unassigned axic May 9, 2018
@yann300 yann300 requested a review from axic May 9, 2018 09:51
@@ -187,6 +187,11 @@ module.exports = {
if (code1 === code2) return true
if (code2 === '0x') return false // abstract contract. see comment

if (code2.substr(4, 40) === '0000000000000000000000000000000000000000') {
Copy link
Member

Choose a reason for hiding this comment

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

It should also check for the push opcode to be safe.

Perhaps even check for the entire signature from here https://github.com/ethereum/solidity/blob/develop/libsolidity/codegen/ContractCompiler.cpp#L277 :

PUSH20 00..00 ADDRESS EQ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done ;)

if (code2.substr(4, 40) === '0000000000000000000000000000000000000000') {
// in the context of a library, that slot contains the address of the library (pushed by the compiler to avoid calling library other than with a DELEGATECALL)
// if code2 is not a library, well we still suppose that the comparison remain relevant even if we remove some information from `code1`
code1 = replaceLibReference(code1, 4)
Copy link
Member

Choose a reason for hiding this comment

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

I think this will replace 32 bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should not, should be 20 bytes

Copy link
Member

Choose a reason for hiding this comment

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

replaceLibReference seems to have a lot of zero padding

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

replaceLibReference inject a null address, so I think that is ok here..

@yann300 yann300 merged commit de2b8c4 into master May 24, 2018
@axic axic deleted the fixLibraryComparison branch September 19, 2018 09:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants