ENS Contest - ABA's results

QA Report for ENS Contest

Overview

During the audit, 2 low, 6 non-critical and 3 refactoring issues were found.

Low Risk Issues

Total: 2 instances over 2 issues

Refactoring Issues

Total: 3 instances over 3 issues

Non-critical Issues

Total: 30 instances over 6 issues

Low Risk Issues (2)

[L-01] BytesUtils.equals reverts if input length is less then offset

Description

Function equals from BytesUtils reverts if input length is less then offset

    function equals(
        bytes memory self,
        uint256 offset,
        bytes memory other,
        uint256 otherOffset
    ) internal pure returns (bool) {
        return
            keccak(self, offset, self.length - offset) ==
            keccak(other, otherOffset, other.length - otherOffset);
    }

This function should specifically consider this case, one way or another.

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L129-L138

Recommendation

Modify function to check if offset > length and either:

• return false
• set offset = length

Or modify function to revert with meaningful message

[L-02] DNSRegistrar.proveAndClaimWithResolver always sets subnode record TTL to 0

Description

Function proveAndClaimWithResolver from DNSRegistrar always sets subnode record TTL to 0.

Link to code

        ens.setSubnodeRecord(rootNode, labelHash, owner, resolver, 0);

setSubnodeRecord will continue execution and arrive at _setResolverAndTTL where the TTL is set regardless of value.

Link to code

        if (ttl != records[node].ttl) {
            records[node].ttl = ttl;
            emit NewTTL(node, ttl);

Depending on how TTL will be used in the future, this severely impacts protocol.

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L114

Recommendation

Suggestions:

• provide a TTL as input/default value for claiming
• retrieve previous TTL from registry and set that.

Refactoring Issues (3)

[R-01] readTXT asset offset check is ambiguous

Description

Function readTXT from OffchainDNSResolver is a function used to read an DNS encoded string starting from an offset in a byte array. It also does a check that the next offset (or last) must be further ahead then the bounds of the current string (label).

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/OffchainDNSResolver.sol#L169

        assert(startIdx + fieldLength < lastIdx);

This check is slightly done incorrect:

• startIdx is from where to read the next label/encoded string length (1 byte)
• startIdx+1 is from where the label starts; string length of fieldLength
• startIdx+1+fieldLength is the end offset from the begining of the label at hand

As such, the logical lastIdx assessment should be assert(startIdx + 1 + fieldLength <= lastIdx);

    startIdx   startIdx+1                     endLabel=startIdx+1+fieldLength    lastIdx
       +-----------+----------------------------------------------------------------+----
       |fieldLength|       fieldLength x bytes             |                        | ...
       +-----------+---------------------------------------------------------------------

Mathematically, if we substract one from the left part, then startIdx + fieldLength < lastIdx does proves to be a valid constraint.

Keeping the original form of the check would be a more clearer solution that would not bring upon it further scrutiny.

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/OffchainDNSResolver.sol#L169

Recommendation

Change assert(startIdx + fieldLength < lastIdx) to assert(startIdx + 1 + fieldLength <= lastIdx)

[R-02] verify function from P256SHA256Algorithm can be changed to pure

Description

verify function from P256SHA256Algorithm can be changed to pure.

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/P256SHA256Algorithm.sol#L17

Recommendation

Change verify function state mutability to pure.

[R-03] Reuse RRUtils.done() instead of duplicating code

Description

In RRUtils, in the function next the first check, to determine if to exit the function

    if (iter.offset >= iter.data.length) {
        return;
    }

can be written using the done function, as it contains the same code; no need for duplication.

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L160

Recommendation

Change the check to:

    if (done(iter)) {
        return;
    }

Non-critical Issues (6)

[NC-01] Non-ABI exposed functions should be _ prepended

Description

It is generally considered a good practice to prepend _ to all internal/private functions so as to more easelly distinguish them from functions that are ABI exposed.

Instances (7)

This is a general concern seen thought the codebase, some file examples:

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/OffchainDNSResolver.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSClaimChecker.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/utils/NameEncoder.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/utils/HexUtils.sol

Recommendation

Prepend _ to all private/internal functions in the code-base.

[NC-02] Remove unused Debug event from SHA1.sol

Description

In SHA1.sol there is a debug event declared but not used

    event Debug(bytes32 x);

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/SHA1.sol#L4

Recommendation

Remove the event.

[NC-03] Incomplete, misleading or missing important key information NatSpec comments

Description

There are cases where function NatSpec comments are either severely incomplete, misleading or missing important key information.

Instances (8)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/RecordParser.sol#L9-L14

• missing the "len" param
• no mention that function returns type(uint256).max when parsing an invalid key-value pair
• document the return variables: bytes memory key, bytes memory value, uint256 nextOffset

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L235

• incorrect comment copy pasted from above function; change @dev Returns the 32 byte value ... to @dev Returns the 20 byte value ...

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L186

• add double asterixis (**) at the start of comment for it to be considered NatSpec
• almost all functions in https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol need changing

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol

• string related functions from RRUtils (nameLength, readName, labelCount, iterateRRs) expect the input offset to always start at an encoded DNS length type string although this is never mentioned

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L252

• change The serialized DS or DNSKEY record to use as proof. to The serialized DNSKEY record to use as proof. as this portraits to DNSKEY input only

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L328

• change The serialized DS or DNSKEY record to use as proof. to The serialized DS record to use as proof. as this portraits to DS input only

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L324-L325

• incomplete @dev comment @dev Attempts to verify a signed RRSET against an already known hash. This function assumes that the record

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L130-L140

• missing NatSpec return parameters

Recommendation

Add/correct the missing documentation.

[NC-04] Parameter resolver shadows already declared variable in DNSRegistrar.proveAndClaimWithResolver

Description

Parameter resolver shadows already declared variable in DNSRegistrar: proveAndClaimWithResolver

Instances (1)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L101-L123

shadows:

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L30

Recommendation

Rename function parameter to

[NC-05] Missing SPDX license identifier

Description

SPDX license identifier not provided in source file.

Instances (11)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/RSASHA1Algorithm.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/EllipticCurve.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/P256SHA256Algorithm.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/ModexpPrecompile.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/RSASHA256Algorithm.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/algorithms/RSAVerify.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/digests/SHA1Digest.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/digests/SHA256Digest.sol

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle\SHA1.sol

Recommendation

Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. See https://spdx.org for more information.

[NC-06] now param declaration shadows built-in symbol

Description

In validateSignedSet and verifyRRSet functions from DNSSECImpl the now param declaration shadows built-in symbol: now (uint): current block timestamp (alias for block.timestamp)

Instances (2)

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L109

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L143

Recommendation

Rename now co timestamp, as it is even mentioned as so in NatSpec @param now The Unix timestamp to validate the records at. and it does not imply the current block time, just an arbitrary time at which to verify the input data.