Name Service (BNS) Contracts v2 - Beranames

The _validateWhitelist and _validateFreeWhitelist functions in the RegistrarController contract calculate the payload for signature validation without including all fields of the RegisterRequest struct. For example, fields such as resolver, data, and reverseRecord are not part of the payload used in signature validation.

This omission creates a potential front-running vulnerability, where an attacker could reuse a valid signature from one transaction and modify the omitted fields in another transaction. The contract would incorrectly validate the altered payload, allowing unauthorized changes.

Code Location

The _validateWhitelist and _validateFreeWhitelist functions calculate the payload without including all fields of the RegisterRequest struct:

    function _validateWhitelist(WhitelistRegisterRequest calldata request, bytes calldata signature) internal {
        (bytes32 r, bytes32 s, uint8 v) = unpackSignature(signature);

        // Encode payload - signature format: (address owner, address referrer, uint256 duration, string name)
        bytes memory payload = abi.encode(
            request.registerRequest.owner,
            request.registerRequest.referrer,
            request.registerRequest.duration,
            request.registerRequest.name,
            request.round_id,
            request.round_total_mint
        );

        if (usedSignatures[keccak256(payload)]) revert SignatureAlreadyUsed();
        if (mintsCountByRoundByAddress[msg.sender][request.round_id] >= request.round_total_mint) {
            revert MintLimitForRoundReached();
        }

        // Validate signature
        whitelistValidator.validateSignature(payload, v, r, s);

        // Store the message hash in used signatures and increment the mint count for the round
        usedSignatures[keccak256(payload)] = true;
        mintsCountByRoundByAddress[msg.sender][request.round_id]++;
    }

    function _validateFreeWhitelist(address owner, bytes calldata signature) internal {
        (bytes32 r, bytes32 s, uint8 v) = unpackSignature(signature);

        bytes memory payload = abi.encode(owner);
        if (usedFreeMintsSignatures[keccak256(payload)]) revert FreeMintSignatureAlreadyUsed();

        whitelistValidator.validateSignature(payload, v, r, s);

        usedFreeMintsSignatures[keccak256(payload)] = true;
    }

This PoC demonstrates a front-running vulnerability in the whitelistFreeRegister function of the RegistrarController contract. The vulnerability arises because the payload used for signature validation does not include critical fields of the RegisterRequest struct, such as name. As a result, an attacker can reuse a valid signature and alter unprotected fields to manipulate the registration process:

    function test_front_run_whitelist_free_register() public {
        // set launch time in 10 days
        vm.prank(registrarAdmin);
        registrar.setLaunchTime(block.timestamp + 10 days);
        vm.stopPrank();

        // mint with success
        vm.startPrank(alice);
        deal(address(alice), 1000 ether);

        RegistrarController.RegisterRequest memory request = RegistrarController.RegisterRequest({
            name: "correct-name",
            owner: alice,
            duration: 365 days,
            resolver: address(resolver),
            data: new bytes[](0),
            reverseRecord: false,
            referrer: address(0)
        });

        bytes memory payload = abi.encode(request.owner);
        bytes32 payloadHash = keccak256(payload);
        bytes memory prefix = "\x19Ethereum Signed Message:\n32";
        bytes32 prefixedHash = keccak256(abi.encodePacked(prefix, payloadHash));

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, prefixedHash);

        bytes memory signature = abi.encodePacked(r, s, v);

        // User calls registrar.whitelistFreeRegister(request, signature) but attacker will try to front-run the tx
        
        vm.stopPrank();
        
        // Attacker forges another request
        RegistrarController.RegisterRequest memory maliciousRequest = RegistrarController.RegisterRequest({
            name: "non-expected-name", // Attacker sets another name
            owner: request.owner,
            duration: request.duration,
            resolver: request.resolver,
            data: request.data,
            reverseRecord: request.reverseRecord,
            referrer: request.referrer
        });

        // Attacker front-runs the original tx using maliciousRequest
        address attacker = address(0xCAFE);
        vm.prank(attacker);
        registrar.whitelistFreeRegister(maliciousRequest, signature);

        // An incorrect name was assigned to Alice
        assertEq(baseRegistrar.ownerOf(uint256(keccak256(bytes(maliciousRequest.name)))), alice);

        // Original tx fails
        vm.expectRevert(abi.encodeWithSelector(RegistrarController.FreeMintSignatureAlreadyUsed.selector));
        registrar.whitelistFreeRegister(request, signature);
    }

The test was successfully executed:

Ensure that all fields in the RegisterRequest struct are included in the payload used for signature validation.

SOLVED: The Beranames team solved the issue in the specified commit id.

In the current implementation of the Registrar contract, the renew function calculates the required payment using price.base, ignoring any applicable discounts. Conversely, the _register function (used by register and whitelistRegister) calculates the required payment as price.base - price.discount, allowing registrants to benefit from discounts.

This creates an inconsistency where existing registrars renewing their domains do not receive the same discount that new registrants would obtain if the domain expires. This discrepancy disincentivizes existing users from renewing their domains, potentially leading to avoidable domain turnover. Furthermore, it allows new registrants to exploit the pricing difference by acquiring expired domains at a lower price than renewals.

Code Location

The renew function calculates the required payment using price.base, ignoring any applicable discounts:

    function renew(string calldata name, uint256 duration) external payable {
        bytes32 labelhash = keccak256(bytes(name));
        uint256 tokenId = uint256(labelhash);
        IPriceOracle.Price memory price = rentPrice(name, duration);

        _validatePayment(price.base);

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

        _refundExcessEth(price.base);

        emit NameRenewed(name, labelhash, expires);
    }

This PoC demonstrates the pricing inconsistency between the renew and register functions in the RegistrarController contract, highlighting how discounts are applied differently:

    function test_renew() public {
        // set launch time in 10 days
        vm.prank(registrarAdmin);
        registrar.setLaunchTime(block.timestamp);
        vm.stopPrank();

        // mint with success
        vm.startPrank(alice);
        deal(address(alice), 1000 ether);

        RegistrarController.RegisterRequest memory request = RegistrarController.RegisterRequest({
            name: "test-name",
            owner: alice,
            duration: 730 days,
            resolver: address(resolver),
            data: new bytes[](0),
            reverseRecord: false,
            referrer: address(0)
        });

        // Set the required ether amount (e.g., price from the registrar)
        uint256 registrationPrice = registrar.registerPrice(request.name, request.duration);
        registrar.register{value: registrationPrice}(request);
        assertEq(baseRegistrar.ownerOf(uint256(keccak256(bytes(request.name)))), alice);

        vm.expectRevert(abi.encodeWithSelector(RegistrarController.InsufficientValue.selector));
        registrar.renew{value: registrationPrice}(request.name, request.duration);
        
        // Discount was 2.5 BERA in registration, but not considered in renewal
        registrar.renew{value: registrationPrice + 2.5 ether}(request.name, request.duration);

        vm.stopPrank();
    }

