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
Merged

Conversation

davesque
Copy link
Contributor

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 July 17, 2019 21:33
Copy link
Contributor

@jacqueswww jacqueswww left a comment

Choose a reason for hiding this comment

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

LGTM

@davesque
Copy link
Contributor Author

@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 fubuloubu left a comment

Choose a reason for hiding this comment

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

LGTM too

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

@davesque
Copy link
Contributor Author

@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
Copy link
Contributor

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

@jacqueswww jacqueswww merged commit 956f8bf into vyperlang:master Jul 18, 2019
@davesque
Copy link
Contributor Author

@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

@davesque davesque deleted the fix-collisions branch July 24, 2019 18:08
@davesque davesque mentioned 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants