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

Disallow conflicting method IDs #1530

Merged
merged 1 commit into from Jul 18, 2019

Conversation

@davesque
Copy link
Contributor

davesque commented Jul 17, 2019

What was wrong?

It's possible to compile a vyper contract that contains conflicting methods. That is, if a contract has two or more methods that produce the same internal routing ID (used for method selection during contract execution), it will still compile and create a contract with a security vulnerability.

How did I fix it?

Added a check for duplicate method IDs in the contract interface.

How to verify it?

Run the tests.

Description for the changelog

  • Fixed a bug that allowed contracts with conflicting methods (methods that produce the same internal routing ID) to compile without error.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@davesque davesque requested review from jacqueswww and fubuloubu Jul 17, 2019
Copy link
Collaborator

jacqueswww left a comment

LGTM

@davesque

This comment has been minimized.

Copy link
Contributor Author

davesque commented Jul 17, 2019

@jacqueswww @fubuloubu Not sure if this check is happening in the right place. But I guess we can move it to wherever it needs to go.

Copy link
Member

fubuloubu left a comment

LGTM too

So, this will throw when compiling other people's interfaces? That's not terrible because no one should have this anyways.

@davesque

This comment has been minimized.

Copy link
Contributor Author

davesque commented Jul 18, 2019

@fubuloubu Didn't realize that same code was used in that context but yes, it should throw in that case as well. Although it should probably say that the problem is with an external interface.

@jacqueswww

This comment has been minimized.

Copy link
Collaborator

jacqueswww commented Jul 18, 2019

Interfaces is fine I reckon, the other location could've been: FunctionSignature.from_definition.

@jacqueswww jacqueswww merged commit 956f8bf into ethereum:master Jul 18, 2019
3 checks passed
3 checks passed
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
@davesque

This comment has been minimized.

Copy link
Contributor Author

davesque commented Jul 18, 2019

@fubuloubu @jacqueswww Actually, this will not throw for issues with external interfaces as far as I know. For example, with this patch, this contract compiles just fine:

contract TestContract:
    def gsf() -> uint256: constant
    def tgeo() -> uint256: constant

@public
@constant
def foo() -> uint256:
    return 1
@montyly montyly referenced this pull request Jul 19, 2019
@davesque davesque deleted the davesque:fix-collisions branch Jul 24, 2019
@davesque davesque referenced this pull request Aug 26, 2019
fubuloubu added a commit that referenced this pull request Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.