The test was successfully executed:

Update the renew function to include the discount in the price calculation, aligning it with the pricing logic used in registration.

SOLVED: The Beranames team solved the issue in the specified commit id.

In the reservedRegister function of the RegistrarController contract, the _registerRequest function is used to process the registration. If the reverseRecord flag in the request is set to true, the _setReverseRecord function is called with msg.sender as the owner. Since msg.sender in this context is required to be the reservedNamesMinter, the reverse record for the registered name is incorrectly assigned to the reservedNamesMinter instead of the intended owner (request.owner).

This misalignment diminishes the utility of the reverse record, potentially causing confusion among users or applications relying on reverse records to identify the actual owner of a name. Furthermore, if reverse records are used for authentication or user attribution, this issue could lead to misattributing or exploitation.

Code Location

The _registerRequest function is always called with msg.sender as the owner:

    function _registerRequest(RegisterRequest calldata request) internal {
        uint256 expires = base.registerWithRecord(
            uint256(keccak256(bytes(request.name))), request.owner, request.duration, request.resolver, 0
        );

        if (request.data.length > 0) {
            _setRecords(request.resolver, keccak256(bytes(request.name)), request.data);
        }

        if (request.reverseRecord) {
            _setReverseRecord(request.name, request.resolver, msg.sender);
        }

        // two different events for ENS compatibility
        emit NameRegistered(request.name, keccak256(bytes(request.name)), request.owner, expires);
        if (request.referrer != address(0)) {
            emit NameRegisteredWithReferral(
                request.name, keccak256(bytes(request.name)), request.owner, request.referrer, expires
            );
        }
    }

This PoC demonstrates a flaw in the reservedRegister function. The test sets up a scenario where a reserved name is registered with reverseRecord set to true. During the registration process, the _registerRequest function assigns the reverse record's ownership to the reservedNamesMinter (minter in this case), rather than the actual owner (Alice):

    function test_name_reserved_with_reverse_record() public {
        address minter = makeAddr("minter");
    
        // add to reserved names
        vm.prank(deployer);
        reservedRegistry.setReservedName(DEFAULT_NAME);

        // set reserved names minter on registrar
        vm.prank(registrarAdmin);
        registrar.setReservedNamesMinter(minter);

        RegistrarController.RegisterRequest memory req = defaultRequest();
        assertEq(req.reverseRecord, true, "Expected reverseRecord to be true");
        assertEq(req.owner, alice, "Expected owner is Alice");
        
        vm.prank(minter);
        registrar.reservedRegister(req);

        bytes32 labelHash = sha3HexAddress(minter);
        bytes32 reverseNode = keccak256(abi.encodePacked(ADDR_REVERSE_NODE, labelHash));
        address owner = registry.owner(reverseNode);

        assertEq(owner, minter, "Owner for the reverse node is minter, but should be Alice");
    }

The test was successfully executed:

Update the _registerRequest function to ensure that the correct owner (request.owner) is passed to _setReverseRecord. Specifically, replace msg.sender with request.owner when calling _setReverseRecord within the _registerRequest function.

SOLVED: The Beranames team solved the issue in the specified commit id.

The unpackSignature function in the RegistrarController contract does not include critical validations present in OpenZeppelin's ECDSA implementation, including:

1. Verifying that the signature length is exactly 65 bytes. An invalid length could cause out-of-bounds memory access or incorrect data parsing.

2. Ensuring the v value is either 27 or 28, as required by Ethereum's signature standard.

3. Ensuring the s value is in the lower half of the secp256k1 curve order, preventing signature malleability attacks.

These omissions could allow the contract to accept malformed or malleable signatures, potentially leading to replay attacks, cryptographic failures, or other security vulnerabilities.

Code Location

The unpackSignature function does not include the critical validations present in OpenZeppelin's ECDSA implementation:

    function unpackSignature(bytes calldata signature) internal pure returns (bytes32 r, bytes32 s, uint8 v) {
        // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L66-L70
        assembly {
            r := calldataload(signature.offset)
            s := calldataload(add(signature.offset, 32))
            v := byte(0, calldataload(add(signature.offset, 64)))
        }

        return (r, s, v);
    }

It is recommended to use OpenZeppelin's ECDSA library to handle signature unpacking and validation.

SOLVED: The Beranames team solved the issue in the specified commit id.

The createBid function in the BeraAuctionHouse contract does not include the whenNotPaused modifier or any explicit check to prevent its execution while the contract is paused. As a result, users can still submit bids during a paused state, contrary to the expected behavior of pausing a contract.

This oversight can lead to operational inconsistencies, such as allowing unintended auction activity during periods where the contract owner might be attempting to address critical issues or make administrative changes. It undermines the expected functionality of the pause mechanism, potentially impacting the integrity of the auction system.

Code Location

The createBid function does not include the whenNotPaused modifier:

    function createBid(uint256 tokenId) external payable override {
        IBeraAuctionHouse.Auction memory _auction = auctionStorage;

        (uint192 _reservePrice, uint56 _timeBuffer, uint8 _minBidIncrementPercentage) =
            (reservePrice, timeBuffer, minBidIncrementPercentage);

        if (_auction.tokenId != tokenId) {
            revert TokenNotForUpAuction(tokenId);
        }

        if (block.timestamp >= _auction.endTime) {
            revert AuctionExpired();
        }

        if (msg.value < _reservePrice) {
            revert MustSendAtLeastReservePrice();
        }

        if (msg.value < _auction.amount + ((_auction.amount * _minBidIncrementPercentage) / 100)) {
            revert MustSendMoreThanLastBidByMinBidIncrementPercentageAmount();
        }
        auctionStorage.amount = uint128(msg.value);
        auctionStorage.bidder = payable(msg.sender);

        // Extend the auction if the bid was received within `timeBuffer` of the auction end time
        bool extended = _auction.endTime - block.timestamp < _timeBuffer;

        emit AuctionBid(_auction.tokenId, msg.sender, msg.value, extended);

        if (extended) {
            auctionStorage.endTime = _auction.endTime = uint40(block.timestamp + _timeBuffer);
            emit AuctionExtended(_auction.tokenId, _auction.endTime);
        }

        address payable lastBidder = _auction.bidder;

        // Refund the last bidder, if applicable
        if (lastBidder != address(0)) {
            _safeTransferETHWithFallback(lastBidder, _auction.amount);
        }
    }

Include the whenNotPaused modifier in the createBid function to ensure that bidding is disabled when the contract is paused.

SOLVED: The Beranames team solved the issue in the specified commit id.

The name received as input during registration or auction creation is not validated for normalization. This means that variations in case (e.g., "test" vs. "TEST"), whitespace, or Unicode representation can result in distinct entries for the same logical name. Without enforcing a standard format, visually similar names can coexist in the system.

