From 2017-10-03 to 2017-10-08, Nick Johnson performed an audit of the Name Bazaar smart contracts. My findings are detailed below.
I, Nick Johnson have no stake or vested interest in Name Bazaar. This audit was performed under a contracted fixed fee with no other compensation.
This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Nick Johnson's keybase.io record.
This audit will evaluate whether the codebase follows the current established best practices for smart contract development.
This audit will evaluate whether the code does what it is intended to do.
This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.
This audit will look for any exploitable security vulnerabilities, or other potential threats to either the operators of ChainLink or its users.
This audit will examine how easily tested the code is, and review how thoroughly tested the code is.
Name Bazaar is a market for the resale of ENS domain names. It provides a pluggable system with different methods of selling names.
This audit uses the following terminology.
How likely a bug is to be encountered or exploited in the wild, as specified by the OWASP risk rating methodology.
The impact a bug would have if exploited, as specified by the OWASP risk rating methodology.
How serious the issue is, derived from Likelihood and Impact as specified by the OWASP risk rating methodology.
The Name Bazaar smart contract source code was made available in the district0x/name-bazaar Github repository.
The code was audited as of commit 3a722e9245c8a546c955139b427fc72327be6f17.
The following files were audited:
SHA1(resources/public/contracts/src/AuctionOffering.sol)= a056b2ae1315fc495ea72ec68380a04dd41df290
SHA1(resources/public/contracts/src/AuctionOfferingFactory.sol)= 47d2a1ae6796f334b9af3370ad2a2d0865e41a18
SHA1(resources/public/contracts/src/AuctionOfferingLibrary.sol)= 9235d3f9230b2342b19426bba07df91b66104ccd
SHA1(resources/public/contracts/src/BuyNowOffering.sol)= f7b7c2546eaed5a76b93f540b0802ba1d191a4fc
SHA1(resources/public/contracts/src/BuyNowOfferingFactory.sol)= 68c07dc8197befd8e57e905948e3cdd8cdd12ff7
SHA1(resources/public/contracts/src/BuyNowOfferingLibrary.sol)= eb536b92049694f363db7038c92c730c3cb63eb8
SHA1(resources/public/contracts/src/Offering.sol)= 1244227e3ed49ebe368610cd2d5aae8fc3b3fe95
SHA1(resources/public/contracts/src/OfferingFactory.sol)= 527564d588932cb576bf2599875707ee5e25f6c7
SHA1(resources/public/contracts/src/OfferingLibrary.sol)= 26588a56aa3c59eb52bb6893784f3e41ee4323d1
SHA1(resources/public/contracts/src/OfferingRegistry.sol)= 0104150898876dc20be8af2aa090c6d0b89ab077
SHA1(resources/public/contracts/src/UsedByFactories.sol)= 0101d22baaecb8c6e6dc4659e86663a169e657ce
Several observations about the coding style and readability of the contract are described in detail below.
Many conditions are checked inline that could be more clearly expressed as modifiers -
for instance, isSenderEmergencyMultisig, isContractNodeOwner, etc. We recommend
refactoring these as modifiers for ease of understanding and auditability.
The 'dispatcher' pattern, where the deployed contract is only a thin wrapper around a library, is used extensively throughout the code, and provides significant gas savings for deployment of commonly repeated contracts.
Changes in Byzantium permit a much simpler generic dispatcher, that provides a DELEGATECALL proxy to a base contract. This reduces the amount of code that needs to be reviewed and understood, saves even more gas, and improves understanding.
Given the imminence of the Byzantium hard fork, we recommend considering replacing these contracts with a generic dispatcher contract. Doing so will require careful reconsideration of what methods are exposed for direct user interaction.
Offering contracts effectively act as state machines, proceeding through a defined
life cycle. This is particularly apparent in the auction contract, where contracts
proceed from new -> bidding -> finished -> finalized. However, these state transitions
are implied from the combined contents of state variables such as highBidder and price,
and not explicitly declared anywhere. This leads to a lack of clarity around what
events can trigger state transitions, and what states an action may be taken in,
and can lead to bugs and vulnerabilities such as the recover-after-finalize one
described below.
An example of this is reclaimOwnership and transferOwnership in OfferingLibrary.
transferOwnership requires !wasOwnershipTransferred, which checks self.newOwner != 0;
setting self.newOwner is effectively a state transition. reclaimOwnership
transfers ownership of the name back to the seller, but does not set self.newOwner.
Determining whether this constitutes an exploitable bug then requires examining
all the callers to transferOwnership to determine if they could be called in a
situation in which reclaimOwnership has already been called, as well as
examining transferOwnership itself to determine if the call will fail for other
reasons if reclaimOwnership has been called previously. An explicit state transition,
and a state check on both of these functions, would eliminate that complexity and
make it significantly easier to verify correct operation.
We recommend refactoring this for more clarity, either introducing a function that determines the state from the set of variables, or better, introducing an explicit state variable.
Offering contracts serve an ephemeral purpose - the sale of a single ENS name - but no provision is allowed for contracts to clean up after themselves, by deleting data or self destructing obsolete contracts.
Both for gas efficiency and for good stewardship of the contract, we recommend consideration be given to having contracts self destruct once no longer needed, and factories remove mapping entries for destroyed contracts.
Rather than defining an event for each distinct operation that can be performed on a contract, a small number of generic events are defined, with string parameters describing the import of each event, and additional data in generic fields.
This leads to a lack of type safety around events, and makes events less self-descriptive. It also requires consumers to be familiar with all the code in order to know which events may be fired. We recommend instead using the standard pattern of an individual event type for each distinct operation on a contract.
Solidity permits the use of contract types interchangeably with the address
type. Doing so provides more type safety and clearer code, without affecting the
ABI.
We recommend functions such as setOfferingRegistry be modified accordingly,
to take or return contract types instead of addresses.
The offering struct is large and has many fields that are duplicated across
all open offerings, such as offeringRegistry, registrar and emergencyMultisig.
This incurs significant storage cost, which could be alleviated by each offering
storing a single pointer to the factory or contract which contains all of this data.
Further, a number of fields will only store small amounts of data (such as timestamps and version numbers); further reductions in gas cost could be effected by storing these using shorter data types that can be packed into a single storage slot.
Only very basic tests are provided. We strongly recommend writing more comprehensive tests for both 'happy paths' and failures.
Testing requires running a separate server process. No automated build is set up for the repository.
We recommend setting up an automated build, so new commits can be vetted against the existing test suite.
We found two note issues, two low issues, and three medium issues.
- Likelihood: low
- Impact: low
In AuctionOfferingLibrary, when a user submits a followup bid - that is, after
they have been outbid by another user but before withdrawing their refunded
funds - the value of their bid is computed from the amount sent plus the value
of refundable funds. This may lead to user confusion, with users bidding more than
they intend, if they do not account for the pending refunds.
We recommend that bid take an explicit value parameter, and return any excess
funds to the caller.
- Likelihood: low
- Impact: low
The finalize method on AuctionOffering takes a boolean parameter indicating
whether the seller should be paid immediately, or only credited. Since this method
is callable by anyone once the auction has ended, the caller gets to decide
whether or not the seller has to take additional action to claim their funds.
Further, it's unclear how this choice would be meaningfully presented to the user.
We recommend instead either always holding funds for the seller, or attempting
to send the funds using send, and falling back to crediting the owner in the
contract if the send fails. This latter principle could in fact be extended to all
instances in which funds are to be returned to a bidder, reducing the number of
necessary transactions.
- Likelihood: low
- Impact: medium
The auction end time parameter lacks sanity checks, permitting the seller to
set an end time arbitrarily far in the future. If a user submits an auction with
a far distant end time, and a user is foolish enough to bid on it, both the
bidder's funds and the seller's name effectively become tied up indefinitely.
Because the reclaimOwnership method is only available to the seller before a
bid is placed, even the seller is unable to recover their name at this point.
We recommend placing a sanity limit on the maximum duration of an auction.
- Likelihood: low
- Impact: medium
The extensionDuration parameter lacks both sanity checks and overflow checks.
As a result, a seller could list an auction that ends at a reasonable time,
but immediately extends for a long duration as soon as a bid is placed, trapping
the bidder's funds. By setting the value sufficiently high, the seller could
cause an overflow, creating an auction that ends earlier than expected, as soon
as a bid is placed after a critical time threshold.
We recommend limiting the extension duration to be no greater than the initial duration of the auction.
- Likelihood: low
- Impact: medium
The ENS registrar address is hardcoded in several contracts. Since the registrar address can and will change as the registrar is upgraded, this will require a redeployment of the factory contracts at the point of each registrar upgrade. Existing auctions will still complete successfully, as it's still possible to transfer deeds that have not been migrated to the new registrar.
We recommend that instead the ENS registry address (0x31415...) be hardcoded, and the current instance of the registrar be looked up using the registry when needed.
- Likelihood: low
- Impact: high
The contracts sensibly provide 'escape hatch' functionality controlled by an emergency multisig address, allowing return of names and funds to the users.
However, there is no global mechanism to freeze operations, and executing the emergency functionality requires the emergency multisig to call each offering's individual methods; in the case of an auction this can even require multiple operations for a single offering. In the situtation that this address is directly controlled by a multisig, this may prove untenable, and even when controlled by an external account, a race between the emergency address and an attacker is undesirable.
We recommend adding functionality for the emergency multisig to freeze operations, in order to permit time to remedy issues and restore funds to users without the time pressure implied by the current configuration.
- Likelihood: low
- Impact: high
The contracts utilise an onchain implementation of the namehash algorithm,
which translates textual names into ENS label and node hashes. However, this
onchain implementation does not perform UTS46
normalisation. As a result, the namehashes generated for some names will be
incorrect; for instance, 'FOO.eth' and 'foo.eth' should resolve to the same
namehash. The same applies to certain unicode names, and some names are outright
prohibited, such as those containing ASCII codepoints other than [a-z0-9-].
As a result, if dapps do not take the precautionary step of verifying that the provided name is already UTS46 normalised, a seller may be able to deceive a buyer into paying for a name that appears to be legitimate, but will never be resolved by compliant user-agents.
Unfortunately, due to the size of the UTS46 lookup tables, performing normalisation onchain may be impractical. In that situation, we recommend instead accepting the plain text name, label hash, and parent node hash from the caller, and expecting dapps to verify compliance before displaying a name - with appropriately dire warnings in the code about the necessity of doing this.
On a related note, we also recommend that dapps implement an IDN policy similar to chromium's in order to alert the user to attempts to sell homograph names (names that look identical in common fonts but use different Unicode glyphs).
- Likelihood: low
- Impact: high
reclaimOwnership contains the following code:
if (offering.isSenderEmergencyMultisig()) {
if (!hasNoBids(self) && !offering.wasEmergencyCancelled()) {
self.pendingReturns[self.winningBidder] = offering.price;
}
}
This is intended to return funds to the winning bidder in the event that an auction is cancelled by the emergency multisig before the auction has completed.
However, there is no check that the call takes place during the auction. As a result, the following sequence of operations is possible:
- Alice lists her name for auction
- Bob bids on the auction
- The auction concludes, and Bob calls
finalizewithtransferPrice=false, transferring ownership of the name to Bob, and crediting the winning amount to Alice in the contract mapping, but not sending the funds. - The multisig calls
reclaimOwnership. Because of the code above, Bob is also credited with the winning amount in the contract mapping. - Bob calls
withdrawand has his winning bid returned to him.
We recommend adding explicit state checks, as described in the general notes section.
None found.
None found.