Linea ENS

Description

In the verifyAccountProof function of the LineaProofHelper contract, the account data fetched from the L2 resolver is verified, the key of account proof(AccountProofStruct.key) is expected to match the target address (L2 ENS resolver address) set by the ENS name owner in L1Resolver contract (L1Resolver.setTarget), This target address corresponds to the ENS fetch request (EVMFetchRequest.target) on L1. However there is no verification of the key against the target address, Additionally, in fetch function, the target address is not specified in the extradata field of the reverted OffchainLookup structure and thus not available in the callback function getStorageSlotsCallback. As a result, the gateway could return fake ENS data fetched from an incorrect L2 resolver not matching the fetching target address, or the client could replace the response from the gateway with data from an incorrect L2 resolver and then feed the response to getStorageSlotsCallback, consequently the user would receive the fake ENS data.

Examples

packages/l1-contracts/contracts/linea-verifier/EVMFetcher.sol:L256-L262

abi.encode(
    request.verifier,
    request.commands,
    request.constants,
    callbackId,
    callbackData
)

packages/l1-contracts/contracts/linea-verifier/EVMFetchTarget.sol:L24-L33

(
    IEVMVerifier verifier,
    bytes32[] memory commands,
    bytes[] memory constants,
    bytes4 callback,
    bytes memory callbackData
) = abi.decode(
        extradata,
        (IEVMVerifier, bytes32[], bytes[], bytes4, bytes)
    );

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L154-L183

function verifyAccountProof(
    AccountProofStruct memory accountProof,
    bytes32 stateRoot
) private pure returns (bool) {
    bool accountProofVerified = SparseMerkleProof.verifyProof(
        accountProof.proof.proofRelatedNodes,
        accountProof.leafIndex,
        stateRoot
    );

    require(
        accountProofVerified,
        "LineaResolverStub: invalid account proof"
    );

    bytes32 hAccountValue = SparseMerkleProof.hashAccountValue(
        accountProof.proof.value
    );

    SparseMerkleProof.Leaf memory accountLeaf = SparseMerkleProof.getLeaf(
        accountProof.proof.proofRelatedNodes[41]
    );

    require(
        accountLeaf.hValue == hAccountValue,
        "LineaResolverStub: account value invalid"
    );

    return true;
}

Recommendation

Add target data in the fetch and getStorageSlotsCallback function, meanwhile add target address verification in verifyAccountProof function.

Description

The storage slot value returned by the gateway is verified by LineaSparseProofVerifier using a Merkel proof, the Merkel root is fetched from Linea based on a block number (IRollup(_rollup).stateRootHashes(blockNo)) in the storage proof provided by the gateway. However the returned block number is not checked against the latest block number on L2 at the time of fetching request from the user, this allows the gateway to potentially return an incorrect L2 block number (either earlier or later) along with incorrect storage proof. Despite these inconsistencies, the verification process would pass and the user would receive incorrect ENS data.

Examples

packages/l1-contracts/contracts/linea-verifier/LineaSparseProofVerifier.sol:L22-L50

function getStorageValues(
    bytes32[] memory commands,
    bytes[] memory constants,
    bytes memory proof
) external view returns (bytes[] memory values) {
    (
        uint256 blockNo,
        AccountProofStruct memory accountProof,
        StorageProofStruct[] memory storageProofs
    ) = abi.decode(
            proof,
            (uint256, AccountProofStruct, StorageProofStruct[])
        );

    bytes32 stateRoot = IRollup(_rollup).stateRootHashes(blockNo);
    require(
        stateRoot != bytes32(0),
        "LineaResolverStub: invalid state root"
    );

    return
        LineaProofHelper.getStorageValues(
            commands,
            constants,
            stateRoot,
            accountProof,
            storageProofs
        );
}

packages/l1-contracts/contracts/linea-verifier/EVMFetcher.sol:L248-L264

    revert OffchainLookup(
        address(this),
        request.verifier.gatewayURLs(),
        abi.encodeCall(
            IEVMGateway.getStorageSlots,
            (request.target, request.commands, request.constants)
        ),
        EVMFetchTarget.getStorageSlotsCallback.selector,
        abi.encode(
            request.verifier,
            request.commands,
            request.constants,
            callbackId,
            callbackData
        )
    );
}

packages/l1-contracts/contracts/linea-verifier/EVMFetchTarget.sol:L19-L38