This lack of normalization introduces risks such as:

• User confusion: Users might unintentionally register or bid on visually similar names, leading to potential misunderstandings or dissatisfaction.

• Scams and fraud: Malicious actors could exploit visually indistinguishable names (e.g., "teѕt" with a Cyrillic s) to impersonate legitimate users or mislead bidders, potentially defrauding them.

• Reputational damage: Allowing inconsistent handling of names could harm user trust and lead to complaints or disputes, impacting the platform's reputation.

• Operational complexity: Handling multiple names that appear identical but are treated as distinct increases the risk of inconsistencies and bugs in the system.

Code Location

The BeraAuctionHouse._createAuction and Registrar._registerRequest functions do not validate the normalization of the name before processing it:

    function _createAuction(string memory label_) internal {
        uint256 id = uint256(keccak256(abi.encodePacked(label_)));
        try base.registerWithRecord(id, address(this), registrationDuration, address(resolver), 0) returns (uint256) {
            uint40 startTime = uint40(block.timestamp);
            uint40 endTime = startTime + uint40(auctionDuration);

            auctionStorage = Auction({
                tokenId: id,
                amount: 0,
                startTime: startTime,
                endTime: endTime,
                bidder: payable(0),
                settled: false
            });

            emit AuctionCreated(id, startTime, endTime);
        } catch Error(string memory reason) {
            _pause();
            emit AuctionCreationError(reason);
        }
    }

    function _registerRequest(RegisterRequest calldata request) internal {
        uint256 expires = base.registerWithRecord(
            uint256(keccak256(bytes(request.name))), request.owner, request.duration, request.resolver, 0
        );

        if (request.data.length > 0) {
            _setRecords(request.resolver, keccak256(bytes(request.name)), request.data);
        }

        if (request.reverseRecord) {
            _setReverseRecord(request.name, request.resolver, msg.sender);
        }

        // two different events for ENS compatibility
        emit NameRegistered(request.name, keccak256(bytes(request.name)), request.owner, expires);
        if (request.referrer != address(0)) {
            emit NameRegisteredWithReferral(
                request.name, keccak256(bytes(request.name)), request.owner, request.referrer, expires
            );
        }
    }

Normalize all input names by converting them to a consistent format (e.g., lowercase, trimmed, and Unicode normalized) before processing. Reject any input that does not comply with the normalization rules.

RISK ACCEPTED: The Beranames team accepted this risk of this finding.

The reservePrice_ parameter in the BeraAuctionHouse contract's constructor is used to set the minimum acceptable bid for auctions. However, the constructor does not validate this parameter to ensure it is non-zero or meets a practical threshold. If reservePrice_ is set to zero or an extremely low value, it could lead to unintended consequences, such as auctions starting with inadequate bids or devaluing the items being auctioned.

Additionally, the setReservePrice function allows the reserve price to be updated during the contract's operation without any validation, which could inadvertently set the value to zero or another impractical amount. This lack of safeguards could compromise the auction's integrity and misalign with the intended functionality of the contract.

Code Location

The constructor does not validate whether reservePrice_ is non-zero or meets a reasonable minimum threshold:

    constructor(
        BaseRegistrar base_,
        BeraDefaultResolver resolver_,
        IWETH weth_,
        uint256 auctionDuration_,
        uint256 registrationDuration_,
        uint192 reservePrice_,
        uint56 timeBuffer_,
        uint8 minBidIncrementPercentage_,
        address paymentReceiver_
    ) Ownable(msg.sender) {
        base = base_;
        resolver = resolver_;
        weth = weth_;
        auctionDuration = auctionDuration_;
        registrationDuration = registrationDuration_;
        paymentReceiver = paymentReceiver_;

        _pause();

        reservePrice = reservePrice_;
        timeBuffer = timeBuffer_;
        minBidIncrementPercentage = minBidIncrementPercentage_;
    }

The setReservePrice function does not validate whether reservePrice_ is non-zero or meets a reasonable minimum threshold:

    function setReservePrice(uint192 _reservePrice) external override onlyOwner {
        reservePrice = _reservePrice;

        emit AuctionReservePriceUpdated(_reservePrice);
    }

Implement a check to ensure reservePrice_ is non-zero and reasonable during contract deployment. Also, add similar validation to the setReservePrice function to prevent inappropriate updates.

SOLVED: The Beranames team solved the issue in the specified commit id.

The minBidIncrementPercentage parameter in the BeraAuctionHouse contract defines the minimum percentage increase for new bids. Although the setMinBidIncrementPercentage function includes a check to prevent setting this value to zero, there is no validation to ensure the parameter is within a reasonable range. If the value is set too low, it could lead to auction inefficiencies by encouraging small incremental bids, which may not be economically meaningful. Conversely, if the value is set too high, it could discourage participation by requiring bidders to place unreasonably large increments.

Additionally, the parameter is mutable, meaning it can be updated at any time. Without proper safeguards, this could lead to unexpected behavior if the value is set to an impractical level during the contract's operation.

Code Location

The constructor does not validate whether minBidIncrementPercentage falls within a reasonable range:

    constructor(
        BaseRegistrar base_,
        BeraDefaultResolver resolver_,
        IWETH weth_,
        uint256 auctionDuration_,
        uint256 registrationDuration_,
        uint192 reservePrice_,
        uint56 timeBuffer_,
        uint8 minBidIncrementPercentage_,
        address paymentReceiver_
    ) Ownable(msg.sender) {
        base = base_;
        resolver = resolver_;
        weth = weth_;
        auctionDuration = auctionDuration_;
        registrationDuration = registrationDuration_;
        paymentReceiver = paymentReceiver_;

        _pause();

        reservePrice = reservePrice_;
        timeBuffer = timeBuffer_;
        minBidIncrementPercentage = minBidIncrementPercentage_;
    }

The setMinBidIncrementPercentage function does not validate whether minBidIncrementPercentage falls within a reasonable range:

    function setMinBidIncrementPercentage(uint8 _minBidIncrementPercentage) external override onlyOwner {
        if (_minBidIncrementPercentage == 0) {
            revert MinBidIncrementPercentageIsZero();
        }

        minBidIncrementPercentage = _minBidIncrementPercentage;

        emit AuctionMinBidIncrementPercentageUpdated(_minBidIncrementPercentage);
    }

Enforce an acceptable range for minBidIncrementPercentage in both the constructor and the setMinBidIncrementPercentage function. A range between 5% and 10% is common in most auction systems.

RISK ACCEPTED: The Beranames team updated the constructor code to validate that minBidIncrementPercentage_ is not zero. This change was introduced in commit e0e6ff2. However, they chose to accept the remaining risk as they preferred not to impose an upper limit on minBidIncrementPercentage_.

