Consensys Diligence recently conducted an audit on upcoming upgrades to the Ethereum Name Service (ENS). The changes, which roll out on May 4th, 2019, include a yearly rent model that replaces the old auction system and a new DNS integration that will open up ENS names under ~1300 top-level domains.
Our audit report is publicly available.
The focus of the audit was to verify that the smart contract system is secure, resilient and working according to its specifications. The audit activities can be grouped in the following three categories:
Security: Identifying security related issues within each contract and within the system of contracts.
Sound Architecture: Evaluation of the architecture of this system through the lens of established smart contract best practices and general software best practices.
Code Correctness and Quality: A full review of the contract source code.
| Minor | Medium | Major | Critical | |
|---|---|---|---|---|
| Open | 0 | 0 | 0 | 0 |
| Closed | 6 | 5 | 1 | 3 |
The audit team found some interesting bugs during this audit. We expand on two of the findings here.
Front-running is a pervasive issue on blockchain. It is really challenging to foresee all possible race conditions in the logic of the DApp. We previously published extensively on the fact that front-running is a very common type of security vulnerability in Ethereum DApps: Transparent Dishonesty: Taxonomy of front-running attacks on Blockchain.
ENS has been one of the cutting edge DApps in the Ethereum ecosystem, starting back in April 2016 as EIP-137. ENS famously implemented Vickrey Auctions for initial registration of the domains through EIP-162, and used commit and reveal schemes to prevent domain sniping and front-running.
As mentioned before, making DApps resilient against front-running is hard. Even though the ETHRegistrarController smart contract uses a proper commit and reveal scheme to prevent front-running, failing to include even one crucial variable can lead to opening up the attack vector again. On the implementation under audit, the owner was not included in the committed message, making it vulnerable to front-running, which allowed the commitment transaction to be revealed and registered by any address.
The solution? include the right set of data the user is committing. You can read more about this specific issue and the remediation here:
3.3 ETHRegistrarController.register is vulnerable to front running
This was an interesting find. On the last few days of the audit, when we thought we have gone through every piece of code in the scope, we decided to expand our search and look at the libraries used in the main contracts. buffer.sol was out of the scope but it was used throughout the main contracts.
buffer.init function was implemented as following:
function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) {
if (capacity % 32 != 0) {
capacity += 32 - (capacity % 32);
}
// Allocate space for the buffer data
buf.capacity = capacity;
assembly {
let ptr := mload(0x40)
mstore(buf, ptr)
mstore(ptr, 0)
mstore(0x40, add(ptr, capacity))
}
return buf;
}
Note that memory is reserved only for capacity bytes, but the bytes actually requires capacity + 32 bytes to account for the prefixed array length. Other functions in Buffer assume correct allocation and therefore corrupt nearby memory.
You can read more about this issue and the remediation here: 3.1 Memory corruption in Buffer
If you are interested in knowing more about the services Consensys Diligence offers, get in touch with us.