ENS contest - Critical's results

Lines of code

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

Vulnerability details

function register(
    string calldata name,
    address owner,
    uint256 duration,
    bytes32 secret,
    address resolver,
    bytes[] calldata data,
    bool reverseRecord,
    uint32 fuses,
    uint64 wrapperExpiry
) public payable override {
    IPriceOracle.Price memory price = rentPrice(name, duration);
    require(
        msg.value >= (price.base + price.premium),
        "ETHRegistrarController: Not enough ether provided"
    );

    _consumeCommitment(
        name,
        duration,
        makeCommitment(
            name,
            owner,
            duration,
            secret,
            resolver,
            data,
            reverseRecord,
            fuses,
            wrapperExpiry
        )
    );

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

    function makeCommitment(
        string memory name,
        address owner,
        uint256 duration,
        bytes32 secret,
        address resolver,
        bytes[] calldata data,
        bool reverseRecord,
        uint32 fuses,
        uint64 wrapperExpiry
    ) public pure override returns (bytes32) {
        bytes32 label = keccak256(bytes(name));
        if (data.length > 0) {
            require(
                resolver != address(0),
                "ETHRegistrarController: resolver is required when data is supplied"
            );
        }
        return
            keccak256(
                abi.encode(
                    label,
                    owner,
                    duration,
                    resolver,
                    data,
                    secret,
                    reverseRecord,
                    fuses,
                    wrapperExpiry
                )
            );
    }

The Commitment is an important mechanism to safeguard users from front run register, the commitment design should make it impossible to front run a register() transaction.

However, we noticed that the makeCommitment is a public function to generate the commitment later to be used for the onchain commit() call.

Impact

Even though it's not necessarily so (the frontend may implement their own local/offline makeCommitment function), the existence of this public makeCommitment() method on the ETHRegistrarController contract creates a Schelling point, which makes it more likely for the frontend choose just to use it.

In which case, the RPC will be able to know the exact domain the user is planning to register, they can later further confirm it (when the user sends the commit() transaction) by comparing the hash with the previous makeCommitment() calls.

We usually consider the public RPC providers trustworthy or harmless, but they can be malicious when they are compromised. And always, they should not be trusted in the first place.

Public RPC gateway provided by Ankr for Polygon (https://polygon-rpc.com) and Fantom (https://rpc.ftm.tools) were comprised via DNS hijack on Jul 1, 2022:

https://twitter.com/Mudit__Gupta/status/1542840515292532736

Recommended Mitigation Steps

Change makeCommitment() from public to internal, and instruct all official frontend and third-party user interfaces to implement an offline commitment computation function and use that instead.