The strlen function in the StringUtils library relies on _charLength to determine the byte length of each character. However, _charLength does not adequately validate whether the bytes conform to the UTF-8 standard. As a result, malformed UTF-8 sequences can cause strlen to misinterpret the input, leading to an incorrect character count. Specifically:

• Malformed UTF-8 strings may result in either over-counting or under-counting characters.

• This behavior could allow malicious users to bypass logic dependent on accurate character counts, such as pricing models, validations, or input restrictions.

For example:

• A string containing invalid UTF-8 bytes like \xE2\x28\xA1 might be misinterpreted as fewer characters than its actual length.

• Conversely, certain malformed sequences could appear as additional characters, affecting validations or cost calculations.

Validate input strings to ensure they conform to UTF-8 encoding before processing. This can be achieved by enhancing the _charLength function to detect and reject invalid sequences explicitly. For example:

function _charLength(bytes memory strBytes, uint256 index) private pure returns (uint256) {
    uint8 b = uint8(strBytes[index]);

    if (b < 0x80) {
        return 1; // 1-byte character (ASCII)
    } else if (b < 0xE0 && index + 1 < strBytes.length && uint8(strBytes[index + 1]) & 0xC0 == 0x80) {
        return 2; // 2-byte character
    } else if (b < 0xF0 && index + 2 < strBytes.length && uint8(strBytes[index + 1]) & 0xC0 == 0x80 && uint8(strBytes[index + 2]) & 0xC0 == 0x80) {
        return 3; // 3-byte character
    } else if (b < 0xF8 && index + 3 < strBytes.length && uint8(strBytes[index + 1]) & 0xC0 == 0x80 && uint8(strBytes[index + 2]) & 0xC0 == 0x80 && uint8(strBytes[index + 3]) & 0xC0 == 0x80) {
        return 4; // 4-byte character (including emojis)
    } else {
        revert("Invalid UTF-8 sequence"); // Revert on invalid sequences
    }
}

SOLVED: The Beranames team solved the issue in the specified commit id.

In the RegistrarController contract, specifically within the setLaunchTime function, the launchTime_ parameter is updated directly without validation against the current block timestamp. This absence of a check means that the launchTime_ can be mistakenly set to a past timestamp, which could lead to inconsistencies or vulnerabilities related to event timing within the contract. Proper time setting is crucial, especially for functions that rely on specific timeframes to execute correctly, such as name registering.

Code Location

The setLaunchTime function does not validate that launchTime_ is greater than or equal to block.timestamp:

    function setLaunchTime(uint256 launchTime_) external onlyOwner {
        launchTime = launchTime_;
        emit LaunchTimeUpdated(launchTime_);
    }

It is recommended to ensure that the new launch time is not set in the past. Implementing a simple conditional check to compare launchTime_ with block.timestamp will prevent the setting of an incorrect launch time.

SOLVED: The Beranames team solved the issue in the specified commit id.

The constructor of the BeraAuctionHouse contract directly assigns the timeBuffer_ parameter to the timeBuffer state variable without validating that it adheres to the defined upper bound of MAX_TIME_BUFFER. This omission could allow a deployment with an invalid timeBuffer value that exceeds MAX_TIME_BUFFER.

Such a misconfiguration would undermine the auction process by allowing excessively long extensions for bids placed close to the end time. It could also lead to unintended behaviors or inefficiencies in the auction system.

Code Location

The constructor does not validate that timeBuffer_ is lower than or equal to MAX_TIME_BUFFER:

    constructor(
        BaseRegistrar base_,
        BeraDefaultResolver resolver_,
        IWETH weth_,
        uint256 auctionDuration_,
        uint256 registrationDuration_,
        uint192 reservePrice_,
        uint56 timeBuffer_,
        uint8 minBidIncrementPercentage_,
        address paymentReceiver_
    ) Ownable(msg.sender) {
        base = base_;
        resolver = resolver_;
        weth = weth_;
        auctionDuration = auctionDuration_;
        registrationDuration = registrationDuration_;
        paymentReceiver = paymentReceiver_;

        _pause();

        reservePrice = reservePrice_;
        timeBuffer = timeBuffer_;
        minBidIncrementPercentage = minBidIncrementPercentage_;
    }

It is recommended to add a check in the constructor to ensure timeBuffer_ does not exceed MAX_TIME_BUFFER.

SOLVED: The Beranames team solved the issue in the specified commit id.

The ethPriceToUint64 function in the BeraAuctionHouse contract converts a BERA price of 256 bits with 18 decimals to a 64-bit value with 10 decimals. However, this function lacks validation to ensure that the result of the division (ethPrice / 1e8) fits within the range of a uint64 (maximum value of 18446744073709551615).

If the division result exceeds the maximum value of uint64, Solidity does not revert. Instead, it silently truncates the value to fit within the uint64 range, potentially leading to incorrect data storage and calculations. For example:

uint256 ethPrice = 18446744073709551616000000000; // A large BERA price
uint64 result = uint64(ethPrice / 1e8); // This silently truncates the value to fit within uint64

This can result in incorrect values, which may have downstream effects, such as incorrect pricing or settlement logic in the protocol.

Code Location

The ethPriceToUint64 functionconverts a BERA price of 256 bits with 18 decimals to a 64-bit value with 10 decimals:

    /**
     * @dev Convert an ETH price of 256 bits with 18 decimals, to 64 bits with 10 decimals.
     * Max supported value is 1844674407.3709551615 ETH.
     *
     */
    function ethPriceToUint64(uint256 ethPrice) internal pure returns (uint64) {
        return uint64(ethPrice / 1e8);
    }

Add a validation check before the type conversion to ensure that the division result fits within the range of a uint64. For example:

require(ethPrice / 1e8 <= type(uint64).max, "Value exceeds uint64 range");

SOLVED: The Beranames team solved the issue in the specified commit id.

The current implementation of the removeReservedName function in the ReservedRegistry contract swaps the element to be removed with the last one in the _reservedNamesList, but it does not properly shrink the list afterward. The list continues to grow with each removal, leading to inefficiencies and an inaccurate count of reserved names. This can cause increasing gas usage over time and discrepancies in the contract's internal state.

Code Location

The removeReservedName function does not pop the last element:

function removeReservedName(uint256 index_) public onlyOwner {
  bytes32 labelHash_ = _reservedNamesList[index_];
  delete _reservedNames[labelHash_];
  _reservedNamesList[index_] = _reservedNamesList[_reservedNamesCount - 1];
  _reservedNamesCount--;
}

Use the pop() method after performing the swap to ensure the list size is reduced and remains accurate. This will improve gas efficiency and maintain the integrity of the reserved names list.

SOLVED: The Beranames team solved the issue in the specified commit id.

The current implementation logic of the setReservedName function in the ReservedRegistry contract allows for duplicate entries in the list of reserved names. If the same name is mistakenly reserved multiple times, it results in redundant entries in the internal list _reservedNamesList and unnecessary increments of the counter _reservedNamesCount. This can result in inefficient gas usage and an inaccurate count of reserved names, potentially impacting the performance and operational cost of the contract.

