ENS contest - _Adam's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L204 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L211

Vulnerability details

Impact

As transfer has a limit of 2,300 gas, when refunding excess Eth to smart contract accounts the transfer can fail. This can be due to:

• The contracts failure to implement a payable fallback function
• The contracts payable callback costs exceed the limit of 2300 gas
• The contract is called through a proxy.

Impact

The functions register, renew & withdraw will be limited to only EOA accounts or contract accounts with a very basic receive payable callback.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend adding reentrant guards and using call() instead of transfer().

[L01] 2 Step Ownership changes

Critical changes such as ownership changes should be a 2 step process to protect against human error. While the errors are unlikely important parts of the contract would become unusable if they occured.

Consider changing the following functions to 2 step procedures: Owned.sol#L18-L20

[L02] Events for Critical State changes

When modifying critical state variables such as changing owner, I recommend emiting an event to allow monitoring with off chain tools.

Consider adding an event to the setOwner function: Owned.sol#L19

[L03] Floating Pragma Versions

For non library/interface contracts I recommend not using a floating pragma version and deploying with the version that was used for testing.

Also the following is missing a pragma version: IBaseRegistrar.sol#L1

[N01] Named Returns and Return Statements

The following functions are using both named returns and return statements. I recommend only using one of them per function. BytesUtils.sol#L135-L136 DNSSECImpl.sol#L110-L139 RRUtils.sol#L38-L40 NameWrapper.sol#L744-L752 NameWrapper.sol#L832-L843 NameWrapper.sol#L853-L864

[N02] Open Todos

Recommend resolving and removing the following TODO before deployment. DNSSECImpl.sol#L238

[N03] Public Functions can be External

The following functions are never called within their given contract can be changed from public to external: DNSSECImpl.sol#L58 DNSSECImpl.sol#L69 Owned.sol#L18 NameWrapper.sol#L105 NameWrapper.sol#L128 NameWrapper.sol#L456-L461 Controllable.sol#L11

[N04] Incomplete Natspec

Natspec is missing in the following locations: BytesUtils.sol#L232 - missing @return DNSSECImpl.sol#L108 - missing @return DNSSECImpl.sol#L145 - missing @return DNSSECImpl.sol#L180 - missing @param rrset DNSSECImpl.sol#L229 - missing @param keyrdata ReverseRegistrar.sol#L73 - missing @param resolver ReverseRegistrar.sol#L129 - missing @param resolver NameWrapper.sol#L114 - missing @param tokenId NameWrapper.sol#L207 - missing @return NameWrapper.sol#L268 - missing @param expiry NameWrapper.sol#L371 - missing @return NameWrapper.sol#L398 - missing @param resolver

[N05] Missing SPDX-License-Identifier

SPDX-License-Identifier is missing in the following files: BytesUtils.sol RRUtils.sol SHA1.sol Owned.sol Algorithm.sol Digest.sol ETHRegistrarController.sol IETHRegistrarController.sol StringUtils.sol IBaseRegistrar.sol ReverseRegistrar.sol IReverseRegistrar.sol IMetadataService.sol ENS.sol

[G01] Unchecked Increments in Loops

When incrementing i in for loops there is no chance of overflow so unchecked can be used to save gas. I ran a simple test in remix and found deployment savings of 31,653 gas and on each function call saved ~141 gas per iteration.

