basenames

Description

The validDiscount modifier is used by the discountedRegister() function to enforce the following constraints:

• The provided discount must exist and be active.
• Discounts can only be used once per address.
• The caller must be eligible for the discount.

The implementation:

modifier validDiscount(bytes32 discountKey, bytes calldata validationData) {    URCStorage storage $ = _getURCStorage();    if ($.discountedRegistrants[msg.sender]) revert AlreadyRegisteredWithDiscount(msg.sender);    DiscountDetails memory details = $.discounts[discountKey];
    if (!details.active) revert InactiveDiscount(discountKey);
    IDiscountValidator validator = IDiscountValidator(details.discountValidator);    if (!validator.isValidDiscountRegistration(msg.sender, validationData)) {        revert InvalidDiscount(discountKey, validationData);    }    _;}

The discountedRegistrants mapping tracks addresses that have already used their discount. However, when the UpgradeableRegistrarController is deployed, replacing the current RegistrarController (stored in legacyRegistrarController), users who have previously claimed a one-time discount in the legacy contract will be able to claim it again in the new contract.

Recommendation

Add a check in validDiscount to verify whether msg.sender has already used their discount in the legacy contract. This ensures that users cannot claim their discount more than once, even across contract upgrades.

Summary

makes sense to limit gas here to avoid griefing gas sponsors

Description

The withdrawETH() function allows anyone to withdraw the ETH balance accumulated in the contract to the designated paymentReceiver:

function withdrawETH() public {  (bool sent,) = payable(_getURCStorage().paymentReceiver).call{value: (address(this).balance)}("");  if (!sent) revert TransferFailed();}

Although this function currently does not pose a reentrancy risk—mainly because _refundExcessEth() prevents such attacks—the contract’s upgradeable nature means this vulnerability could be introduced in future versions. An attacker exploiting a reentrancy vulnerability could hijack the call flow by repeatedly invoking withdrawETH() and potentially drain user funds.

Recommendation

Add non-reentrant guards to all state-changing functions, including withdrawETH(), to prevent reentrancy attacks. For gas efficiency, consider using OpenZeppelin’s ReentrancyGuardTransient.sol.

Summary

consider using Ownable2Step

Summary

discountedRegisterPrice doesn't check discount key is valid.

mixing discount keys due to error may mislead to sending invalid register transactions.

Recommendation

perhaps consider moving the validDiscount modifier from discountedRegister to here.

Description

The hasRegisteredWithDiscount() function exists in both the current UpgradeableRegistrarController and the previous RegistrarController implementations.

In the current version, the function iterates over the provided addresses array to check whether any address has used its discount by looking it up in the local discountedRegistrants mapping. Additionally, for each iteration, the function redundantly calls the legacy contract’s hasRegisteredWithDiscount() function, passing the entire addresses array every time.

This results in an inefficient pattern where the same external call to the legacy contract is made multiple times (once per address in the array - for the entire array), instead of just once. Ideally, the legacy contract check should be performed a single time outside the loop, and its result reused within the current iteration.

function hasRegisteredWithDiscount(address[] memory addresses) external view returns (bool) {    URCStorage storage $ = _getURCStorage();    for (uint256 i; i < addresses.length; i++) {        if (            $.discountedRegistrants[addresses[i]]                || RegistrarController($.legacyRegistrarController).hasRegisteredWithDiscount(addresses)        ) {            return true;        }    }    return false;}

Recommendation

Consider calling hasRegisteredWithDiscount() only once outside of the loop instead.

Description

The available() function is called during the registration process to verify that a name is valid and available for purchase in the BaseRegistrar. However, the call to base.isAvailable() within available() is redundant during the register() and discountedRegister() flows because this availability check is already performed later in the process by BaseRegistrar.registerWithRecord().

Recommendation

Consider refactoring by implementing an internal function named _available() that only verifies the validity of the name. Both available() and validRegistration can call this internal function, replacing the current direct call to available(). This change allows available() to serve solely as an external view function, which can then be marked as external, improving clarity and gas efficiency.

Description

The contracts RegistrarController, BaseRegistrar, and L2Resolver are imported into UpgradeableRegistrarController, but only their interfaces are utilized. Importing the full contract implementations unnecessarily increases deployment gas costs.

Recommendation

Replace these full contract imports with their corresponding interface imports. This optimization reduces deployment size and gas consumption.

minor gas optimization: can just shift by 96 shl(96, a)