Code Location

The setReservedName function does not check if the name has already been reserved before adding it to the list and incrementing the count:

function setReservedName(string calldata name_) public onlyOwner {
  bytes32 labelHash_ = keccak256(abi.encodePacked(name_));
  _reservedNames[labelHash_] = name_;
  _reservedNamesList.push(labelHash_);
  _reservedNamesCount++;
}

Introduce a check in the setReservedName function to verify if the name has already been reserved before adding it to the list and incrementing the count. This would prevent the redundant updates to the state and improve gas efficiency.

SOLVED: The Beranames team solved the issue in the specified commit id.

The hexStringToBytes32 function in the HexUtils library does not properly validate the idx and lastIdx input indices. While it ensures that lastIdx does not exceed the string's length, it neglects to verify that idx is within bounds and that lastIdx is greater than idx. This insufficient validation can result in out-of-bounds memory access, leading to reverts or undefined behavior.

The hexStringToBytes32 function should implement comprehensive checks to validate input indices. It must ensure that idx is less than the string's length, lastIdx does not exceed the string's length, and idx is strictly less than lastIdx. These validations should be enforced before processing the input to prevent any potential memory errors and guarantee reliable execution.

SOLVED: The Beranames team solved the issue in the specified commit id.

The hexToAddress function in the HexUtils library checks that the substring defined by idx and lastIdx is at least 40 characters long but does not validate whether it is exactly 40 characters. On Berachain, addresses also require a precise length of 40 hexadecimal characters, and allowing longer substrings can result in incorrect parsing or invalid address generation.

The hexToAddress function should enforce a strict validation to ensure that the substring between idx and lastIdx is exactly 40 characters long before proceeding.

SOLVED: The Beranames team solved the issue in the specified commit id.

In multiple functions across the codebase, the for loops use i++ (post-increment) for incrementing the loop counter. In Solidity, post-increment (i++) is slightly less efficient than pre-increment (++i) because i++ requires storing the original value of i in a temporary variable before incrementing, which consumes more gas. Although the gas difference is minimal, especially in recent Solidity versions, it becomes noticeable in larger loops or frequently executed functions, leading to inefficiencies in contract execution. The affected functions are the following:

• Multicallable._multicall

• UniversalResolver._resolveCallback

• UniversalResolver._multicall

To optimize gas usage, especially when iterating over large arrays or loops, it is recommendd to replace i++ with ++i. Pre-increment (++i) does not require storing the old value of i, making it slightly more efficient in terms of gas consumption.

SOLVED: The Beranames team solved the issue in the specified commit id.

The codebase contains instances of keccak256(bytes(name)) and keccak256(abi.encodePacked(name)) for calculating the hash of the UTF-8 string name. While these two methods currently produce the same result for UTF-8 strings because bytes(name) and abi.encodePacked(name) are equivalent in such cases, their inconsistent usage introduces potential future risks.

If the logic in the codebase changes to include additional parameters or complex data structures in the hash calculation, the semantics of abi.encodePacked differ significantly from bytes. This could result in unintended behavior or hashing discrepancies. Furthermore, inconsistent usage can reduce code readability, leading to confusion among developers.

Standardize the use of keccak256(abi.encodePacked(name)) throughout the codebase. This approach ensures uniformity and aligns with Solidity's best practices, especially when dealing with dynamic or concatenated data.

SOLVED: The Beranames team solved the issue in the specified commit id.

The getSettlements(uint256, uint256), getSettlements(uint256), and getSettlementsFromIdtoTimestamp functions in the BeraAuctionHouse contract all pre-allocate arrays with a fixed size, which may lead to unused slots if some token IDs or settlements do not meet the inclusion criteria. This inefficient allocation results in unnecessary gas consumption and wasted memory, particularly for large ranges with sparse or invalid data. While trimming the array with inline assembly reduces the size afterward, the initial over-allocation remains inefficient.

Adopt dynamic array allocation across all affected getSettlements functions. Instead of pre-allocating arrays with a maximum size, dynamically append valid data to the array using the push() function. This approach ensures efficient memory usage without the need for post-trimming.

SOLVED: The Beranames team solved the issue in the specified commit id.

The implementation of settlement retrieval functions in the BeraAuctionHouse contract, such as getSettlements(uint256), getSettlementsFromIdtoTimestamp and getSettlements(uint256, uint256), does not filter out uninitialized settlement entries.

Specifically, if settlementState.blockTimestamp or other fields remain uninitialized (e.g., defaulting to zero), these entries may be included in the returned results. This can occur when querying settlement data where auctions were not successfully settled or processed. Including such invalid data can result in misleading outputs and unreliable analytics, potentially impacting decision-making or user experience.

Introduce validation checks in the settlement retrieval functions to skip entries with uninitialized or zero values. For example, ensure that entries are included only if settlementState.blockTimestamp > 0 or if other critical fields indicate that the settlement is valid. This validation ensures the returned data is accurate and meaningful.

SOLVED: The Beranames team solved the issue in the specified commit id.

The getSettlements(uint256) and getPrices functions in the BeraAuctionHouse contract do not properly handle cases where latestTokenId is 0 and the last auction is not settled. When latestTokenId equals 0, the functions will include it in the return value regardless of whether the auction has been settled. This behavior can lead to the inclusion of incomplete or invalid settlement data, which might mislead users or systems relying on this function for accurate historical auction information.

Add a validation check to exclude the latest token ID if it corresponds to an unsettled auction. Specifically, ensure that the functions skip the token if auctionStorage.settled is false or if other indicators show that the auction has not been finalized. This adjustment ensures that only settled and valid auction data is included in the results.

SOLVED: The Beranames team solved the issue in the specified commit id.

In the getPrices function of the BeraAuctionHouse contract, there is a contradictory logic when dealing with uninitialized settlement data. Specifically, if settlementState.blockTimestamp == 0, the function reverts with a MissingSettlementsData error. However, if settlementState.winner == address(0), the function skips the iteration using continue. In practice, uninitialized data is likely to have both blockTimestamp == 0 and winner == address(0), which results in inconsistent handling: some cases halt execution while others are silently ignored. This inconsistency can lead to confusion and unintended behavior, particularly when the contract processes settlement history.

Unify the handling of uninitialized or invalid auction data to avoid contradictory behavior. If encountering uninitialized data is a critical issue, revert execution for both conditions. If skipping such entries is acceptable, handle both conditions with a single continue statement. For example:

if (settlementState.blockTimestamp == 0 || settlementState.winner == address(0)) {
    continue; // Skip uninitialized or no-bid settlements
}

SOLVED: The Beranames team solved the issue in the specified commit id.

