ENS contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/BytesUtils.sol#L115-L127

Vulnerability details

Impact

equals with offset might return true when equals without offset returns false.

Proof of Concept

The problem is that self.length could be greater than other.length + offset, it should be ==, or it should contain a length argument.

Here you have an example of the failure:

• equals(0x0102030000, 0, 0x010203) => return true

decoded input	{
	"bytes self": "0x0102030000",
	"uint256 offset": "0",
	"bytes other": "0x010203"
}
decoded output	{
	"0": "bool: true"
}

Recommended Mitigation Steps

    function equals(bytes memory self, uint offset, bytes memory other) internal pure returns (bool) {
-       return self.length >= offset + other.length && equals(self, offset, other, 0, other.length);
+       return self.length == offset + other.length && equals(self, offset, other, 0, other.length);
    }

• Low
  • 1. Very Outdated packages
  • 2. Outdated compiler
  • 3. Lack of ACK during owner change
  • 4. Lack of check
  • 5. Ether could be blocked
  • 6. Use encode instead of encodePacked for hashig
  • 7. Logic mismatch in ERC1155Fuse
  • 8. now alias should be avoided
• Non-Critical or OutOfScope
  • 9. Open TODO
  • 10. [OutOfScope] Use abstract for base contracts
  • 11. [OutOfScope] Lack of auth

Low

1. Very Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

"@ensdomains/buffer": "^0.0.13" => 0.1.0
"@openzeppelin/contracts": "^4.1.0" => 4.7.0

2. Outdated compiler

The pragma version used is:

pragma solidity ^0.8.4;
pragma solidity ^0.8.13;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.15

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• Ownable.sol#L18
• Owned.sol#L19
• ENSRegistryWithFallback.sol#L59

4. Lack of check

The following lines requires a valid address as an argument, in some of them, once the value is set, it cannot be changed again, so it is mandatory to check if these values are as expected.

Affected source code for address(0):

• PublicResolver.sol#L55-L58
• ENSRegistryWithFallback.sol#L16
• ETHRegistrarController.sol#L50-L55
• FIFSRegistrar.sol#L24
• TestRegistrar.sol#L22
• ReverseRegistrar.sol#L29
• NameWrapper.sol#L54-L56

5. Ether could be blocked

The user's ether could be locked and unrecoverable.

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it will be impossible for this contract cancel the offer or receive funds back to that address.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected lines:

High impact:

• ETHRegistrarController.sol#L211

Low impact:

• ETHRegistrarController.sol#L183
• ETHRegistrarController.sol#L204

Recommended Mitigation Steps:

• Use .call instead of .transfer.

6. Use encode instead of encodePacked for hashig

Use of abi.encodePacked is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• ETHRegistrarController.sol#L255
• FIFSRegistrar.sol#L13
• ReverseRegistrar.sol#L83
• ReverseRegistrar.sol#L151
• ENSRegistry.sol#L75

7. Logic mismatch in ERC1155Fuse

In the contract ERC1155Fuse the methods safeTransferFrom and safeBatchTransferFrom has different logic when the to is the same owner. if the user use safeTransferFrom to transfer a not owned item to the same owner, nothing will happend, and no-revert will ocurr, but if you use safeBatchTransferFrom a revert will happend.

In the ERC1155Fuse contract, the safeTransferFrom and safeBatchTransferFrom methods have different logic when the destination (to) is the owner. if the user uses safeTransferFrom to transfer an item that they don't own to the same owner, nothing will happen and it won't be reverted, but if they use safeBatchTransferFrom, it will be reverted.

Affected source code:

• ERC1155Fuse.sol#L282
• ERC1155Fuse.sol#L216

8. now alias should be avoided

There is an argument named now, this is an alias that could work in a different way with different versions of solidity because it's a reserved word. As the documentation said is not critical, but should be avoided.

In version 0.7.0, the alias now (for block.timestamp) was removed.

Reference:

• https://docs.soliditylang.org/en/latest/units-and-global-variables.html?highlight=now#block-and-transaction-properties

Affected source code:

