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

fix constructor arguments verification #1790

Merged

Conversation

ayrat555
Copy link
Contributor

fixes #1786

Motivation

Currently, we're assuming that constructor arguments are concatenated with contract code in transaction input data. But some transactions have additional data between source code and constructor arguments in transaction input

Changelog

  • fix constructor arguments verification

@ghost ghost assigned ayrat555 Apr 19, 2019
@ghost ghost added the in progress label Apr 19, 2019
@coveralls
Copy link

coveralls commented Apr 19, 2019

Pull Request Test Coverage Report for Build 7a1d07b1-813f-4aed-9cab-14ab29dae27e

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 82.938%

Totals Coverage Status
Change from base Build 13d809b7-3281-45f2-9232-db3900a65244: 0.08%
Covered Lines: 4438
Relevant Lines: 5351

💛 - Coveralls

arguments_data = String.replace(arguments_data, "0x", "")
creation_input_data = Chain.contract_creation_input_data(address_hash)

expected_arguments_data =
creation_input_data
|> String.split(bytecode)
|> String.split("0029")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not well-versed enough in smart contracts, so here's a dumb question: is there a possibility of a false positive (i.e. arguments containing 0029 fragment) and splitting the data at a wrong place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is 100% possible as I was just noticing a few contracts where this occurred. I'll try to find some examples and post here.

Copy link
Contributor

@acravenho acravenho Apr 20, 2019

Choose a reason for hiding this comment

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

Please see this contract: https://etherscan.io/address/0x64d14595152b430cf6940da15c6e39545c7c5b7e#code

Constructor:

000000000000000000000000802275979b020f0ec871c5ec1db6e412b72ff20b000000000000000000000000f5a38fbc26c720c79350b99d9c0bd42b3e9b83160000000000000000000000002929e21109901461659c0f26ad7f0e7633ea6539000000000000000000000000431f429035a1e3059d5c6a9a83208c6d3143d925

The 3rd argument 0000000000000000000000002929e21109901461659c0f26ad7f0e7633ea6539 contains 0029

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought, since we're receiving the constructor arguments from the user why not subtract them from the bytecode leaving us the correct creation code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acravenho bytecode provided by a user doesn't contain constructor arguments. or are you talking about subtracting constructor arguments from transaction input data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acravenho @goodsoft I updated my PR. Can you please try it? Now, I'm removing source code from transaction data and then I'm checking constructor arguments

Copy link
Contributor

Choose a reason for hiding this comment

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

@ayrat555 During the verification process the constructor arguments are supplied by the end user. eth_getCode which is used to obtain the bytecode should also contain this data appended at the bottom.

@ghost ghost assigned vbaranov Apr 22, 2019
@vbaranov vbaranov merged commit e93ecb5 into master Apr 22, 2019
@ghost ghost removed the in progress label Apr 22, 2019
@vbaranov vbaranov deleted the ab-fix-contructor-verification-with-contructor-arguments branch April 22, 2019 10:11
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.

Constructor Arguments Do not match
7 participants