Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

Complete Audit - Piper #97

Closed
cfl0ws opened this issue Mar 29, 2017 · 8 comments
Closed

Complete Audit - Piper #97

cfl0ws opened this issue Mar 29, 2017 · 8 comments

Comments

@cfl0ws
Copy link

cfl0ws commented Mar 29, 2017

Issue to track Piper's audit progress.

@cfl0ws cfl0ws created this issue from a note in ENS Relaunch (Pending - Audit) Mar 29, 2017
@cfl0ws cfl0ws added the pm label Mar 29, 2017
@cfl0ws
Copy link
Author

cfl0ws commented Mar 29, 2017

From @pipermerriam -

Quick update on the status of the audit.

I'm currently at 7 hours billable.

I've still got a few section of code to go through but here is what I have thus far. Standard caveats apply that this is an early draft. I suspect I should be able to wrap this up by end of day Friday.

https://gist.github.com/pipermerriam/dfa9c541aef80690c29d353bc9301291

@pipermerriam
Copy link

Here is a draft of the final report.

https://gist.github.com/pipermerriam/6bec14a2d8d8abb904529849c6b03131

@cfl0ws
Copy link
Author

cfl0ws commented Apr 2, 2017

@cfl0ws
Copy link
Author

cfl0ws commented Apr 2, 2017

@alexvandesande and @Arachnid I'd expect to see a 1:1 correspondence between the issues listed in sections 4 & 5. Should I ask @pipermerriam to clarify or do you understand why there's not a 1:1 mapping?

4.2 - Minor Issues

4.3 - Medium Issues

4.3.1 - Deed Factory
4.3.2 - Implement Registrar.trySetSubnodeOwner function.

4.4 - Major Issues

4.5 - Critical Issues

5 - Detailed Findings

5.1 - Minor Issues

5.1.1 - Registrar.returnDeed function will always throw.

5.2 - Medium Issues

5.2.1 - Registrar contract uses the entry.highestBid and entry.deed variables to derive secondary information about the state of the entry.
5.2.2 - Deed.destroyDeed contains a multi-line if statement without braces.
5.2.3 - Registrar.finalizeAuction uses multi-line if statement without braces.
5.2.4 - Registrar.transfer naively calls ens.setSubnodeOwner

@cfl0ws
Copy link
Author

cfl0ws commented Apr 2, 2017

@alexvandesande I saw your email comment about getting to work on these Monday. Does it make sense to prioritize them, based on whether or not they need to be addressed prior to launch? Or, based on your review, do they all need to be addressed prior to launch?

@pipermerriam
Copy link

@chris-remus

section 4 are higher level things that don't necessarily correspond to a specific part of the code, or, that apply to many parts of the code.

Section 5 is a fine grained list of items that correspond to a specific section of code.

Some things may be present in both lists but neither is a super-set of the other.

@Arachnid
Copy link
Member

Arachnid commented Apr 3, 2017

@pipermerriam Is the 'final draft' the released report, or do you have more edits you want to make?

@pipermerriam
Copy link

Final signed version here.

https://gist.github.com/pipermerriam/cd7a9a3369ae6d163f615117be6e071d

Content is the same as the previously linked gist. Sorry for the delays.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

3 participants