function getStorageSlotsCallback(
    bytes calldata response,
    bytes calldata extradata
) external {
    bytes memory proof = abi.decode(response, (bytes));
    (
        IEVMVerifier verifier,
        bytes32[] memory commands,
        bytes[] memory constants,
        bytes4 callback,
        bytes memory callbackData
    ) = abi.decode(
            extradata,
            (IEVMVerifier, bytes32[], bytes[], bytes4, bytes)
        );
    bytes[] memory values = verifier.getStorageValues(
        commands,
        constants,
        proof
    );

Recommendation

We recommend that client include the latest L2 block number at the time of fetching through a parameter L2blockNumber in the extradata of revert OffchainLookup in fetch function,

abi.encode(
    request.verifier,
    request.commands,
    request.constants,
    callbackId,
    callbackData,
    L2blockNumber
)

then verify the block number from the proof against the corresponding block number decoded from extradata in getStorageSlotsCallback by getStorageValues of LineaSparseProofVerifier

Description

In the hash function of the Mimc library, because of the following block of code:

                lastChunk := shr(mul(sub(0x20, remaining), 0x8), lastChunk)

Different calldata can result in the same hash. For example, by passing:

0x00000000000000000000000000000000000000000000000000000000000000000001, 0x000000000000000000000000000000000000000000000000000000000000000000000001

the mimcHash variable will be 0x0abfc49c04232d502cde9bfe87350d596ee3a8d4f736f747dc163c21991adab2 in both cases. This can lead to unexpected behavior within the protocol and integrator problems using the external mimcHash function, resulting in hash collisions.

Examples

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L54-L56

function mimcHash(bytes calldata _input) external pure returns (bytes32) {
    return Mimc.hash(_input);
}

packages/l1-contracts/contracts/linea-verifier/lib/Mimc.sol:L24-L44

function hash(
    bytes calldata _msg
) external pure returns (bytes32 mimcHash) {
    assembly {
        let chunks := div(add(_msg.length, 0x1f), 0x20)

        for {
            let i := 0
        } lt(i, sub(chunks, 1)) {
            i := add(i, 1)
        } {
            let offset := add(_msg.offset, mul(i, 0x20))
            let chunk := calldataload(offset)

            let r := encrypt(mimcHash, chunk)
            mimcHash := addmod(
                addmod(mimcHash, r, FR_FIELD),
                chunk,
                FR_FIELD
            )
        }

Recommendation

We recommend validating that the input code is a multiple of 32 bytes in length.

Description

In the register function of the ETHRegistrarController contract, in the _consumeCommitment function, there is a validation for the minimal registration duration. However, there is no check for the maximal registration duration, which leads to the overflow in the NameWrapper contract. The MAX_EXPIRY variable in the ETHRegistrarController contract is unused.

Consider the following PoC, which is a modification of the test from the TestEthRegistrarController file:

  it.only('should permit new registrations with resolver and records', async () => {
    var commitment = await controller2.makeCommitment(
      'newconfigname',
      registrantAccount,
      // 18446744073709551616 - 7776000
      // uintMax64 + 1 - grace period
      (18446744073701775616).toString(),
      secret,
      resolver.address,
      callData,
      false,
      5,
    )
    var tx = await controller2.commit(commitment)

    await evm.advanceTime((await controller2.minCommitmentAge()).toNumber())
    var tx = await controller2.register(
      'newconfigname',
      registrantAccount,
      (18446744073701775616).toString(),
      secret,
      resolver.address,
      callData,
      false,
      5,
      { value: ethers.BigNumber.from((300000000000000*(28*DAY)+3*DAY).toString()).toString() },
    )
   
    await expect(tx)
      .to.emit(controller, 'NameRegistered')
  })

In the registerAndWrap function, when the duration passed is 18446744073701775616, which is uintMax64 value + 1 (to lead to the overflow) - 7776000 (GRACE_PERIOD), the uint64(registrarExpiry) + GRACE_PERIOD statement overflows:

        registrarExpiry = registrar.register(tokenId, address(this), duration); // <-- normal value, 18446744075420660688
        _wrap(
            label,
            wrappedOwner,
            ownerControlledFuses,
            uint64(registrarExpiry) + GRACE_PERIOD, // <-- overflow, 1718885072
            resolver
        );
    }

Which makes the wrapped name immediately expired.

Based on the documentation of the ENS:

If the name is a .eth 2LD, then the expiry will automatically be set to the same expiry in the .eth Registrar. But for all other names, the parent can choose what expiry to set for a child name.

However, due to the overflow, the expiry date in Registrar and Wrapper will differ.

When the name’s expiration date (from the .eth Registrar) has been reached, and the name is now in the grace period, all Name Wrapper operations on the name will be restricted.

The owner will not yet lose ownership of the name, but they will also not be able to unwrap or update the name until it has been renewed.

The owner will still have ownership of the name. However, he will have to spend excessive money to renew the wrapped name. This problem can occur when the price oracle returns cheap prices for names or when the price for a name is fixed. For example, in the Linea ENS scope, there is a FixedPriceOracle contract that returns the FIXED_PRICE_ETH price for the name. If the user decides to buy it and specify a large duration value, it will lead to the overflow, causing unexpected behavior and creating an already outdated name.

Examples

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L337-L353

function registerAndWrap(
    string calldata label,
    address wrappedOwner,
    uint256 duration,
    address resolver,
    uint16 ownerControlledFuses
) external onlyController returns (uint256 registrarExpiry) {
    uint256 tokenId = uint256(keccak256(bytes(label)));
    registrarExpiry = registrar.register(tokenId, address(this), duration);
    _wrap(
        label,
        wrappedOwner,
        ownerControlledFuses,
        uint64(registrarExpiry) + GRACE_PERIOD,
        resolver
    );
}

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L286-L310

function register(
    string calldata name,
    address owner,
    uint256 duration,
    bytes32 secret,
    address resolver,
    bytes[] calldata data,
    bool reverseRecord,
    uint16 ownerControlledFuses
) public payable override {
    IPriceOracle.Price memory price = rentPrice(name, duration);
    if (msg.value < price.base + price.premium) {
        revert InsufficientValue();
    }
    uint256 expires = _register(
        name,
        owner,
        duration,
        secret,
        resolver,
        data,
        reverseRecord,
        ownerControlledFuses,
        false
    );

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L54

uint64 private constant MAX_EXPIRY = type(uint64).max;

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L478-L480

if (duration < MIN_REGISTRATION_DURATION) {
    revert DurationTooShort(duration);
}

Recommendation

We recommend validating the MAX_REGISTRATION_DURATION to prevent overflow issues.

Description

While this is out of scope, we wanted to point this out: for a non-baseDomain(linea.eth) name, it is wrapped with wrap function without fuses and expiry parameters: _wrap(node, name, wrappedOwner, 0, 0); If Linea were to support wrapping other .eth 2 level ENS names, for example foo.arb.eth or foo.scroll.eth, these names will be incorrectly wrapped without fuses and expiry. Although Linea currently only supports registration of base domain linea.eth, and this issue does not presently occur, it remains a potential problem should support for other .eth domains be added in the future.

Examples

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L428

_wrap(node, name, wrappedOwner, 0, 0);

Recommendation

Add support for other .eth 2 level domains in wrap function to ensure they are correctly wrapped with appropriate fuses and expiry parameters.

Description

In the getDynamicValue function of the LineaProofHelper contract, there is a while loop over the length of the value, where each 32-byte chunk of the value is verified for its correctness in the verifyStorageProof function. Furthermore, in the verifyStorageProof function, during the internal SparseMerkleProof.verifyProof() call, there is a for loop with 40 iterations executing the hash function, which is also gas-heavy and has a for loop inside. After formatting proofs, there is one more for loop in the _verify function with 40 iterations, where the hash function is also called.

To sum up, a value with a length of 32 bytes results in over 80 for loops executing custom logic, and the length of the value is not limited. This unoptimized and gas-heavy logic creates risks of a gas bomb attack, where function execution will cost more gas than the gas block limit. Additionally, a malicious user of the system can take advantage of the unoptimized system and drain all the gas from the resolver’s address, blocking the execution of other fetch requests.

For example, by setting the contract as a target:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.25;

contract TargetMock {
    mapping(uint256 => bytes) contentHash; // Slot 0

    constructor() {
        contentHash[0] = "";
        contentHash[1] = "";
        // for e.x. the value with the length of 960 bytes will result in at least 2400 `for` loops
        contentHash[2] = "0xe3010170122029f2...e22a892c7e3f1f"; // <-- value of the unlimited length
    }
}

And calling the resolve function of the L1Resolver contract with the contentHash selector will execute the _contentHash function, request the value, and after the resolver processes the value, the while loop will consume an unlimited amount of gas based on the variable’s length. Repeated calls to this value will drain all the native tokens from the resolver’s address for gas fees.

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L258-L270

function _contenthash(
    bytes32 node,
    address target
) private view returns (bytes memory) {
    EVMFetcher
        .newFetchRequest(verifier, target)
        .getStatic(RECORD_VERSIONS_SLOT)
        .element(node)
        .getDynamic(VERSIONABLE_HASHES_SLOT)
        .ref(0)
        .element(node)
        .fetch(this.contenthashCallback.selector, "");
}

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L102-L130

while (length > 0) {
    verifyStorageProof(
        account,
        storageProofs[proofIdx].leafIndex,
        storageProofs[proofIdx].proof.proofRelatedNodes,
        storageProofs[proofIdx].proof.value,
        bytes32(slot)
    );
    slot++;

    if (length < 32) {
        value = bytes.concat(
            value,
            sliceBytes(
                abi.encode(storageProofs[proofIdx++].proof.value),
                0,
                length
            )
        );

        length = 0;
    } else {
        value = bytes.concat(
            value,
            storageProofs[proofIdx++].proof.value
        );

        length -= 32;
    }

Recommendation

We recommend reviewing the current logic and optimizing it in the while loop, limiting the length of the dynamic values.

Description

In the SparseMerkleProof contract, there is an Account struct:

struct Account {
        uint64 nonce; // 8 bytes, but occupies full slot, so 32 bytes
        uint256 balance; // 32 bytes
        bytes32 storageRoot; // 32 bytes
        bytes32 mimcCodeHash; // 32 bytes
        bytes32 keccakCodeHash; // 32 bytes
        uint64 codeSize; // 8 bytes, but occupies full slot, so 32 bytes
    } // = 192 bytes

The total length of the Account struct is 192 bytes, and the struct could be optimized by putting the codeSize variable in the same slot as nonce. In the _parseAccount function of the SparseMerkleProof contract, there is a check that the length of the _value variable must not be less than 192 bytes, throwing a WrongBytesLength exception in such a case.

However, no exception is thrown if the length of _value is more than 192 bytes. Consequently, this excess data gets silently ignored during the abi.decode execution since it takes exactly 192 bytes for decoding and leftover bytes remain untouched. This creates a problem where, for different inputs, the result is the same. For example:

// Input data with 192 bytes:
0x000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000d2a66d5598b4fc5482c311f22d2dc657579b5452ab4b3e60fb1a9e9dbbfc99e00c24dd0468f02fbece668291f3c3eb20e06d1baec856f28430555967f2bf280d798c662debc23e8199fbf0b0a3a95649f2defe90af458d7f62c03881f916b3f0000000000000000000000000000000000000000000000000000000000003030
// Output:
tuple(uint64,uint256,bytes32,bytes32,bytes32,uint64): 1,0,0x0d2a66d5598b4fc5482c311f22d2dc657579b5452ab4b3e60fb1a9e9dbbfc99e,0x00c24dd0468f02fbece668291f3c3eb20e06d1baec856f28430555967f2bf280,0xd798c662debc23e8199fbf0b0a3a95649f2defe90af458d7f62c03881f916b3f,12336

// Input data with more than 192 bytes:
0x000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000d2a66d5598b4fc5482c311f22d2dc657579b5452ab4b3e60fb1a9e9dbbfc99e00c24dd0468f02fbece668291f3c3eb20e06d1baec856f28430555967f2bf280d798c662debc23e8199fbf0b0a3a95649f2defe90af458d7f62c03881f916b3f000000000000000000000000000000000000000000000000000000000000303000122313
// Output is the same:
tuple(uint64,uint256,bytes32,bytes32,bytes32,uint64): 1,0,0x0d2a66d5598b4fc5482c311f22d2dc657579b5452ab4b3e60fb1a9e9dbbfc99e,0x00c24dd0468f02fbece668291f3c3eb20e06d1baec856f28430555967f2bf280,0xd798c662debc23e8199fbf0b0a3a95649f2defe90af458d7f62c03881f916b3f,12336

In the hashAccountValue function of the SparseMerkleProof contract, the _parseAccount function is used for decoding the calldata. Because the function is used for hashing already decoded input values, there is a possible collision where different input values to the hashAccountValue function result in the same hash. This can be dangerous for future development, integrators, parts of code not in scope, and can cause unexpected behavior of the verifyAccountProof function.

Examples

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L9-L16

struct Account {
    uint64 nonce;
    uint256 balance;
    bytes32 storageRoot;
    bytes32 mimcCodeHash;
    bytes32 keccakCodeHash;
    uint64 codeSize;
}

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L133-L140

function _parseAccount(
    bytes calldata _value
) private pure returns (Account memory) {
    if (_value.length < 192) {
        revert WrongBytesLength(192, _value.length);
    }
    return abi.decode(_value, (Account));
}

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L85-L102

function hashAccountValue(
    bytes calldata _value
) external pure returns (bytes32) {
    Account memory account = _parseAccount(_value);
    (bytes32 msb, bytes32 lsb) = _splitBytes32(account.keccakCodeHash);
    return
        Mimc.hash(
            abi.encode(
                account.nonce,
                account.balance,
                account.storageRoot,
                account.mimcCodeHash,
                lsb,
                msb,
                account.codeSize
            )
        );
}

Recommendation

We recommend reviewing existing logic and adding a condition to validate that the length of _value is exactly 192 bytes.

Description

In the getStorageValues function of the LineaSparseProofVerifier contract, the proof bytes variable is passed, which after the abi.decode operation is decoded to the blockNo variable, accountProof struct, and storageProofs struct array. In both accountProof and StorageProofStruct structs, there is a leafIndex variable, which is not validated, as well as other variables after the decode operation. After the LineaProofHelper.getStorageValues call, the leafIndex variable from these structs can be used either in verifyStorageProof or verifyAccountProof functions, where it will be used in the verifyProof function, and then passed directly to the _verify function without any validations. In the _verify function of the SparseMerkleProof library, the currentIndex is used in the for loop, in order to compute a computedHash variable in the right order with the _proof[height] variable. However, since this variable is not validated, it can be any index, which will result in the same function execution as it is supposed to be. For example, suppose the correct leaf index is 100 and the tree depth is 40, so anyone can pass the leafIndex variable as 2**40 + 100 = 1099511627876, and the computedHash will be built correctly, even though the 1099511627876 leaf is not a part of the tree.

Examples

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L224-L243

function _verify(
    bytes32[] memory _proof,
    bytes32 _leafHash,
    uint256 _leafIndex,
    bytes32 _root,
    bytes32 _nextFreeNode
) private pure returns (bool) {
    bytes32 computedHash = _leafHash;
    uint256 currentIndex = _leafIndex;

    for (uint256 height; height < TREE_DEPTH; ++height) {
        if ((currentIndex >> height) & 1 == 1)
            computedHash = Mimc.hash(
                abi.encodePacked(_proof[height], computedHash)
            );
        else
            computedHash = Mimc.hash(
                abi.encodePacked(computedHash, _proof[height])
            );
    }

Recommendation

We recommend reviewing existing logic and adding validation to the leafIndex variable to ensure that the variable does not exceed the tree height.

Description

In the _isZeroBytes function of the SparseMerkleProof contract, there is no validation that the _data variable occupies all of the slot in each chunk. Due to missing validation, the function can incorrectly return false even when _data contains only zero bytes if the provided _data.length is not a multiple of 32 and there are subsequent non-zero bytes in calldata. The issue arises because the function assumes that each loop iteration processes exactly 32 bytes and does not properly handle cases where the final block of _data is less than 32 bytes but followed by non-zero bytes outside the _data range. This leads to misinterpretation of the byte array’s content, affecting processes or validations that rely on this function.

Examples

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L194-L214

function _isZeroBytes(
    bytes calldata _data
) private pure returns (bool isZeroBytes) {
    isZeroBytes = true;
    assembly {
        let dataStart := _data.offset

        for {
            let currentPtr := dataStart
        } lt(currentPtr, add(dataStart, _data.length)) {
            currentPtr := add(currentPtr, 0x20)
        } {
            let dataWord := calldataload(currentPtr)

            if eq(iszero(dataWord), 0) {
                isZeroBytes := 0
                break
            }
        }
    }
}

Consider the following PoC:

contract ReadExtraCalldata {

   function _isZeroBytes(
        bytes calldata _data
    ) public pure returns (bool isZeroBytes) {
        isZeroBytes = true;
        assembly {
            let dataStart := _data.offset

            for {
                let currentPtr := dataStart
            } lt(currentPtr, add(dataStart, _data.length)) {
                currentPtr := add(currentPtr, 0x20) // <-- takes full chunk of data for each iteration
            } {
                let dataWord := calldataload(currentPtr)

                if eq(iszero(dataWord), 0) {
                    isZeroBytes := 0
                    break
                }
            }
        }
    }


    function foo() public view returns (bool clean, bool dirty) {
        // form `data` variable, it consists of empty bytes, can be divided into two parts: 32 + 10
        bytes memory _data = new bytes(42);
        bytes memory _calldata = abi.encodeWithSelector(this._isZeroBytes.selector, _data);

        bool success;
        bytes memory result;

        // call the function with the clean calldata
        (success, result) = address(this).staticcall(_calldata);
        require(success);
        clean = abi.decode(result, (bool)); // <-- we receive true

        // Form dirty calldata. Note that _calldata is 132 bytes long:
        // 4 bytes: selector,
        // 32 bytes: offset,
        // 32 bytes: length,
        // 32 bytes: first byte of _data,
        // 32 bytes: padded second byte of _data.
        // We change the last bit of the padding:
        _calldata[131] = 0x01; // <-- this dirty byte is not a part of the _data variable, but a part of the calldata

        (success, result) = address(this).staticcall(_calldata);
        require(success);
        dirty = abi.decode(result, (bool)); // <-- we receive false, however in each example _data is empty bytes
    }

}

Recommendation

We recommend adjusting the assembly code to properly handle cases where _data.length does not align with 32-byte word boundaries. Specifically, the function should ensure that its _data.length is a multiple of 32, without inadvertently loading additional bytes beyond this limit.

Description

In the _formatProof function in the SparseMerkleProof contract there is no length validation of the first element in the _rawProof array. The function extracts the first 32 bytes of the _rawProof[0] array element to assign to nextFreeNode without ensuring that this element contains at least 32 bytes, which can lead to a panic error if the length of _rawProof[0] is less than 32 bytes.

Examples

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L162-L169

function _formatProof(
    bytes[] calldata _rawProof
) private pure returns (bytes32, bytes32, bytes32[] memory) {
    uint256 rawProofLength = _rawProof.length;
    uint256 formattedProofLength = rawProofLength - 2;

    bytes32[] memory proof = new bytes32[](formattedProofLength);
    bytes32 nextFreeNode = bytes32(_rawProof[0][:32]);

Recommendation

We recommend validating that the length of _rawProof[0] is at least 32 bytes before proceeding with the extraction of the first 32 bytes.

Description

In the constructor of contract ETHRegistrarController , the parameters pohVerifier and pohRegistrationManager are not checked against zero address. In contract NameWrapper and ETHRegistrarController, the base node(baseNode) and base domain(baseDomain) are not validated to ensure they are neither ETH_NODE nor ROOT_NODE, nor are they checked for being empty. Additionally, the baseNode is expected to be the namehash of the baseDomain. However these validations are missing in the constructors of both contracts. As a result if invalid base node or base domain is set, it could lead to the registration of invalid ENS names, causing conflicts in name resolution with the L1 ENS system.

Examples

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L130-L148

) ReverseClaimer(_ens, msg.sender) {
    if (_maxCommitmentAge <= _minCommitmentAge) {
        revert MaxCommitmentAgeTooLow();
    }

    if (_maxCommitmentAge > block.timestamp) {
        revert MaxCommitmentAgeTooHigh();
    }

    base = _base;
    prices = _prices;
    minCommitmentAge = _minCommitmentAge;
    maxCommitmentAge = _maxCommitmentAge;
    reverseRegistrar = _reverseRegistrar;
    nameWrapper = _nameWrapper;
    pohVerifier = _pohVerifier;
    pohRegistrationManager = _pohRegistrationManager;
    baseNode = _baseNode;
    baseDomain = _baseDomain;

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L77-L101

baseNode = _baseNode;

/* Burn PARENT_CANNOT_CONTROL and CANNOT_UNWRAP fuses for ROOT_NODE, ETH_NODE and baseNode and set expiry to max */

_setData(
    uint256(_baseNode),
    address(0),
    uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
    MAX_EXPIRY
);
_setData(
    uint256(ETH_NODE),
    address(0),
    uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
    MAX_EXPIRY
);
_setData(
    uint256(ROOT_NODE),
    address(0),
    uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
    MAX_EXPIRY
);
names[ROOT_NODE] = "\x00";
names[ETH_NODE] = "\x03eth\x00";
names[_baseNode] = _baseNodeDnsEncoded;

Recommendation

Add missing validations in the constructor of NameWrapper and ETHRegistrarController contract.

Description

In the function getStorageValues of the LineaProofHelper library, if the number of storage proofs (storageProofs) returned by the gateway is less than number of commands (commands.length), there will be an out-of-bounds array error. This situation arises when the gateway provides fewer storage proofs than the requested storage slots leading to a failure in getStorageValues.

Examples

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L232-L260

for (uint256 i = 0; i < commands.length; i++) {
    bytes32 command = commands[i];
    (bool isDynamic, uint256 slot) = computeFirstSlot(
        command,
        constants,
        values
    );
    if (!isDynamic) {
        if (!storageProofs[proofIdx].initialized) {
            values[i] = abi.encode(0);
            proofIdx++;
        } else {
            verifyStorageProof(
                account,
                storageProofs[proofIdx].leafIndex,
                storageProofs[proofIdx].proof.proofRelatedNodes,
                storageProofs[proofIdx].proof.value,
                bytes32(slot)
            );

            values[i] = abi.encode(
                storageProofs[proofIdx++].proof.value
            );

            if (values[i].length > 32) {
                revert InvalidSlotSize(values[i].length);
            }
        }
    } else {

Recommendation

Add a require statement to check whether the number of storage proofs is equal to the number of commands

Description

In the function verifyProof of the SparseMerkleProof contract, the length of the raw Merkel proof(_rawProof) should be same as the Merkel tree depth (TREE_DEPTH 42). However, there is no check of the length. If the length is less than 42, the function _verify would fail on an out of bounds of array access error:

for (uint256 height; height < TREE_DEPTH; ++height) {
            if ((currentIndex >> height) & 1 == 1)
                computedHash = Mimc.hash(abi.encodePacked(_proof[height], computedHash)
            );

Examples

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L36-L47

function verifyProof(
    bytes[] calldata _rawProof,
    uint256 _leafIndex,
    bytes32 _root
) external pure returns (bool) {
    (
        bytes32 nextFreeNode,
        bytes32 leafHash,
        bytes32[] memory proof
    ) = _formatProof(_rawProof);
    return _verify(proof, leafHash, _leafIndex, _root, nextFreeNode);
}

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L234-L243

for (uint256 height; height < TREE_DEPTH; ++height) {
    if ((currentIndex >> height) & 1 == 1)
        computedHash = Mimc.hash(
            abi.encodePacked(_proof[height], computedHash)
        );
    else
        computedHash = Mimc.hash(
            abi.encodePacked(computedHash, _proof[height])
        );
}

Recommendation

Add a require statement to verify that the length of _rawProof is 42 in the verifyProof function.

Description

In the hash function of the Mimc library, there is no validation of the length of the _msg variable. The _msg is divided into chunks in the assembly block, and after that, there is an iteration over the chunks:

        assembly {
            let chunks := div(add(_msg.length, 0x1f), 0x20)

            for {
                let i := 0
            } lt(i, sub(chunks, 1)) {
                i := add(i, 1)
            } {

For better understanding, this part of the code can be represented in Solidity like:

        uint chunks = (_msg.length + 31) / 32;

        unchecked {
            for (uint i; i < chunks - 1; ++i)
            {
            ...
            }
        }

Since there is no length validation of the _msg variable, this variable can be passed as an empty variable. In that case, the chunks variable will be equal to zero, and in the for loop, the expression chunks - 1 will underflow, leading to the for loop over type(uint).max elements, resulting in unbounded gas consumption and an out-of-gas panic error.

Recommendation

We recommend validating the length of the input calldata.

Description

In the function getStorageSlotsCallback of the EVMFetchTarget contract, abi.encodeWithSelector is used instead of abi.encodeCall. Since version 0.8.11, abi.encodeCall provides a type-safe encoding utility compared to abi.encodeWithSelector. While abi.encodeWithSelector can be used with interface.<function>.selector to prevent typographical errors, it lacks type checking during compile time, a feature offered by abi.encodeCall.

Examples

packages/l1-contracts/contracts/linea-verifier/EVMFetchTarget.sol:L42-L44

bytes memory ret = address(this).functionCall(
    abi.encodeWithSelector(callback, values, callbackData)
);

packages/l1-contracts/contracts/linea-verifier/EVMFetchTarget.sol:L42-L44

bytes memory ret = address(this).functionCall(
    abi.encodeWithSelector(callback, values, callbackData)
);

Recommendation

We recommend using abi.encodeCall instead of abi.encodeWithSelector to adhere to best practices in the web3 sphere.

Description

In the EVMFetchTarget contract in the getStorageSlotsCallback function, there is a return in the assembly block. However, the function is missing the modifier indicating that it will return any variable. Thus, the assembly block is executed, consuming gas, but no variable is returned.

Examples

packages/l1-contracts/contracts/linea-verifier/EVMFetchTarget.sol:L19-L48

function getStorageSlotsCallback(
    bytes calldata response,
    bytes calldata extradata
) external {
    bytes memory proof = abi.decode(response, (bytes));
    (
        IEVMVerifier verifier,
        bytes32[] memory commands,
        bytes[] memory constants,
        bytes4 callback,
        bytes memory callbackData
    ) = abi.decode(
            extradata,
            (IEVMVerifier, bytes32[], bytes[], bytes4, bytes)
        );
    bytes[] memory values = verifier.getStorageValues(
        commands,
        constants,
        proof
    );
    if (values.length != commands.length) {
        revert ResponseLengthMismatch(values.length, commands.length);
    }
    bytes memory ret = address(this).functionCall(
        abi.encodeWithSelector(callback, values, callbackData)
    );
    assembly {
        return(add(ret, 32), mload(ret))
    }
}

Recommendation

We recommend either removing the assembly block to save execution gas costs and keep the codebase clean or adding a modifier to the getStorageSlotsCallback function to return the ret variable to make the assembly work.

Description

In the L1Resolver contract and in the PohRegistrationManager contract, there are empty require() statements without providing reason strings or custom errors in Solidity. Using reason strings or custom errors in require() statements helps in debugging and understanding why a transaction was reverted, thereby improving the overall readability and maintainability of the code.

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L102

require(isAuthorised(node));

packages/l2-contracts/contracts/ethregistrar/PohRegistrationManager.sol:L14

require(managers[msg.sender]);

Recommendation

We recommend adding a descriptive reason string or using a custom error for the require() statement to provide more context when a transaction fails.

Description

In the PohRegistrationManager contract, the Ownable contract is used for access verification. However, the Ownable contract lacks protection for cases when there is a transfer of ownership to an incorrect account or to contracts that are unable to interact with the permission system. To eliminate such risks, we recommend utilizing the Ownable2Step contract, which includes a two-step mechanism to transfer ownership, where the new owner must call acceptOwnership in order to replace the old one.

Examples

packages/l2-contracts/contracts/ethregistrar/PohRegistrationManager.sol:L3

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

Recommendation

We recommend using Ownable2Step instead of Ownable.

Description

In the constructor of the PohVerifier contract, the signer value is assigned the value of msg.sender, however, no event is emitted. Not emitting an event when state variables change can make tracking the state of the contract more difficult and less transparent. For example, third-party services will not see to what value the signer was assigned when the contract was just deployed because the SignerUpdated event wasn’t emitted.

Examples

packages/l2-contracts/contracts/ethregistrar/PohVerifier.sol:L24-L27

constructor() EIP712(SIGNING_DOMAIN, SIGNATURE_VERSION) Ownable() {
    signer = msg.sender;
}

Recommendation

We recommend adding the SignerUpdated event to the constructor.

Description

In the following locations, there are unused imports:

• In ETHRegistrarController.sol#L17, the NameEncoder import is unused.
• In NameWrapper.sol#L4, the IERC165 import is unused.
• In NameWrapper.sol#L6, the CAN_DO_EVERYTHING import is unused.
• In NameWrapper.sol#L10, the IReverseRegistrar import is unused.
• In NameWrapper.sol#L14, the IERC1155 import is unused.

Examples

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L4

import {ERC1155Fuse, IERC165, IERC1155MetadataURI} from "./ERC1155Fuse.sol";

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L6

import {INameWrapper, CANNOT_UNWRAP, CANNOT_BURN_FUSES, CANNOT_TRANSFER, CANNOT_SET_RESOLVER, CANNOT_SET_TTL, CANNOT_CREATE_SUBDOMAIN, CANNOT_APPROVE, PARENT_CANNOT_CONTROL, CAN_DO_EVERYTHING, IS_DOT_ETH, CAN_EXTEND_EXPIRY, PARENT_CONTROLLED_FUSES, USER_SETTABLE_FUSES} from "./INameWrapper.sol";

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L10

import {IReverseRegistrar} from "../reverseRegistrar/IReverseRegistrar.sol";

packages/l2-contracts/contracts/wrapper/NameWrapper.sol:L14

import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";

Recommendation

We recommend removing unused imports to keep the codebase clean.

Description

In the LineaProofHelper contract and in the ETHRegistrarController contract, the AccountNotFound and Unauthorised custom errors are not used.

Examples

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L29

error Unauthorised(bytes32 node);

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L35

error AccountNotFound(address);

Recommendation

We recommend reviewing the use of these custom errors and removing them if they are not needed.

Description

In the function verifyProof of the SparseMerkleProof contract, there is documentation: Format input and verify sparse merkle proof, however, the function doesn’t verify anything. Both verifyProof and _verify functions are missing any require or if & revert statements. This leads to confusion and possible misuse of the function and makes the code less gas-efficient.

In the verifyAccountProof and verifyStorageProof functions, there are two separate require validations, which could be done in one require statement, making the code cleaner and saving deployment gas costs.

This also relates to the verify function of the PohVerifier contract.

Examples

packages/l2-contracts/contracts/ethregistrar/PohVerifier.sol:L39-L55

/**
 * @notice Verify the signature sent in parameter
 * @dev human is supposed to be a POH address, this is what is being signed by the POH API
 * @param signature The signature to verify
 * @param human the address for which the signature has been crafted
 */
function verify(
    bytes memory signature,
    address human
) public view virtual returns (bool) {
    bytes32 digest = _hashTypedDataV4(
        keccak256(abi.encode(keccak256("POH(address to)"), human))
    );

    address recoveredSigner = ECDSA.recover(digest, signature);
    return recoveredSigner == signer;
}

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L30-L47

/**
 * @notice Format input and verify sparse merkle proof
 * @param _rawProof Raw sparse merkle tree proof
 * @param _leafIndex Index of the leaf
 * @param _root Sparse merkle root
 */
function verifyProof(
    bytes[] calldata _rawProof,
    uint256 _leafIndex,
    bytes32 _root
) external pure returns (bool) {
    (
        bytes32 nextFreeNode,
        bytes32 leafHash,
        bytes32[] memory proof
    ) = _formatProof(_rawProof);
    return _verify(proof, leafHash, _leafIndex, _root, nextFreeNode);
}

packages/l1-contracts/contracts/linea-verifier/lib/SparseMerkleProof.sol:L216-L247

/**
 * @notice Verify sparse merkle proof
 * @param _proof Sparse merkle tree proof
 * @param _leafHash Leaf hash
 * @param _leafIndex Index of the leaf
 * @param _root Sparse merkle root
 * @param _nextFreeNode Next free node
 */
function _verify(
    bytes32[] memory _proof,
    bytes32 _leafHash,
    uint256 _leafIndex,
    bytes32 _root,
    bytes32 _nextFreeNode
) private pure returns (bool) {
    bytes32 computedHash = _leafHash;
    uint256 currentIndex = _leafIndex;

    for (uint256 height; height < TREE_DEPTH; ++height) {
        if ((currentIndex >> height) & 1 == 1)
            computedHash = Mimc.hash(
                abi.encodePacked(_proof[height], computedHash)
            );
        else
            computedHash = Mimc.hash(
                abi.encodePacked(computedHash, _proof[height])
            );
    }

    return
        Mimc.hash(abi.encodePacked(_nextFreeNode, computedHash)) == _root;
}

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L185-L201

function verifyStorageProof(
    SparseMerkleProof.Account memory account,
    uint256 leafIndex,
    bytes[] memory proof,
    bytes32 value,
    bytes32 key
) private pure {
    bool storageProofVerified = SparseMerkleProof.verifyProof(
        proof,
        leafIndex,
        account.storageRoot
    );

    require(
        storageProofVerified,
        "LineaResolverStub: invalid storage proof"
    );

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L154-L167

function verifyAccountProof(
    AccountProofStruct memory accountProof,
    bytes32 stateRoot
) private pure returns (bool) {
    bool accountProofVerified = SparseMerkleProof.verifyProof(
        accountProof.proof.proofRelatedNodes,
        accountProof.leafIndex,
        stateRoot
    );

    require(
        accountProofVerified,
        "LineaResolverStub: invalid account proof"
    );

Recommendation

We recommend adding the validations inside the verifyProof function to improve clarity and readability of the code, as well as decrease deployment gas costs.

Description

In the function getStorageSlotsCallback of the EVMFetchTarget contract, the response input parameter is being decoded back into bytes, which is unnecessary as response is already in bytes form. This decoding step is redundant and doesn’t perform any meaningful operation.

Examples

packages/l1-contracts/contracts/linea-verifier/EVMFetchTarget.sol:L19-L23

function getStorageSlotsCallback(
    bytes calldata response,
    bytes calldata extradata
) external {
    bytes memory proof = abi.decode(response, (bytes));

Recommendation

We recommend removing the decoding of the response variable into bytes to make the code cleaner and save execution gas costs. The decoding step is not necessary unless decoding response into a different form other than bytes.

Description

In the mentioned locations, public functions are used within the smart contracts, but these functions do not appear to be called internally. Functions declared as public can be called both externally and internally, which can result in larger deployment gas costs and a bigger size of the contract.

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L100

function setTarget(bytes calldata name, address target) public {

packages/l1-contracts/contracts/L1Resolver.sol:L205

function addrCallback(

packages/l1-contracts/contracts/L1Resolver.sol:L228

function addrCoinTypeCallback(

packages/l1-contracts/contracts/L1Resolver.sol:L251

function textCallback(

packages/l1-contracts/contracts/L1Resolver.sol:L272

function contenthashCallback(

packages/l1-contracts/contracts/L1Resolver.sol:L285

function metadata(bytes calldata name) public view returns (string memory) {

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L197

function commit(bytes32 commitment) public override {

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L216

function registerPoh(

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L286

function register(

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol:L444

function withdraw() public {

packages/l2-contracts/contracts/ethregistrar/PohRegistrationManager.sol:L31

function isRegistered(address _address) public view returns (bool) {

packages/l2-contracts/contracts/ethregistrar/PohVerifier.sol:L33

function setSigner(address _signer) public onlyOwner {

packages/l2-contracts/contracts/ethregistrar/PohVerifier.sol:L45

function verify(

packages/l2-contracts/contracts/ethregistrar/PohVerifier.sol:L60

function getSigner() public view returns (address) {

Recommendation

We recommend revising the modifier of these functions from public to external to optimize the smart contract gas usage and keep the codebase clean, as these functions are not needed internally.

Description

In the verifyAccountProof function of the LineaProofHelper contract, there is a magic number, specifically the index 41. A “magic number,” in computer programming, is a numeric literal in source code used without any explanation of its significance. This could lead to issues of understandability, maintainability, and potential errors in the code as there is no clarity on what 41 represents, while 41 is the last element of the proofRelatedNodes array variable. At the same time, in the verifyStorageProof function of the LineaProofHelper contract, a constant variable with the value 41 is used.

Examples

packages/l1-contracts/contracts/linea-verifier/LineaProofHelper.sol:L154-L182

function verifyAccountProof(
    AccountProofStruct memory accountProof,
    bytes32 stateRoot
) private pure returns (bool) {
    bool accountProofVerified = SparseMerkleProof.verifyProof(
        accountProof.proof.proofRelatedNodes,
        accountProof.leafIndex,
        stateRoot
    );

    require(
        accountProofVerified,
        "LineaResolverStub: invalid account proof"
    );

    bytes32 hAccountValue = SparseMerkleProof.hashAccountValue(
        accountProof.proof.value
    );

    SparseMerkleProof.Leaf memory accountLeaf = SparseMerkleProof.getLeaf(
        accountProof.proof.proofRelatedNodes[41]
    );

    require(
        accountLeaf.hValue == hAccountValue,
        "LineaResolverStub: account value invalid"
    );

    return true;

Recommendation

We recommend avoiding magic numbers in the code, removing the 41 value, and using the LAST_LEAF_INDEX constant variable with a value of 41 to keep the codebase clean.

Description

In the constructor of the L1Resolver contract, the variables graphqlUrl and l2ChainId can be marked as immutable as other variables like verifier, ens, and nameWrapper are already immutable. Since these variables are unchangeable, making them immutable will save gas on every execution.

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L40-L41

string public graphqlUrl;
uint256 public l2ChainId;

Recommendation

We recommend making these variables immutable.

Description

The resolve function in the L1Resolver contract is missing error handling for cases where the input data does not match any recognized selector. This function attempts to decode and process various types of requests based on the selector derived from the provided data. However, if none of the conditionals for known selectors match, the function will be executed without handling the case or notifying the caller that the provided data could not be resolved, and the execution of such a transaction will be successful, misleading the caller.

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L145-L174

function resolve(
    bytes calldata name,
    bytes calldata data
) external view returns (bytes memory result) {
    (, address target) = _getTarget(name, 0);
    bytes4 selector = bytes4(data);

    if (selector == IAddrResolver.addr.selector) {
        bytes32 node = abi.decode(data[4:], (bytes32));
        return _addr(node, target);
    }
    if (selector == IAddressResolver.addr.selector) {
        (bytes32 node, uint256 cointype) = abi.decode(
            data[4:],
            (bytes32, uint256)
        );
        return _addr(node, cointype, target);
    }
    if (selector == ITextResolver.text.selector) {
        (bytes32 node, string memory key) = abi.decode(
            data[4:],
            (bytes32, string)
        );
        return bytes(_text(node, key, target));
    }
    if (selector == IContentHashResolver.contenthash.selector) {
        bytes32 node = abi.decode(data[4:], (bytes32));
        return _contenthash(node, target);
    }
}

Recommendation

We recommend adding a revert statement when the input data’s selector does not match any known patterns. This approach ensures that users are informed when their queries cannot be processed, thus enhancing the robustness and usability of the contract.

Description

In the following locations, there are typos:

• nearlest instead of nearest
• aagainst instead of against

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L96

* Set target address to verify aagainst

packages/l1-contracts/contracts/L1Resolver.sol:L140

* @dev Resolve and verify a record stored in l2 target address. It supports subname by fetching target recursively to the nearlest parent.

Recommendation

We recommend fixing typos to keep the codebase clean.

Description

In the resolve function of the L1Resolver contract, there is a lack of proper validation for the length of the data parameter. This function decodes the data parameter assuming it is sufficiently large to contain specific selector information followed by additional data. The current implementation extracts a 4-byte selector from data and proceeds to decode further bytes without ensuring that data contains enough bytes. This can lead to out-of-bounds access if data is less than 4 bytes and the transaction will fail with a panic error.

Examples

packages/l1-contracts/contracts/L1Resolver.sol:L145-L154

function resolve(
    bytes calldata name,
    bytes calldata data
) external view returns (bytes memory result) {
    (, address target) = _getTarget(name, 0);
    bytes4 selector = bytes4(data);

    if (selector == IAddrResolver.addr.selector) {
        bytes32 node = abi.decode(data[4:], (bytes32));
        return _addr(node, target);

Recommendation

We recommend adding a validation at the beginning of the resolve function to verify that the data parameter is longer than 4 bytes.