contract Test {
	function loopTest() external {
		for (uint256 i; i < 1; ++i) {
		Deployment Cost: 125,637, Cost on function call: 24,601
		vs
		for (uint256 i; i < 1; ) {
		// for loop body
		unchecked { ++i }
		Deployment Cost: 93,984, Cost on function call: 24,460
		}
	}
}

For loops that can use unchecked increments: BytesUtils.sol#L266 BytesUtils.sol#L313 DNSSECImpl.sol#L93 ETHRegistrarController.sol#L256 ERC1155Fuse.sol#L92 ERC1155Fuse.sol#L205

[G02] Pre Increments in Loops

In for loops pre increments can be used to save a small amount of gas per iteration. I ran a test in remix using a for loop and found the deployment savings of 497 gas and ~5 gas per iteration.

contract Test {
	function loopTest() external {
		for (uint256 i; i < 1; i++) {
		(Deployment cost: 118,408, Cost on function call: 24,532)
		vs
		for (uint256 i; i < 1; ++i) {
		(Deployment cost: 117,911, Cost on function call: 24,527)
		}
	}
}

For loops that can use pre increments: BytesUtils.sol#L266 BytesUtils.sol#L313 DNSSECImpl.sol#L93 ETHRegistrarController.sol#L256

[G03] Custom Errors

As your using a solidity version > 0.8.4 you can replace revert strings with custom errors. This will save in deployment costs and runtime costs. Based on a test in remix, replacing a single revert string with a custom error saved 12,404 gas in deployment cost and 86 gas on each function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "check failed");
	}
}   (Deployment cost: 114,703, Cost on Function call: 23,392)
vs 
contract Test {
	uint256 a;
	error checkFailed();
	function check() external {
		if (a != 0) revert checkFailed();
	}
}   (Deployment cost: 102,299, Cost on Function call: 23,306)

Instances where custom errors can be implemented: RRUtils.sol#L307 ETHRegistrarController.sol#L99-L102 ETHRegistrarController.sol#L137-L139 ETHRegistrarController.sol#L196-L199 ETHRegistrarController.sol#L232-L242 ETHRegistrarController.sol#L259-L261 ReverseRegistrar.sol#L41-L54 BytesUtil.sol#L28 BytesUtil.sol#L42 ERC1155Fuse.sol#L60-L62 ERC1155Fuse.sol#L85-L87 ERC1155Fuse.sol#L107-L109 ERC1155Fuse.sol#L176-L179 ERC1155Fuse.sol#L195-L203 ERC1155Fuse.sol#L215-L217 ERC1155Fuse.sol#L248-L252 ERC1155Fuse.sol#L290-L293 Controllable.sol#L17

[G04] Long Revert Strings

If you opt not to use custom errors keeping revert strings <= 32 bytes in length will save gas. I ran a test in remix and found the savings for a single short revert string vs long string to be 9,377 gas in deployment cost and 18 gas on function call.

contract Test {
	uint256 a;
	function check() external {
		require(a != 0, "short error message"); 
		(Deployment cost: 114,799, Cost on function call: 23,392)	
		vs 
		require(a != 0, "A longer Error Message over 32 bytes in     
        length"); 
		(Deployment cost: 124,176, Cost on function call: 23,410)	
	}
}

I recommend shortenning the following revert strings to < 32 bytes in length: ETHRegistrarController.sol#L99-L102 ETHRegistrarController.sol#L137-L139 ETHRegistrarController.sol#L198 ETHRegistrarController.sol#L232-L242 ETHRegistrarController.sol#L259-L261 ReverseRegistrar.sol#L41-L54 ERC1155Fuse.sol#L60-L62 ERC1155Fuse.sol#L85-L87 ERC1155Fuse.sol#L107-L109 ERC1155Fuse.sol#L176-L179 ERC1155Fuse.sol#L195-L203 ERC1155Fuse.sol#L215-L217 ERC1155Fuse.sol#L248-L252 ERC1155Fuse.sol#L290-L293 Controllable.sol#L17

[G05] && in Require Statements

If optimising for runtime costs over deployment costs you can seperate && in require functions into 2 parts. I ran a basic test in remix and it cost an extra 234 gas to deploy but will save ~9 gas everytime the require function is called. (note: If you upgrade to solidity version > 0.8.13 this is no longer true)

contract Test {
	uint256 a = 0;
	uint256 b = 1;

	function test() external {
		require(a == 0 && b > a) 
		(Deployment cost: 123,291, Cost on function call: 29,371)
		vs
		require(a == 0);
		require(b > a);
		(Deployment cost: 123,525, Cost on function call: 29,362)
	}
}

Require statements that can be split up: BytesUtils.sol#L268 ERC1155Fuse.sol#L215-L218 ERC1155Fuse.sol#L290-L293