The readReturnData function in the LowLevelCallUtils library lacks proper validation of the offset and length parameters. If these parameters are crafted to exceed the available returndatasize(), the function will revert during the returndatacopy operation. This lack of validation does not follow best practices for handling external return data and can lead to disruptions in the resolution process within the UniversalResolver contract.

To ensure robustness and adhere to best practices, implement validation for the offset and length parameters in readReturnData. This will ensure that the parameters fall within the bounds of returndatasize() and prevent unintended reverts. For example:

function readReturnData(uint256 offset, uint256 length) internal pure returns (bytes memory data) {
    require(offset + length <= returndatasize(), "Invalid offset or length");
    data = new bytes(length);
    assembly {
        returndatacopy(add(data, 32), offset, length)
    }
}

SOLVED: The Beranames team solved the issue in the specified commit id.

The approve function in the BeraDefaultResolver contract allows callers to set delegate approvals for specific nodes without validating ownership against the BNS registry. While the isAuthorised function ensures that only the legitimate owner or their authorized operators can execute actions protected by the authorised modifier, the lack of ownership validation in approve could lead to unnecessary and potentially confusing state updates in _tokenApprovals. This indirect behavior might create ambiguity and affect the clarity of the approval process.

Code Location

The approve function allows callers to set delegate approvals without validating ownership:

    function approve(bytes32 node, address delegate, bool approved) external {
        if (msg.sender == delegate) revert CantSetSelf();

        _tokenApprovals[msg.sender][node][delegate] = approved;
        emit Approved(msg.sender, node, delegate, approved);
    }

The approve function should explicitly validate that the caller is the owner of the specified node using bns.owner(node) before modifying the _tokenApprovals mapping. This additional validation ensures that only legitimate owners can set or revoke delegate permissions, reducing unnecessary state changes and aligning the approval logic with the ownership-based access control enforced by isAuthorised.

SOLVED: The Beranames team solved the issue in the specified commit id.

The _multicall function in the UniversalResolver contract processes multiple arrays, such as multicallData.data and multicallData.failures, assuming their lengths are aligned. However, the function does not enforce strict equality between these arrays' lengths. If a mismatch occurs, the loop in _multicall may process data incorrectly or leave some entries unprocessed.

Code Location

