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

QA Report #137

Open
code423n4 opened this issue Jul 19, 2022 · 0 comments
Open

QA Report #137

code423n4 opened this issue Jul 19, 2022 · 0 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

[L01] 2 Step Ownership changes

Critical changes such as ownership changes should be a 2 step process to protect against human error. While the errors are unlikely important parts of the contract would become unusable if they occured.

Consider changing the following functions to 2 step procedures:
Owned.sol#L18-L20

[L02] Events for Critical State changes

When modifying critical state variables such as changing owner, I recommend emiting an event to allow monitoring with off chain tools.

Consider adding an event to the setOwner function:
Owned.sol#L19

[L03] Floating Pragma Versions

For non library/interface contracts I recommend not using a floating pragma version and deploying with the version that was used for testing.

Also the following is missing a pragma version:
IBaseRegistrar.sol#L1

[N01] Named Returns and Return Statements

The following functions are using both named returns and return statements. I recommend only using one of them per function.
BytesUtils.sol#L135-L136
DNSSECImpl.sol#L110-L139
RRUtils.sol#L38-L40
NameWrapper.sol#L744-L752
NameWrapper.sol#L832-L843
NameWrapper.sol#L853-L864

[N02] Open Todos

Recommend resolving and removing the following TODO before deployment.
DNSSECImpl.sol#L238

[N03] Public Functions can be External

The following functions are never called within their given contract can be changed from public to external:
DNSSECImpl.sol#L58
DNSSECImpl.sol#L69
Owned.sol#L18
NameWrapper.sol#L105
NameWrapper.sol#L128
NameWrapper.sol#L456-L461
Controllable.sol#L11

[N04] Incomplete Natspec

Natspec is missing in the following locations:
BytesUtils.sol#L232 - missing @return
DNSSECImpl.sol#L108 - missing @return
DNSSECImpl.sol#L145 - missing @return
DNSSECImpl.sol#L180 - missing @param rrset
DNSSECImpl.sol#L229 - missing @param keyrdata
ReverseRegistrar.sol#L73 - missing @param resolver
ReverseRegistrar.sol#L129 - missing @param resolver
NameWrapper.sol#L114 - missing @param tokenId
NameWrapper.sol#L207 - missing @return
NameWrapper.sol#L268 - missing @param expiry
NameWrapper.sol#L371 - missing @return
NameWrapper.sol#L398 - missing @param resolver

[N05] Missing SPDX-License-Identifier

SPDX-License-Identifier is missing in the following files:
BytesUtils.sol
RRUtils.sol
SHA1.sol
Owned.sol
Algorithm.sol
Digest.sol
ETHRegistrarController.sol
IETHRegistrarController.sol
StringUtils.sol
IBaseRegistrar.sol
ReverseRegistrar.sol
IReverseRegistrar.sol
IMetadataService.sol
ENS.sol

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 19, 2022
code423n4 added a commit that referenced this issue Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

1 participant