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: do not store code hash for EOAs #1644

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

obatirou
Copy link
Collaborator

Time spent on this PR:

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Resolves #

What is the new behavior?

Code hash for eoa cannot be stored as they can change. Put the logic of 0 or empty code hash in function.

Copy link
Member

@ClementWalter ClementWalter left a comment

Choose a reason for hiding this comment

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

fix empty pointer

@obatirou obatirou marked this pull request as ready for review November 27, 2024 16:38
@ClementWalter ClementWalter merged commit 930226c into kkrt-labs:main Nov 28, 2024
13 checks passed
Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

TLDR;

  1. All accounts have a codehash. If that account is an eoa it's simply keccak256("");
  2. You never want to have a disrepancy between the account's code and it's hash, thus it makes sense to write the code_hash in the same function as the account.

I think there's a small misconception between the code_hash of an account and the extcodehash in the way we handled this finding:

  • All accounts have a code hash that is keccak256(bytecode). If bytecode is empty, that code hash is keccak256("") = EMPTY_CODE_HASH
  • The finding is valid when saying that account deployed with deploy_externally_owned_account would have the wrong codehash

However I don't really agree with the way we handled this; I would rather have:

  1. Modified Starknet.deploy to set_code_hash(EMPTY_CODE_HASH) (you're deploying a contract that doesn't have any code)
  2. Replaced the write_bytecode of the account contract to write the code hash as well.

enitrat added a commit that referenced this pull request Dec 3, 2024
ClementWalter pushed a commit that referenced this pull request Dec 4, 2024
Overwrite fix to codehash done in #1644

Motivation:

 I prefer a solution that avoids having invalid states altogether.

- All accounts have a code_hash set to keccak256("") when deployed
- Any modification to the account's code will also modify it's code_hash
- extcodehash simply check the account_exists = has_code_or_nonce or
has_balance condition to know whether to return 0 or code_hash

which is more inline with the spec. The `codehash` of an account is
_never_ zero, the `extcodehash` is.
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