The _multicall function does not enforce strict equality between the arrays' lengths:

    function _multicall(MulticallData memory multicallData) internal view returns (bytes[] memory results) {
        uint256 length = multicallData.data.length;
        uint256 offchainCount = 0;
        OffchainLookupCallData[] memory callDatas = new OffchainLookupCallData[](length);
        OffchainLookupExtraData[] memory extraDatas = new OffchainLookupExtraData[](length);
        results = new bytes[](length);
        bool isCallback = multicallData.name.length == 0;
        bool hasExtendedResolver = _hasExtendedResolver(multicallData.resolver);

        if (multicallData.isWildcard && !hasExtendedResolver) {
            revert ResolverWildcardNotSupported();
        }

        for (uint256 i = 0; i < length; i++) {
            bytes memory item = multicallData.data[i];
            bool failure = multicallData.failures[i];
            if (failure) {
                results[i] = item;
                continue;
            }

Ensure that all related arrays, including multicallData.data and multicallData.failures, have strictly equal lengths before processing.

SOLVED: The Beranames team solved the issue in the specified commit id.

Reentrancy attacks are a critical security concern in smart contracts, especially when functions that alter contract state or transfer funds can be re-entered before they complete. These vulnerabilities can lead to fund theft or corrupted states. In the cases outlined below, specific functions within the contracts lack safeguards against such reentrancy, potentially allowing attackers to drain resources or manipulate contract states to their advantage:

• BeraAuctionHouse.createBid: This function updates the auction state (e.g., auctionStorage.amount, auctionStorage.bidder) before transferring funds to the previous bidder. However, this flow still exposes a potential reentrancy risk. The external transfer to the previous bidder could invoke their fallback function, allowing them to re-enter the contract and call createBid again. If this happens, the auction's state has already been updated, but the contract may still allow another bid with the same funds, potentially leading to multiple bids being placed with the same amount.

  
  

• BeraAuctionHouse._settleAuction: In this function, the ERC721 token is transferred using base.transferFrom(address(this), _auction.bidder, _auction.tokenId), followed by the payment transfer to paymentReceiver using _safeTransferETHWithFallback. This sequence opens up a potential reentrancy risk because the transfer of the ERC721 token could trigger hooks or malicious fallback logic on the receiver's side. If paymentReceiver is a contract with a reentrant fallback function, it could exploit the auction state during the ETH transfer. Additionally, while auctionStorage.settled = true mitigates some risk, the state transition alone is insufficient without a reentrancy guard, as external interactions during token transfers are inherently unsafe.
  

• Registrar._refundExcessEth: This function is used as the last operation in both renew() and _register() to refund excess BERA back to msg.sender. While this sequencing minimizes risk by ensuring all state changes are completed before the refund, an attacker with a malicious fallback function could potentially re-enter the contract during the refund operation. Although no sensitive state is modified after the refund in the current implementation, the absence of a reentrancy guard leaves the function theoretically susceptible to recursive interactions.

By implementing a reentrancy guard, such as OpenZeppelin's ReentrancyGuard, which uses a nonReentrant modifier, the contracts can protect against this class of attacks by preventing recursive calls during the execution of sensitive functions.

It is recommended to implement ReentrancyGuard from OpenZeppelin’s library by adding the nonReentrant modifier to the mentioned functions to prevent recursive calls.

SOLVED: The Beranames team solved the issue in commit cf8a9d7 and 1029be2.

The reclaim function in the BaseRegistrar contract allows a user to transfer the registry ownership of a node to another address without changing the NFT ownership. This functionality enables users to update the registry owner to a different address, even if it is not the current NFT owner. While this behavior is intentional, it might not always be advisable, as transferring registry ownership to another party effectively relinquishes control over the node.

Additionally, the transferFrom method in the contract ensures that registry ownership is automatically updated when the NFT is transferred. This design prevents discrepancies in typical workflows where NFT ownership and registry ownership would naturally align. However, using the reclaim function to assign registry ownership to a separate address could still create scenarios where the NFT owner and registry owner differ, leading to potential inconsistencies or unintended loss of control.

To address this divergence, consider either removing the functionality that allows registry ownership updates without transferring the associated NFT, or modifying the implementation so that any change to the registry ownership automatically triggers the transfer of the NFT to maintain consistency.

SOLVED: The Beranames team solved the issue in the specified commit id.

The hexStringToBytes32 function in the HexUtils library does not validate whether the substring defined by idx and lastIdx contains an even number of characters, which is essential for accurate hexadecimal parsing. When the range results in an odd-length substring, the function may produce incorrect or incomplete results, potentially impacting downstream processes that rely on the parsed output.

The hexStringToBytes32 function should include a validation step to ensure that the length of the substring between idx and lastIdx is even before performing any parsing operations.

SOLVED: The Beranames team solved the issue in the specified commit id.

The _safeTransferETHWithFallback function in the BeraAuctionHouse contract emits an event indirectly when the fallback mechanism is triggered via WETH (weth.deposit and weth.transfer). However, no event is emitted when the direct ETH transfer through _safeTransferETH is successful. This inconsistency in logging could create gaps in tracking ETH transfers, making it challenging to audit or monitor transfers that do not involve the fallback.

Emit an event regardless of whether _safeTransferETH succeeds or the fallback is used. This ensures complete transparency and consistency in logging ETH transfers.

SOLVED: The Beranames team solved the issue in the specified commit id.

Some functions in the codebase do not include a zero address check for their parameters. If one of those parameters is mistakenly set to zero, it could affect the correct operation of the protocol. The affected functions are the following:

• BeraAuctionHouse.constructor

• BaseRegistrar.constructor

• PriceOracle.constructor

• PriceOracle.setPyth

• WhitelistValidator.constructor

• WhitelistValidator.setWhitelistAuthorizer

• Registrar.constructor

• Registar.setPriceOracle

• Registar.setReverseRegistrar

• ReverseRegistrar.constructor

• Resolver.constructor

• Resolver.setRegistrarController

• Resolver.setReverseRegistrar

• UniversalResolver.constructor

It is recommended to add a zero address check in the functions mentioned above.

SOLVED: The Beranames team solved the issue in commit a0bd599, 29a4984 and cc12c26.

In the createBid function of the BeraAuctionHouse contract, msg.value (a uint256) is cast to uint128 using the line auctionStorage.amount = uint128(msg.value);. While uint128 can represent large values (up to approximately 3.4 × 10^38), this cast introduces a hard limit on the maximum allowable bid. If the contract were to accept bid amounts greater than 2^128 - 1 (the maximum value of uint128), the excess value would be truncated.

This truncation does not cause a revert and can lead to significant issues. The bid amount would be incorrectly recorded, potentially resulting in a loss of funds or an inaccurate auction state. For example, if a bidder sends an exceptionally large bid, the excess BERA would be discarded, leading to an incomplete or erroneous representation of the bid amount in the auction state.

It is recommended to keep msg.value as a uint256 to ensure that all possible bid amounts, including those that may exceed the uint128 limit, are accurately captured. Only cast to uint128 if you are certain that the bid amounts will always remain within the uint128 limit.

SOLVED: The Beranames team solved the issue in the specified commit id.

The getSettlements(uint256, uint256) function in the BeraAuctionHouse contract does not validate whether the startId is less than or equal to the endId. If an invalid range (e.g., startId > endId) is provided, the function will still execute and iterate unnecessarily, resulting in wasted gas and potential unintended behavior. This could also create confusion for users expecting a logical result when querying settlements.

Code Location

The getSettlements function does not validate whether the startId is less than or equal to the endId

    function getSettlements(uint256 startId, uint256 endId) external view returns (Settlement[] memory settlements) {
        settlements = new Settlement[](endId - startId);
        uint256 actualCount = 0;

        SettlementState memory settlementState;
        for (uint256 id = startId; id < endId; ++id) {
            settlementState = settlementHistory[id];

            settlements[actualCount] = Settlement({
                blockTimestamp: settlementState.blockTimestamp,
                amount: uint64PriceToUint256(settlementState.amount),
                winner: settlementState.winner,
                tokenId: id
            });
            ++actualCount;
        }

        if (settlements.length > actualCount) {
            // this assembly trims the settlements array, getting rid of unused cells
            assembly {
                mstore(settlements, actualCount)
            }
        }
    }

Add an explicit check to validate the input parameters at the start of the function. This ensures that the function operates on a valid range, optimizing gas usage and preventing logical errors.

SOLVED: The Beranames team solved the issue in the specified commit id.

The settlement retrieval functions getSettlements(uint256, uint256), getSettlements(uint256), and getSettlementsFromIdtoTimestamp from the BeraAuctionHouse contract lack proper input range validation.

Without enforcing reasonable limits on the number of settlements retrieved, these functions allow excessively large queries, which can:

• Consume excessive gas, leading to transaction failures.

• Exceed block gas limits, effectively preventing legitimate use of the contract.

• Cause memory overflows and performance degradation due to inefficient handling of unbounded arrays.

It is recommended to introduce reasonable caps, such as MAX_RANGE or MAX_COUNT, to limit the number of entries retrieved in a single call.

SOLVED: The Beranames team solved the issue in the specified commit id.

The namehash function in the BytesUtils library relies on recursion to compute the hash of a DNS-encoded name. When provided with a name containing a large number of labels, the recursion depth can grow excessively, leading to potential stack overflow or out-of-gas errors. This can disrupt the function's execution, especially when processing inputs from untrusted sources or handling edge cases.

The namehash function should be refactored to use an iterative approach for calculating the hash of DNS-encoded names. This ensures the function can handle inputs with a large number of labels efficiently and without the risk of exceeding stack limits.

SOLVED: The Beranames team solved the issue in the specified commit id.

The findResolver(bytes, uint256) function in the UniversalResolver contract lacks comprehensive bounds validation for the offset and labelLength parameters. These parameters are used to index and slice the name array, but there is no explicit check ensuring that both offset and offset + labelLength + 1 remain within the array's bounds. This oversight can lead to out-of-bounds memory access, causing the function to revert unexpectedly when handling crafted or invalid inputs.

Code Location

The findResolver function lacks comprehensive bounds validation for the offset and labelLength parameters:

    function findResolver(bytes calldata name, uint256 offset) internal view returns (address, bytes32, uint256) {
        uint256 labelLength = uint256(uint8(name[offset]));
        if (labelLength == 0) {
            return (address(0), bytes32(0), offset);
        }
        uint256 nextLabel = offset + labelLength + 1;
        bytes32 labelHash;
        if (
            // 0x5b == '['
            // 0x5d == ']'
            labelLength == 66 && name[offset + 1] == 0x5b && name[nextLabel - 1] == 0x5d
        ) {
            // Encrypted label
            (labelHash,) = bytes(name[offset + 2:nextLabel - 1]).hexStringToBytes32(0, 64);
        } else {
            labelHash = keccak256(name[offset + 1:nextLabel]);
        }
        (address parentresolver, bytes32 parentnode, uint256 parentoffset) = findResolver(name, nextLabel);
        bytes32 node = keccak256(abi.encodePacked(parentnode, labelHash));
        address resolver = registry.resolver(node);
        if (resolver != address(0)) {
            return (resolver, node, offset);
        }
        return (parentresolver, node, parentoffset);
    }

Add a validation step at the beginning of the findResolver function to ensure that offset is within the array's bounds and that the calculated range offset + labelLength + 1 does not exceed the length of the name array.

SOLVED: The Beranames team solved the issue in the specified commit id.

The findResolver(bytes, uint256) function in the UniversalResolver contract processes encrypted labels by identifying specific conditions, such as the presence of square brackets ([ ]) and a label length of 66 bytes. It then attempts to decode the encrypted portion of the label as a hex-encoded string using hexStringToBytes32. However, the function does not validate whether the hex-encoded data is well-formed or of the correct length before decoding. This can lead to unexpected reverts or incorrect calculations when the input data is malformed or does not conform to the expected structure.

Code Location

The findResolver function does not validate whether the hex-encoded data is well-formed or of the correct length before decoding:

    function findResolver(bytes calldata name, uint256 offset) internal view returns (address, bytes32, uint256) {
        uint256 labelLength = uint256(uint8(name[offset]));
        if (labelLength == 0) {
            return (address(0), bytes32(0), offset);
        }
        uint256 nextLabel = offset + labelLength + 1;
        bytes32 labelHash;
        if (
            // 0x5b == '['
            // 0x5d == ']'
            labelLength == 66 && name[offset + 1] == 0x5b && name[nextLabel - 1] == 0x5d
        ) {
            // Encrypted label
            (labelHash,) = bytes(name[offset + 2:nextLabel - 1]).hexStringToBytes32(0, 64);
        } else {
            labelHash = keccak256(name[offset + 1:nextLabel]);
        }
        (address parentresolver, bytes32 parentnode, uint256 parentoffset) = findResolver(name, nextLabel);
        bytes32 node = keccak256(abi.encodePacked(parentnode, labelHash));
        address resolver = registry.resolver(node);
        if (resolver != address(0)) {
            return (resolver, node, offset);
        }
        return (parentresolver, node, parentoffset);
    }

Introduce explicit validations to ensure that the encrypted portion of the label meets the expected format before decoding it. Verify that the hex-encoded string contains only valid hexadecimal characters and that its length matches the required 64 characters (32 bytes).

SOLVED: The Beranames team solved the issue in the specified commit id.

The register and registerOnly functions in the BaseRegistrar contract both invoke the _register function, which is restricted by the onlyController modifier. However, no contract or address with onlyController privileges is calling these functions, rendering them effectively unused in the current implementation.

The presence of unused functions increases the complexity of the contract and the potential attack surface, even if the functions are restricted. Additionally, these functions occupy space in the deployed bytecode, resulting in slightly higher deployment costs without contributing to the contract's functionality.

While these functions might have been added for future use or integration, their lack of utilization in the current implementation raises questions about their necessity.

If the mentioned functions are not required, consider removing them to simplify the contract and minimize its size and complexity.

SOLVED: The Beranames team solved the issue in the specified commit id.

Some contracts contain redundant checks in their codebase that do not add functional value, potentially increasing gas costs and reducing code clarity:

1. Ownership initialization in BaseRegistrar.constructor: Both Ownable(owner_) and _transferOwnership(owner_) are used in the constructor to set ownership, but they effectively perform the same task. This duplication is unnecessary since the Ownable constructor already sets the initial owner.

2. Authorization in BaseRegistrar._isApprovedOrOwner: The function performs two checks: comparing owner_ == spender and calling _isAuthorized. However, _isAuthorized itself includes a check for owner == spender, making the first comparison redundant.

3. Ownership initialization in ReservedRegistry.constructor: Both Ownable(owner_) and _transferOwnership(owner_) are used in the constructor to set ownership, but they effectively perform the same task. This duplication is unnecessary since the Ownable constructor already sets the initial owner.

4. Ownership initialization in WhitelistValidator.constructor: Both Ownable(owner_) and _transferOwnership(owner_) are used in the constructor to set ownership, but they effectively perform the same task. This duplication is unnecessary since the Ownable constructor already sets the initial owner.

These redundancies do not impact security but increase gas usage and code complexity

It is recommended to refactor the contract to eliminate redundant logic by consolidating checks and operations where appropriate.

SOLVED: The Beranames team solved the issue in the commit 94d98d1 and e10b4e3.

In the BaseRegistrar contract, the transferFrom function does not include the live modifier, which ensures that the contract is actively managing a registration for its baseNode. While this may seem like an oversight, the NFT tokens in this contract can only be minted through registerWithRecord function, which already requires the live modifier. As a result, tokens cannot exist unless the contract is in a live state, reducing the likelihood of issues arising from the omission of the live modifier in transferFrom.

However, without the live modifier in transferFrom, there is no explicit guarantee that token transfers are restricted to periods when the contract is managing the baseNode, which may lead to edge cases if the contract's state changes unexpectedly after tokens have been minted.

It is recommended to add the live modifier to the transferFrom function to ensure that the contract is actively managing a registration for its baseNode before allowing transfers.

SOLVED: The Beranames team solved the issue in the specified commit id.

The _multicall function in the Multicallable contract uses delegatecall to execute provided calldata. When the delegatecall fails, the function reverts without returning the error message or reason for the failure. This limits visibility into the cause of the issue, making debugging and troubleshooting challenging for developers and users.

Code Location

The _multicall function could revert without returning the error message or reason for the failure:

    function _multicall(bytes32 nodehash, bytes[] calldata data) internal returns (bytes[] memory results) {
        results = new bytes[](data.length);
        for (uint256 i = 0; i < data.length; i++) {
            if (nodehash != bytes32(0)) {
                bytes32 txNamehash = bytes32(data[i][4:36]);
                require(txNamehash == nodehash, "multicall: All records must have a matching namehash");
            }
            (bool success, bytes memory result) = address(this).delegatecall(data[i]);
            require(success);
            results[i] = result;
        }
        return results;
    }

It is recommended to update the function to propagate the revert reason from the delegatecall using assembly. For example:

(bool success, bytes memory result) = address(this).delegatecall(data[i]);
if (!success) {
    assembly {
        let resultSize := mload(result)
        revert(add(result, 0x20), resultSize)
    }
}

SOLVED: The Beranames team solved the issue in the specified commit id.

The removeReservedName function in the ReservedRegistry contract does not validate whether the provided index is within the valid range (e.g.: less than _reservedNamesCount). If an invalid index is supplied, the function will revert, as the access to an out-of-bounds index will trigger a failure in the operation. However, without explicit index validation, the function may not provide a clear error message to the caller, and the revert behavior might lead to a poor user experience.

Code Location

The removeReservedName function does not validate whether the provided index is within the valid range:

function removeReservedName(uint256 index_) public onlyOwner {
  bytes32 labelHash_ = _reservedNamesList[index_];
  delete _reservedNames[labelHash_];
  _reservedNamesList[index_] = _reservedNamesList[_reservedNamesCount - 1];
  _reservedNamesCount--;
}

It is recommended to explicitly validate the index to ensure that it is within the valid range before performing the removal operation. This will provide a clear error message and improve the clarity of contract interactions.

SOLVED: The Beranames team solved the issue in the specified commit id.