• DNSSECImpl.sol#L91-L94
• DNSSECImpl.sol#L110-L133

Non-Critical or OutOfScope

9. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

• DNSSECImpl.sol#L238

10. [OutOfScope] Use abstract for base contracts

Abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• OwnedResolver.sol#L16
• TestResolver.sol#L6

11. [OutOfScope] Lack of auth

The following methods do not have any authentication and the states can be modified by anyone:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/TestResolver.sol#L21 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/DummyOracle.sol#L11 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/TestRegistrar.sol#L31

• Gas
  • 1. Reduce the size of error messages Long revert Strings
    • Use Custom Errors instead of Revert Strings to save Gas
  • 2. Gas saving using immutable
  • 3. Constants expressions are expressions, not constants
  • 4. ++i costs less gas compared to i++ or i += 1
  • 5. Don't use the length of an array for loops condition
  • 6. There's no need to set default values for variables
  • 7. Use abi.encodeWithSelector instead of abi.encodeWithSignature
  • 8. Avoid encodepacked
  • 9. delete optimization

Gas

1. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected source code:

• Controllable.sol#L13
• Controllable.sol#L17
• LowLevelCallUtils.sol#L24
• UpgradedNameWrapperMock.sol#L75
• PublicResolver.sol#L72
• ETHRegistrarController.sol#L101
• ETHRegistrarController.sol#L139
• ETHRegistrarController.sol#L198
• ETHRegistrarController.sol#L234-L242
• ETHRegistrarController.sol#L261
• ERC1155Fuse.sol#L62
• ERC1155Fuse.sol#L87
• ERC1155Fuse.sol#L109
• ERC1155Fuse.sol#L176
• ERC1155Fuse.sol#L197-L202
• ERC1155Fuse.sol#L248-L252
• ERC1155Fuse.sol#L322-L327
• ERC1155Fuse.sol#L354-L359
• ReverseRegistrar.sol#L46
• ReverseRegistrar.sol#L54

2. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• StaticMetadataService.sol#L5
• Root.sol#L15
• DummyProxyRegistry.sol#L4
• ENSRegistryWithFallback.sol#L11
• FIFSRegistrar.sol#L9-L11

3. constants expressions are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

• https://github.com/ethereum/solidity/issues/9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Affected source code:

• Root.sol#L10

4. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• Multicallable.sol#L10
• NameEncoder.sol#L27
• StringUtils.sol#L14
• ETHRegistrarController.sol#L256
• EllipticCurve.sol#L304
• BytesUtils.sol#L266
• BytesUtils.sol#L313
• DNSSECImpl.sol#L93

5. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• Multicallable.sol#L10
• ETHRegistrarController.sol#L256
• ERC1155Fuse.sol#L92
• ERC1155Fuse.sol#L205
• DNSSECImpl.sol#L93

6. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• NameEncoder.sol#L20
• DNSResolver.sol#L54-L55
• ERC1155Fuse.sol#L92
• ERC1155Fuse.sol#L205
• EllipticCurve.sol#L304
• BytesUtils.sol#L266
• DNSSECImpl.sol#L93

7. Use abi.encodeWithSelector instead of abi.encodeWithSignature

abi.encodeWithSelector is much cheaper than abi.encodeWithSignature because it doesn't require to compute the selector from the string.

Reference:

• https://docs.soliditylang.org/en/v0.8.15/units-and-global-variables.html#abi-encoding-and-decoding-functions
• https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

Affected source code:

• InterfaceResolver.sol#L45-L51

8. avoid encodepacked

Use encodepacked with one unique dynamic value will return the same output and input, so it can be avoided.

Affected source code:

• DNSResolver.sol#L64

9. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

    function _burn(uint256 tokenId) internal virtual {
        address owner = ownerOf(tokenId);
        // Clear fuses and set owner to 0
-       _setData(tokenId, address(0x0), 0, 0);
+       delete _tokens[tokenId];
        emit TransferSingle(msg.sender, owner, address(0x0), tokenId, 1);
    }

• ERC1155Fuse.sol#L270