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: resolve false positive constructor arguments #1875

Merged
merged 3 commits into from
May 8, 2019

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented May 1, 2019

Resolves #1873

Changelog

Bug Fixes

  • We only want to take everything up to the swarm hash.
  • I added an entry to CHANGELOG.md with this PR
  • If I added new functionality, I added tests covering it.
  • If I fixed a bug, I added a regression test to prevent the bug from silently reappearing again.
  • I checked whether I should update the docs and did so if necessary

@ghost ghost assigned zachdaniel May 1, 2019
@ghost ghost added the in progress label May 1, 2019
@zachdaniel zachdaniel force-pushed the split-constructor-arguments-correctly branch from d2c1ef4 to 5c85efa Compare May 1, 2019 14:36
@goodsoft
Copy link
Contributor

goodsoft commented May 1, 2019

Are you really sure there can't be any false positives after the 0029 we need?
If the constructor arguments are there, are you sure they can't contain 0029 themselves?
Or, even, x0 02 9x, which would match as well?

@coveralls
Copy link

coveralls commented May 1, 2019

Pull Request Test Coverage Report for Build 25fc482d-5f11-4b03-bb82-c87979d3fcfb

  • 11 of 13 (84.62%) changed or added relevant lines in 3 files are covered.
  • 7 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.04%) to 81.947%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/explorer/lib/explorer/smart_contract/verifier.ex 5 6 83.33%
apps/explorer/lib/explorer/smart_contract/verifier/constructor_arguments.ex 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
apps/explorer/lib/explorer/smart_contract/solidity/code_compiler.ex 1 95.83%
apps/indexer/lib/indexer/fetcher/token.ex 1 78.57%
apps/indexer/lib/indexer/fetcher/token_balance.ex 2 87.1%
apps/indexer/lib/indexer/block/fetcher.ex 3 86.57%
Totals Coverage Status
Change from base Build ef7c3a4c-fe72-4c65-a1d9-4d1f537e9755: -0.04%
Covered Lines: 4630
Relevant Lines: 5650

💛 - Coveralls

@acravenho
Copy link
Contributor

@zachdaniel @goodsoft Here is an example of a false positive where we would run into an issue: https://etherscan.io/address/0x64d14595152b430cf6940da15c6e39545c7c5b7e#code

Please see the constructor arguments and the 0029.

@zachdaniel
Copy link
Contributor Author

@goodsoft definitely not really sure. I only just started looking into it this morning, and had a similar thought myself, but I figured I would get the bare bones solution for that specific contract up in front of people.

@zachdaniel
Copy link
Contributor Author

Okay, so the swarm source starts with a known value, I think. So perhaps what we do is skip to the swarm source and split there?

@zachdaniel
Copy link
Contributor Author

https://solidity.readthedocs.io/en/v0.4.21/metadata.html#encoding-of-the-metadata-hash-in-the-bytecode

@acravenho
Copy link
Contributor

Yes, that's correct. Are you able to identify the swarm source without the 0029?

@acravenho
Copy link
Contributor

Nice! That should work.

@zachdaniel
Copy link
Contributor Author

We could use the length + bzzr0 bytes? Although I think its implying that the mapping itself might change at anytime.

@zachdaniel
Copy link
Contributor Author

I think it looks reliable enough for now. If they "support other ways to retrieve contract metadata in the future", this can be updated.

@acravenho
Copy link
Contributor

{a165627a7a72305820} - {32 bytes} - {0029} - {constructor arguments}

@acravenho
Copy link
Contributor

acravenho commented May 1, 2019

Comparing now against these addresses:

  • 0xada931ac30aff9e51148d6aaf565fc89e7e90bc0 ✅
  • 0x8f639224f992e466ecdffc3f5880194a73f1d93b ✅
  • 0x1155c5605d08585594a428f473c77eeb33f28bc1 ✅
  • 0x71320e08c267746616456aeca4efde73ed98caf3 ❌
  • 0x99831db28f7bb7c03a1a6c517a54ac3e80da2be1 ✅
  • 0x7c6ef33ab884862dadf412f7271d669fd3cb8875 ✅
  • 0x5152345f9722bc1bef5fd4ec0428946d5eb18c82 ✅
  • 0xe241db15493adfe3c49884255f4edf012389bcc8 ✅
  • 0x61fe6d3692abdb010347d89d7ec391b719115519 ✅
  • 0xbf67ebc297685863ae7e0e6f88eaffe8052a0d3b ✅
  • 0x74dd3ee13dc6714de5670198f6e5cd586690b79a ❌
  • 0x348c19f7de78acf3c89c7a5fd67b3f4b515bf69a ✅
  • 0x2020fe35fc3a2ad06073b5083917b284f927f7b3 ✅

@acravenho
Copy link
Contributor

The two contracts that failed above don't seem to have a swarm source hash. Looks like they were created prior to when swarm source was created.

@zachdaniel
Copy link
Contributor Author

Do they also have constructor arguments? If so, then we'll need another heuristic for those. I'm almost done with the code to use the swarm hash.

@acravenho
Copy link
Contributor

No constructor arguments. They fail when trying to verify them but probably for some other reason. Looks like an edge case.

@zachdaniel
Copy link
Contributor Author

Well for now, if we don't find a swarm hash I'll have it use the whole thing.

@zachdaniel zachdaniel force-pushed the split-constructor-arguments-correctly branch from 5c85efa to a2ebe94 Compare May 1, 2019 17:41
@@ -181,13 +181,29 @@ defmodule Explorer.SmartContract.VerifierTest do
swarm_source = "3c381c1b48b38d050c54d7ef296ecd411040e19420dfec94772b9c49ae106a0b"

bytecode =
"0x608060405234801561001057600080fd5b5060df8061001f6000396000f3006080604052600436106049576000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff16806360fe47b114604e5780636d4ce63c146078575b600080fd5b348015605957600080fd5b5060766004803603810190808035906020019092919050505060a0565b005b348015608357600080fd5b50608a60aa565b6040518082815260200191505060405180910390f35b8060008190555050565b600080549050905600a165627a7a72305820"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was asserting that part of the swarm metadata hash would come back, but that would be incorrect.

@zachdaniel
Copy link
Contributor Author

Alright @vbaranov @acravenho @goodsoft this should be ready to go for real now.

Copy link
Contributor

@acravenho acravenho left a comment

Choose a reason for hiding this comment

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

LGTM 👍

<<>> ->
String.reverse(extracted)

"a165627a7a72305820" <> <<_::binary-size(64)>> <> "0029" <> _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.

what is a165627a7a72305820?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s what the swarm hash starts with.

@zachdaniel zachdaniel force-pushed the split-constructor-arguments-correctly branch 3 times, most recently from 7437644 to 54a3c99 Compare May 6, 2019 15:40
@ghost ghost assigned vbaranov May 7, 2019
@zachdaniel zachdaniel force-pushed the split-constructor-arguments-correctly branch from 06f5fe7 to 91c0011 Compare May 7, 2019 16:43
@zachdaniel zachdaniel requested review from goodsoft and ayrat555 and removed request for goodsoft May 7, 2019 16:44
@zachdaniel zachdaniel force-pushed the split-constructor-arguments-correctly branch 4 times, most recently from 41022d1 to 09446f6 Compare May 7, 2019 19:27
@zachdaniel zachdaniel force-pushed the split-constructor-arguments-correctly branch from 09446f6 to c3a5218 Compare May 7, 2019 20:22
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.

There was an error compiling your contract.
6 participants