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

Address cs audit 5.5 #123

Merged
merged 7 commits into from
Dec 13, 2023
Merged

Address cs audit 5.5 #123

merged 7 commits into from
Dec 13, 2023

Conversation

cwang25
Copy link
Contributor

@cwang25 cwang25 commented Dec 7, 2023

  • Can be merged after refactor, should be easy to rebase this.
  • Now treat "" as empty code, codeExists will return false if code is ""

Open question:
Mentioned in #quark-eng channel

Question on ChainSecurity [5.5] Correctness feedback. 
So it's saying that sending a eth to a empty address will cause our codeJar.codeExists() return true. Cuz having eth in it the codehash will be equal to keccak256(""). 
Was thinking codeExists should also check on the code.length > 0 if the code exsits there. "
But in testCodeJarInputVariety it looks like we treat deploying "" as a valid code. 

So was curious if we should treat all empty code (includeing "") as not exists?  (It's a bit tricky to say to check on eth balance if detect codeHash == keccak256("") cuz that's the same as deploy a "" code and send eth to it 🤔 )

@cwang25 cwang25 merged commit 6791726 into main Dec 13, 2023
4 checks passed
@cwang25 cwang25 deleted the hans/cs-audit-5.5 branch December 13, 2023 01:44
fluffywaffles pushed a commit that referenced this pull request Dec 15, 2023
* address cs audit 5.5

* now empty code is not valid

* Update test/quark-core/CodeJar.t.sol

Co-authored-by: Kevin Cheng <[email protected]>

* Update test/quark-core/CodeJar.t.sol

Co-authored-by: Kevin Cheng <[email protected]>

* address comments, remove line 43, and use saveCode()

* update gas snapshot

* update snapshot

---------

Co-authored-by: Kevin Cheng <[email protected]>
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.

2 participants