ENS contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

You should use call() instead of transfer(). If the recipient is a contract it might use more than 2300 gas which will make the transaction fail if the ETH was sent with transfer().

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Since the transfer is always at the end of the function, there shouldn't be any risk of reentrancy.

Proof of Concept

Tools Used

none

Recommended Mitigation Steps

Use call() instead.

Report

• Low
  • L-01: Use a single Ownable contract consistently
  • L-02: use a two-step process for critical address changes
• Non-Critical
  • N-01: abiencoder v2 is enabled by default
  • N-02: The public keyword for constructors was removed in 0.7.0
  • N-03: emit an event when changing a contract's configuration
  • N-04: SafeMath is unnecessary since 0.8.0
  • N-05: Files with unused imports

Low

L-01: Use a single Ownable contract consistently

The repository normally uses OZ's Ownable contract. But, the new DNSECImpl contract uses a custom Owned contract. There's no benefit in introducing another one doing the same thing. I recommend sticking to one.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L16

L-02: use a two-step process for critical address changes

Consider using a two-step process for transferring the ownership of a contract. While it costs a little more gas, it's safer than transferring directly.

Here's an example from the Compound Timelock contract: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/Owned.sol#L19

Non-Critical

N-01: abiencoder v2 is enabled by default

ABI encoder v2 is enabled by default. You don't have to explicitly enable it anymore, see https://github.com/ethereum/solidity/blob/develop/Changelog.md#080-2020-12-16

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L3

N-02: The public keyword for constructors was removed in 0.7.0

Constructors now don't have any extra keywords, see https://docs.soliditylang.org/en/v0.7.0/070-breaking-changes.html#functions-and-events. You should remove it.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/Owned.sol#L14

N-03: emit an event when changing a contract's configuration

It allows third parties to track those changes efficiently.

e.g.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/Owned.sol#L19
• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L56
• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L109

N-04: SafeMath is unnecessary since 0.8.0

Under- and overflow checks are built-in since 0.8.0. Using the SafeMath library is not necessary anymore.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StablePriceOracle.sol#L15

N-05: Files with unused imports

Remove unused imports to clearly see which contracts and libraries the contract uses.

e.g.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StablePriceOracle.sol#L6

Gas Report

G-01: use unchecked when incrementing the loop's variable

Since the loop's variable can't overflow in most circumstances, you can safe gas by incrementing it inside a unchecked block:

for (uint i; i < length;) {
    unchecked {
        ++i;
    }
}

e.g.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L14
• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L256

G-02: Don't allow the user to send more ETH than necessary allows you to remove the refund logic

In the ETHRegistraryController contract, you allow the user to pay more than necessary for the registration. Any surplus amount is then refunded.

If you force the user to pay the exact amount, you can remove the refund logic and thus save gas.

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L138
• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L183