ENS-Audit.md

Summary

This is the report from a security.audit performed on ENS by Martin Holst Swende. The audit has focused primarily on ensuring the integrity of the auction model, the security of users and funds, and the reliability of ENS as a whole.

Background and scope

Vulnerabilities have been classified according to the following scale:

• Critical
• High
• Medium
• Low
• Info

The rating Info means that the finding does not have an impact on security, but has been included for informational purposes, as it may be relevant for reasons such as best-practice or gas consuption.

In scope

Included are the contracts (solidity-files and lll-files) in the ENS-repo at commit hash 55ea24a5f22a4db09eb4268fefa9337fb8ca8127.

Excluded

Other files, such as html/javascript were not included in the audit.

Findings

Low: Missing user error protection

Users can perform actions mistakenly which lead to loss of funds.

• It is possible to register an _owner with 0x0 address
  • Since an explicit owner is used instead of msg.sender.
• It is possible to transfer to 0x0.

Recommendation

If unsealBid reveals that _owner is 0x0, set owner to msg.sender:

function unsealBid(bytes32 _hash, address _owner, uint _value, bytes32 _salt) {
    bytes32 seal = shaBid(_hash, _owner, _value, _salt);
    if ( _owner == 0) _owner = msg.sender 
    [...]

Prevent transfer to 0x0:

function transfer(bytes32 _hash, address newOwner) onlyOwner(_hash) {
    if ( newOwner == 0x0) throw;
    [...]

Low: User inactivity causes loss of funds

The method cancelBid can be called by anyone to cancel a bid which has not been revealed during the revealPeriod (last 24 hours of the initialAuctionPeriod).

The method will send 0.5% of the money to the canceller, and burn the remaining payment.

If someone takes part in an auction, but sees that someone else reveals a higher bid, he/she needs to be aware that performing a reveal is required in order to recover the invested ether - as otherwise there is a large risk for the deposited ether to be destroyed.

Recommendation

Users need to be made aware of the 24-hour window to in which they are required to perform unsealBid. Needs to be very clear about the need to take action to regain the ether.

Low: Loss of funds due to forgotten salt

If a user makes a bid for a name, but forgets or displaces the salt parameter, it may prove impossible to call unsealBid on the bid. In that case, all ether deposited will basically be forfeit.

Normally, an Ethereum user would expect the private key/wallet to be sufficient to authorize actions, making this is a bit of an exceptional case.

Recommendation

Users need to be made aware of this additional requirement, and be prompted to backup the salt, since it is required in order to claim the bid at a later date or from a different computer.

Alternatively, the salt could be generated from the private key, by using e.g. the signature from a signed message (e.g. "ENS <domain>").

Edit: Not all clients generated deterministic signatures, so that mechanism may prove unreliable.

Low: Old registry does not become inactive

If the registry becomes outdated, and the functionality is taken over by a new registry, the old one becomes partially locked.

• startAuction will throw, due to the modifier registryOpen
• Open auctions will still accept newBid
• Open auctions will still allow unsealBid
• Open auctions will be able to finalizeAuction
• transferRegistrars will still work

This means that while a new registrar is opened, there is a 4-week period in which old auctions may still be 'live'.

Recommendation

In order to ensure that there are no conflicts, where a name is bought both in the new and old registry, there should be a 4-week period after switching to the new registrar before the new registrar allows registering new domains.

Low, character set ambiguity

There are a lot of room for ambiguity in ENS-names.

• Linebreaks, linefeeds
• Invisible whitespace
• l2r-characters
• Various alphabets with characters near-indistinguishable from latin characters.

Recommendations

This is not a problem for ENS in itself, but may be a challenge for apps and browsers which use the ENS namesystem, to prevent phishing due to users being lured by fake domain names.

Info: Deed.setBalance should not be payable

The payable modifier is not needed for setBalance:

function setBalance(uint newValue, bool throwOnFailure) onlyRegistrar onlyActive payable {

Recommendation

Remove the payable modifier

Info: ENS.lll should revert instead of bad jump

THe ENS.lll defines invalid_location as 0x02:

;; Jumping here causes an EVM error.
(def 'invalid-location 0x02)

This is then invoked to create an error: (jump invalid-location)))

This mechanism is how Solidity used to implement throw, however, with the introduction of the revert opcode, a better way to implement throw (which Solidity now uses under the hood), so to push two 0x00 on the stack and call revert. Before Metropolis, this will be functionally equal to invalid jump destination, but after Metrolpolis it will save gas for the caller.

Recommendation

Use revert instead of illegal jump.

Miscalleneous

Not directly related to security, but 'quirks' in the implementation.

1. Why is finalizeAuction using onlyOwner(_hash) ?

This seems strange, but the reason is that it sets back the owner to the deed-owner. The owner could have changed the ENS-owner to be another address, and allowing finalizeAuction to be set by anyone else would thus make it possible to grief the original owner.

In this sense, finalizeAuction serves two purposes:

• Recover the bond between deed and ENS
• Finalize the auction (releasing extraneous money from the deed)

It could make sense to split this into two functions:

• finalizeAuction, to only release extraneous moeny from the deed
• claimDeed, to set the subnodeowner in ENS according to the deed.

2. transfer does not check if we're still the registrar.

In the other places where ens.setSubnodeOwner is used, the code first validates that this is the current registrar. By not doing that, this code basically disallows transfer of deeds/domains on old registrars. This is not a big issue, the owner can still release the deed.