ENS contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L201 https://github.com/ensdomains/ens-contracts/blob/master/contracts/wrapper/NameWrapper.sol#L271

Vulnerability details

Impact

The ETHRegistrarController is calling renew from base registrar and not through Namewrapper. This means the fuses for the subdomain will not be updated via _setData. This impacts the permission model set over subdomain and could lead to takeover

Proof of Concept

1. Observe the renew function

function renew(string calldata name, uint256 duration)
        external
        payable
        override
    {
        ...

        uint256 expires = base.renew(uint256(label), duration);

        ....
    }

2. As we can see this is calling renew function of Base Registrar instead of NameWrapper. Since this is not going via NameWrapper fuses will not be set

3. Also since renew in NameWrapper can only be called via Controller which is ETHRegistrarController so there is no way to renew subdomain

Recommended Mitigation Steps

The ETHRegistrarController must renew using Namewrapper's renew contract

Lines of code

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

Vulnerability details

Impact

The RRSIG RR's Signer's Name field could differ from name of the RRSIG record due to incorrect checks

Proof of Concept

1. Assume name of RRSIG record is abc and RRSIG RR's Signer's Name is bc

2. In verifySignature it is checked that the RRSIG RR's Signer's Name field MUST be the name of the zone that contains the RRset

if(rrset.signerName.length > name.length
            || !rrset.signerName.equals(0, name, name.length - rrset.signerName.length))
        {
            revert InvalidSignerName(name, rrset.signerName);
        }

3. In our case it should not match as both are different but they will as we see below

4. Since signerName is bc so rrset.signerName.length is 2. Similarly name of RRSIG record is abc so name.length is 3

5. So length comparison gets false since 2>3 is false, passing first hurdle

6. name.length - rrset.signerName.length will become 3-2=1 so name offset becomes 1

7. This means equal comparison becomes like

rrset.signerName.equals(0, name, 1))

8. This checks bc=bc which is true and hence the validation passes even though names are different

Recommended Mitigation Steps

Revise if condition to fail if signer name length is not equal to name length

if(rrset.signerName.length != name.length
            || !rrset.signerName.equals(0, name, 0))

Default proof is returned when input is empty

Contract: DNSSECImpl.sol

Description:

1. If submitRRSets function is called with an empty list of RRSets then no RRSet gets submitted.
2. Instead of throwing an error that input is empty, function simply the proof (The DNSKEY or DS) which was passed as argument

Recommendation: Add below check

require(input.length>0, "No RRSET to submit");

2 step owner change

Contract: Owned.sol

Description: The setOwner function directly sets the new owner value passed without checking for 0 address or performing a 2 step change

Recommendation: Add a zero address change. Also add 2 step change for Admin which includes pendingAdmin inclusion

Use call instead of transfer

Contract ETHRegistrarController.sol

Description It was observed that withdraw function is using transfer function instead of call for transferring ether. This could become a problem if owner is contract and require more than 2300 gas (in which case transfer reverts)

Recommendation Use call which does not have 2300 gas limitation