QA Report - Githubissues

Typos

     *        data, followed by a series of canonicalised RR records that the signature

Change canonicalised to canonicalized

The same typo (Authorises/ Authorizes, Authorised/authorised, or Unauthorized/unauthorised) occurs in all 20 lines referenced below.

Example: ReverseRegistrar.sol: L40:

    modifier authorised(address addr) {

IBaseRegistrar.sol: L20

ReverseRegistrar.sol: L40

ReverseRegistrar.sol: L46

ReverseRegistrar.sol: L80

ReverseRegistrar.sol: L126

NameWrapper.sol: L15

NameWrapper.sol: L152

NameWrapper.sol: L202

NameWrapper.sol: L224

NameWrapper.sol: L241

NameWrapper.sol: L266

NameWrapper.sol: L289

NameWrapper.sol: L315

NameWrapper.sol: L329

NameWrapper.sol: L350

NameWrapper.sol: L381

NameWrapper.sol: L421

NameWrapper.sol: L469

NameWrapper.sol: L475

NameWrapper.sol: L804

Change authorised to authorized. Similarly for other forms of the word. This assumes that a decision has been made to use standard American English throughout ENS.

ETHRegistrarController.sol: L257

            // check first few bytes are namehash

Change check first to check that first

ReverseRegistrar.sol: L156

     * @dev An optimised function to compute the sha3 of the lower-case

Change optimised to optimized

ERC1155Fuse.sol: L10

/* This contract is a variation on ERC1155 with the additions of _setData, getData and _canTransfer and ownerOf. _setData and getData allows the use of the other 96 bits next to the address of the owner for extra data. We use this to store 'fuses' that control permissions that can be burnt. 32 bits are used for the fuses themselves and 64 bits are used for the expiry of the name. When a name has expired, its fuses will be be set back to 0 */

Remove duplicate word be

NameWrapper.sol: L534

     * @param ttl ttl in the regsitry

Change regsitry to registry

Long single line comment

For the sake of readability, comments over 79 characters, such as the one below, should wrap using multi-line comment syntax.

ERC1155Fuse.sol: L10

/* This contract is a variation on ERC1155 with the additions of _setData, getData and _canTransfer and ownerOf. _setData and getData allows the use of the other 96 bits next to the address of the owner for extra data. We use this to store 'fuses' that control permissions that can be burnt. 32 bits are used for the fuses themselves and 64 bits are used for the expiry of the name. When a name has expired, its fuses will be be set back to 0 */

Comments concerning unfinished work or open items

Comments such as the one below that contain TODOs or similar language should be addressed and modified or removed

DNSSECImpl.sol: L238

        // TODO: Check key isn't expired, unless updating key itself

Named `return` variable not used when a function returns

The existence of a named return variable that is never used (here, newFuses) is potentially perplexing

INameWrapper.sol: L80-82

    function setFuses(bytes32 node, uint32 fuses)
        external
        returns (uint32 newFuses);

Missing `@param` statements

DNSSECImpl.sol: L179-183

     * @param name The name of the RRSIG record, in DNS label-sequence format.
     * @param data The original data to verify.
     * @param proof A DS or DNSKEY record that's already verified by the oracle.
     */
    function verifySignature(bytes memory name, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, bytes memory proof) internal view {

@param statement is missing for rrset

DNSSECImpl.sol: L228-238

     * @param dnskey The dns key record to verify the signature with.
     * @param rrset The signed RRSET being verified.
     * @param data The original data `rrset` was decoded from.
     * @return True iff the key verifies the signature.
     */
    function verifySignatureWithKey(RRUtils.DNSKEY memory dnskey, bytes memory keyrdata, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data)
        internal
        view
        returns (bool)
    {
        // TODO: Check key isn't expired, unless updating key itself

@param statement is missing for keyrdata. I've included the "TODO" comment here (which I mentioned earlier) which refers to the key

ReverseRegistrar.sol: L72-79

     * @param addr The reverse record to set
     * @param owner The address to set as the owner of the reverse record in ENS.
     * @return The ENS node hash of the reverse record.
     */
    function claimForAddr(
        address addr,
        address owner,
        address resolver

@param missing for resolver. The resolver @param is also missing in the two examples below:

ReverseRegistrar.sol: L127-136

     * @param addr The reverse record to set
     * @param owner The owner of the reverse node
     * @param name The name to set for this address.
     * @return The ENS node hash of the reverse record.
     */
    function setNameForAddr(
        address addr,
        address owner,
        address resolver,
        string memory name

NameWrapper.sol: L397-404

     * @param label Label as a string of the .eth name to upgrade
     * @param wrappedOwner The owner of the wrapped name
     */

    function upgradeETH2LD(
        string calldata label,
        address wrappedOwner,
        address resolver

NameWrapper.sol: L267-274

     * @param tokenId The hash of the label to register (eg, `keccak256('foo')`, for 'foo.eth').
     * @param duration The number of seconds to renew the name for.
     * @return expires The expiry date of the name on the .eth registrar, in seconds since the Unix epoch.
     */
    function renew(
        uint256 tokenId,
        uint256 duration,
        uint64 expiry

@param missing for expiry

Missing `require` revert string

A require message should be included to enable users to understand reason for failure Recommendation: Add a brief (<= 32 char) explanatory message to each of the lines below

BytesUtils.sol: L12

        require(offset + len <= self.length);

BytesUtils.sol: L146

        require(idx + 2 <= self.length);

BytesUtils.sol: L159

        require(idx + 4 <= self.length);

BytesUtils.sol: L172

        require(idx + 32 <= self.length);

BytesUtils.sol: L185

        require(idx + 20 <= self.length);

BytesUtils.sol: L199

        require(len <= 32);

BytesUtils.sol: L200

        require(idx + len <= self.length);

BytesUtils.sol: L235

        require(offset + len <= self.length);

BytesUtils.sol: L262

        require(len <= 52);

BytesUtils.sol: L268

            require(char >= 0x30 && char <= 0x7A);

BytesUtils.sol: L270

            require(decoded <= 0x20);

Owned.sol: L10

        require(msg.sender == owner);

ETHRegistrarController.sol: L57

        require(_maxCommitmentAge > _minCommitmentAge);

ETHRegistrarController.sol: L121

        require(commitments[commitment] + maxCommitmentAge < block.timestamp);

ETHRegistrarController.sol: L246

        require(duration >= MIN_REGISTRATION_DURATION);

BytesUtil.sol: L13

        require(offset + len <= self.length);

code-423n4 / 2022-07-ens-findings

QA Report #108

Typos

Long single line comment

Comments concerning unfinished work or open items

Named `return` variable not used when a function returns

Missing `@param` statements

Missing `require` revert string

code-423n4 / 2022-07-ens-findings

QA Report #108

Typos

Long single line comment

Comments concerning unfinished work or open items

Named return variable not used when a function returns

Missing @param statements

Missing require revert string

Named `return` variable not used when a function returns

Missing `@param` statements

Missing `require` revert string