Hashgraph Name Protocol Audit Report

This is the function used to extend the expiration date of a sub-domain. The function now can be called by anyone. Ideally, the function can only be called by the top-domain owner.

The attacker can call the function arbitrarily to make the sub-level domain chaos.

The function should be protected by the onlyTLDOwner modifier.

Resolved in commit 7cd9d48906129942436d9c05e939d89bd59dc0df.

TLD node might add the same SLD hash record more than one time.

When the top-level domain (TLD) node adds a new second-level domain (SLD) hash record, it first checks if the SLD node is full or not. If it’s full, a new SLD node contract will be created. In this case, when an SLD hash record is being added to the newly created SLD node contract, we have to first check if the SLD hash record to be added has already been there in the old smart contract before adding it to the new one. Since the new SLD node contract does not have SLD hash records of the previous SLD nodes, the same SLD hash record may be added to the newly created SLD node again.

The same issue may also occur when updating the SLD nodes (i.e., registering the expired SLD) for the reason that there might be multiple SLD records with the same SLD hash.

1. The SLD hash "Alice" exists on the current SLD node;

2. The current SLD node is already full;

3. Alice is able to add the same SLD hash "Alice" to the newly created SLD node;

4. Now, TLD node exists two same "Alice" hash and each can have different settings and subdomains;

5. The process can be repeated.

Please change the code and check if the SLD hash exists first before adding a new record to the existing SLD node or creating any new SLD node.

Acknowledged.

Reply from PieFi team:

“This is a known issue. Hedera is different than other chains in that there are restrictions that limit scalability. The name service is designed to scale with large numbers of SLDNodes, so it is not possible for the TLDNode to search all SLDNodes within the gas limit. As this is a known issue, calls to the TLDNode::registerSLD() function will be restricted to only our backend. Our backend will check for SLD existence before registering a new SLD.”

SubdomainNode.addSubdomain() has a few logic flaws. Some parameters are not used when the subdomain hash already exists and the information of the subdomain cannot be updated if the length of the subdomain hash array reaches the max record limit.

The function SubdomainNode.addSubdomain() code is as follows:

function addSubdomain(bytes32 _subdomainHash, string memory _name, address _owner) external onlySLDNode returns(int32) {
  require(subdomainHashes.length < maxRecords, "This contract is full, no more records can be added.");
  if(records[_subdomainHash].owner == address(0)) {
      subdomainHashes.push(_subdomainHash);
      names.push(_name);
      records[_subdomainHash].name = _name;
  }
  records[_subdomainHash].owner = _owner;
  return HederaResponseCodes.SUCCESS;
}

1. If the _subdomainHash exists in the records mapping, the parameter _name is never used. The subdomain’s name cannot be updated after being created.

2. The function will revert when the subdomain hash length reaches the max record limit. The information like owner of the existing subdomain cannot be updated.

1. The subdomain hash length reaches the max record limit.

2. Alice cannot update the subdomain’s owner because the function reverts.

1. Please consider checking if the length of subdomainHashes array exceeds the max record limit or not only when adding a new subdomain.

2. Please double-check and confirm that the name does not need to be updated.

The 1st issue is that the _name argument is unused in a specific case.

Reply from PieFi team:

“This is intentional. The subdomain contract was designed to be as lean as possible. The _name parameter here is the text form of the _subdomainHash. If the subdomain already exists, then there is no reason to update the name.”

The 2nd issue is that there were flaws in the logic of the function. Those flaws have been resolved in commit b3a22e085eac735371064a14e95e972ba721d078.

SLDNode.addSubdomain() will revert if the corresponding subdomain node reaches the max record limit.

According to the contract code, every second-level domain will only have one subdomain node to maintain the subdomain record. No more subdomain can be added to the second level domain if the amount of subdomains maintained by the subdomain node reaches the max record limit.

N/A

Create a new subdomain node for a second-level domain when a single subdomain node reaches the max record limit.

Acknowledged.

Reply from PieFi team:

“This is intentional. In our testing, we found that the Subdomain.sol contract can store more than 2000 subdomains before the contract runs out of space. We find this is an acceptable limitation.”

SLD hash existence is not checked when extending the expiry date.

SLDNode.extendExpiry() does not check if the SLD hash record exists or not. SLD hash record that does not exist will have an expiry date after calling this function.

N/A

Check if the SLD hash record exists before extending the expiry date.

Resolved in commit b3a22e085eac735371064a14e95e972ba721d078.

The function selfdestruct is used in multiple contracts. Consider removing the self-destruct functionality unless it is absolutely required

Using selfdestruct makes the contract a suspicious metamorphic contract and places users under contract re-init risks. Consider removing the self-destruct functionality unless it is absolutely required. If there is a valid use case, it is recommended to implement a multi-sig scheme so that multiple parties must approve the self-destruct action.

The contract is self-destructed and re-initialized as a new contract. In this case, users lose all the data and values stored on the original contract.

Implement a multi-sig scheme so that multiple parties must approve the self-destruct action.

Resolved in commits b3a22e085eac735371064a14e95e972ba721d078 and 743971fae48b905e18435f6370890a584387efea. The self-destruct function is removed and the major contracts are now pausable.

PieFi team implemented this feature so that in the event of an emergency they will be able to pause the contracts and prevent any state changes. Meanwhile, read operations will still be possible.

There is no function that can be used to update the variable SubDomainNode.maxRecords.

N/A

Mark the variable as immutable or add a function to update the variable’s value.

Acknowledged.

Reply from PieFi team:

“This is intentional. In our testing, we found that the Subdomain.sol contract can store more than 2000 subdomains before the contract runs out of space. We find this is an acceptable limitation.”

Many of the HNS contract functions related to critical operations do not emit necessary events. Events are vital aids in monitoring contracts and detecting suspicious behaviors. Operations that perform critical operations should trigger events so that users and off-chain monitoring tools can account for important state changes.

N/A

Please consider emitting events in functions that perform critical operations.

Resolved in commit b3a22e085eac735371064a14e95e972ba721d078.

In this commit,SetSLDInfo, ExtendExpiry, UpdateSLD, AddSubdomain ,SetSubdomainInfo, AddSubdomain, TokenTransfer seven events are added.

There is no view function to fetch the length of array SLDNode.serials, SubdomainNode.subdomainHashes, SubdomainNode.name.

N/A

Add a view function to fetch the length of arrays.

Acknowledged.

Reply from PieFi team:

“We don’t have a use case to be able to query for the array lengths.”

A view function can be added to fetch array items by index.

N/A

Add view functions to get array items by index.

Acknowledged.

Reply from PieFi team:

“We don’t have a use case to fetch individual items in the arrays. They are mostly used for tracking purposes. We do, however, want to be able to fetch the entire array in the event we want to view all data in the Node.”

SLD nodes contract still exists in array in TLD node contract after self-destructed. A view function to view all the nodes that are self-destructed can be added.

N/A

A view function to view all the nodes that are self-destructed can be added.

Resolved in commit b3a22e085eac735371064a14e95e972ba721d078.

The view functions SLDNode.getMaxRecords() and SLDNode.getNumRecords() will revert if called by non-TLD owner. Access control on public view functions is unnecessary since everything on the blockchain is public and can be accessed by everyone.

N/A

Suggest removing the access control if it is unnecessary.

Resolved in commit b3a22e085eac735371064a14e95e972ba721d078.