Hyperware

Description

Function claim() removes sHYPR tokens one vestingEndTimestamp has been reached.

However after that time, new sHYPR tokens can still be received via transfer() or by claiming a merkledrop.

In that situation userTotalClaimed_[] still has its value which means the new sHYPR tokens can't be claimed.

Recommendation

Consider resetting userTotalClaimed_[] to 0 when sHYPR tokens are removed.

Note: resetting userTotalClaimed_[] to 0 could possibly enable transfers that were earlier considered not possible (because of userTotalClaimed_ > requiredClaimedAmount). However not an issues because at moment vesting has ended anyway.

Description

Function claim() transfers HYPR tokens out (after the cliff). In that situation the test of L73 doesn't work as expected any more and too few tokens will be able to minted.

Recommendation

Consider changing the code to:

-if (totalMinted + amount > _balance) {+if (totalMinted + amount > _balance + totalClaimed) {     revert NotEnoughHyprSupplyToMint();}

Description

Function manageLock() uses unchecked, however this can be abused to change the newDuration to a value that is lower than lock.duration. This would allow unlocked tokens earlier than expected.

Proof of Concept

Here is POC to show the issue:

// SPDX-License-Identifier: MITpragma solidity ^0.8.0;import "hardhat/console.sol";      contract test {    uint256 public constant MAX_LOCK_DURATION = 4 * 365 days;    uint256 public constant MIN_LOCK_DURATION = 7 days;
    function _min(uint256 a, uint256 b) internal pure returns (uint256) {        unchecked {            return a < b ? a : b;        }    }    function _validateDuration(uint256 _duration) internal pure {        if (_duration < MIN_LOCK_DURATION || _duration > MAX_LOCK_DURATION) {            revert ();        }    }    constructor() {        uint _duration = type(uint256).max - MAX_LOCK_DURATION + MIN_LOCK_DURATION+1;        uint lock_duration = MAX_LOCK_DURATION;        if (_duration > 0) {            // Increase lock duration            uint256 newDuration;            unchecked {                newDuration = lock_duration + _duration;            }            _validateDuration(newDuration);            console.log("orginal duration %d days",lock_duration / 1 days); // 1460            console.log("new duration %d days",newDuration / 1 days); // 7        }    }    }

Recommendation

Remove the unchecked for the calculation of newDuration.

Description

Contract Account includes BaseAccount from entryPoint version 0.7, while the constant _entryPoint uses version 0.6. As these are incompatible, the Account Abstraction logic will not work.

Also a newer version of the entryPoint is released.

Recommendation

Consider using entryPoint version 0.8 in both the include and the constant.

Description

There are several issues with the creation of the _DOMAINSEPARATOR:

• EIP712Domain contains an extra space rights before bytes32 salt, this will lead to a different hash;
• there is no salt added in abi.encode();
• address(this) references the address of the template contract and not of the proxy.

Recommendation

Consider using EIP712Upgradeable.

This uses a TYPE_HASH without a salt, but that is fine because the salt isn't used.

bytes32 private constant TYPE_HASH =        keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

Description

In the constructor ofHyperToken, a continue is done if (to == address(0) || amount == 0). In that situation the loop counter isn't incremented and the loop would continue until it is out of gas.

Recommendation

Move the i++ inside of the for statement. The unchecked isn't necessary any more since solidity version 0.8.22.

Description

The functions transfer() and transferFrom() can return false. However this isn't best practice because it can be overlooked by users of the contract.

Although the use of SafeERC20 / safeTransfer() / safeTransferFrom() fixes this, it is safer to use the best practice.

Also see weird-erc20.

For example function claim() uses transfer() without using safeTransfer().

Recommendation

Consider using revert() on failure of transfer() and transferFrom().

Description

The function claim() uses transfer() to transfer HyperTokens. However if that tokens would return false like this contract does, then potentially errors would not be detected.

Recommendation

Consider using safeTransfer().

Description

Several init functions of (underlying) upgradable contracts are not called. Currently they are all empty, so the risk is low. However future version might have code.

Recommendation

Consider also calling:

In MerkleDistributor::initialize():

• __ReentrancyGuard_init
• __ERC165_init
• __Context_init

In TokenRegistry::initialize():

• __ERC165_init()
• __Context_init()

In Hypermap::initialize():

• __UUPSUpgradeable_init()
• __ERC165_init()
• __Context_init()

Description

Function create() allow to accidentally add the same merkleRoot multiple times. This could be for the same token or for different tokens.

It also allows adding multiple different merkleRoots for the same token.

Recommendation

Consider disallowing to add multiple the same merkleRoots are added. Also consider only allowing one merkleRoot per token.

Description

Functions claim() doen't check that _dIndex < count. In theory this could allow claiming tokens in the case that the root is 0. However its very unlikely that a root of 0 will be valid.

Function setAlias() also doesn't check that _dIndex < count. This could allow setting an alias for future roots.

For consistency it good to check for this.

Recommendation

Consider disallowing _dIndex >= count in functions claim() and setAlias().

Description

Function transferRegistration() checks _duration for validity but in the situation of dstBind.amount ! = 0, _duration indicates a duration extention which is added to dstBind.endTime.

This way it is not possible to extend the duration with an amount smaller than MIN_LOCK_DURATION (7 days)

Recommendation

Consider changing the code to something like:

function transferRegistration(...) ... {    ...-        if (_duration > 0) {-            _validateDuration(_duration);-        }    ...}function _transferRegistration(    ...    if (dstBind.amount == 0) {        ...+       _validateDuration(_duration);        ...    } else {        ...        if (_duration > 0) {+           _validateDuration(dstBind.endTime + _duration - block.timestamp);            unchecked {                dstBind.endTime = _min(_lockEndTime, dstBind.endTime + _duration);            }        }        ...    }}

Description

Function withdraw() processes the first 20 namehashs and remove them from the list when processed so the next call to withdraw() will process the next 20.

However in the edge case that bind.amount == 0, the entries are not removed, which could potentially lead to not being able to process all entries.

This situation shouldn't happen but it still safer to always remove the processed entries.

Recommendation

Consider always doing _userBinds[].remove(...);.

Description

Function _transferRegistration() emits BindCreated for all destionations when _binds[][].amount is set.

However function manageLock() doesn't do this when _binds[][DEFAULT_REGISTRATION_NAMEHASH].amount is set.

Recommendation

Consider doing an emit BindCreated() in function manageLock().

Description

Function manageLock() emits the final value (e.g. lock.amount) in emit LockIncreased(). However function _transferRegistration() emits the increase (e.g. _maxAmount) in emit BindAmountIncreased(). This is inconsistent.

Recommendation

Consider changing the code to:

-emit BindAmountIncreased(_user, _dstNamehash, _maxAmount, dstBind.endTime);+emit BindAmountIncreased(_user, _dstNamehash, dstBind.amount, dstBind.endTime);

Description

Not all upgradable contracts have _disableInitializers(). This is recommended to use on the template contracts to prevent aunautorized parties to initialize the contracts and potential selfdestruct them on chains where this is still possible.

Recommendation

Consider adding the following to HyperAccountMinterUpgradable, HyperAccountUpgradable, Hypermap.

constructor() {    _disableInitializers();}

Description

The check in function _isValidLabel() isn't robust. When len == 0 and offset == 1 then this if won't trigger.

In the current code this isn't an issue, but it potentially could be with future changes. It could be made more robust.

Recommendation

Consider changing the code to:

-if (len == offset) revert LabelTooShort();+if (offset >= len) revert LabelTooShort();

Description

Function supportsInterface() of Hypermap doesn't include the functions of the current contract.

Recommendation

Consider changing the code to:

-return super.supportsInterface(interfaceId);+return super.supportsInterface(interfaceId) || interfaceId == type(IHypermap).interfaceId;

Description

The _getCommitHash() could be guessed when using Account Abstraction because the msg.sender is always the same and the name could be a well known name. That way the commit could be front run.

Additional in case of Account Abstraction and because the msg.sender is always the same, an NFT can be minted from someone else.

On the other hand when using mintBySignature(), the msg.sender could be any party so might not match the commit.

Recommendation

For Account Abstractions consider storing userOp.sender in Account::_validateSignature() in transient storage and use that as the sender when verifying the commit.

For mintBySignature() use the signer as the sender when verifying the commit.

Hyperware

Solved for _mintBySignature() and acknowledged for Account Abstraction.

Description

In function withdraw()funds are send to the msg.sender, however the msg.sender could have an unexpected value.

In case of account abstraction the msg.sender is the EntryPoint.

If the funds are send there then the orignal sender doesn't receive them. Although the funds can be retrieved via exec() and EntryPoint::withdrawStake().

Recommendation

Consider doing one of the following:

• specify a destination
• when msg.sender == entrypoint : revert and retrieve if via exec()
• when msg.sender == entrypoint : send it to the owner of the NFT
• store userOp.sender in Account::_validateSignature() in transient storage and use that as destination

Description

In function HyperAccountPermissionedMinter::_mint() actions are taken based on the msg.sender, however the msg.sender could have an unexpected value.

In case of Account Abstraction the msg.sender is the entryPoint. In case of _mintBySignature() then msg.sender can be any user.

Recommendation

For Account Abstraction consider storing userOp.sender in Account::_validateSignature() in transient storage and use that as the parameter for the _allowances[].

For mintBySignature() use the signer as the parameter for the _allowances[].

Hyperware

Solved for _mintBySignature() and acknowledged for Account Abstraction.

Description

HyperAccountPaidMinter::_mint() isn't payable so it won't work with mintBySignature.

However its unlikely that someone else will pay, unless it is the entrypoint.

Recommendation

Doublecheck to usefulness of mintBySignature in combination with HyperAccountPaidMinter::_mint(). If this is relevant make the function mintBySignature payable.

Hyperware

Acknowledged. This combination will not be used.

Description

The variable _initialized of HyperAccountProxy is located on storage location 0. So an implementation contract, which is used via delegatecall, is very likely to overwrite this.

For example HyperAccountMinterUpgradable will overwrite this with _nonce[][] that is also located on storage location 0.

Due to the other checks in initialize() its unlikely this will cause issues, however if still safer to solve this.

Recommendation

Consider doing one of the following:

• eliminate the use of _initialized, this can be done because the checks with msg.sender == HYPERMAP and _implementation() == address(0) are sufficient;
• use a ERC-7201 namespace.

Description

Several contracts don't extend the supportsInterface() function with their own interfaceId.

Recommendation

Consider also extending supportsInterface() in the attached contracts.

Description

The message hash is currently constructed as keccak256(abi.encodePacked(to, address(this), _nonce[signer], name, initialization, implementation, signer));

Since both name and initialization are of dynamic type, the same final encoded bytes can be attributed to multiple combinations

Eg:

bytes name1 = "hello";   bytes initialization1 = " world";   bytes name2 = "he";   bytes initialization2 = "llo world";   constructor() {       // both results in `hello world`       console.logBytes32(keccak256(abi.encodePacked(name1, initialization1))); // 0x471732...       console.logBytes32(keccak256(abi.encodePacked(name2, initialization2))); // 0x471732...   }

Hence this allows an attacker to pass the signature verification using values different from what was signed

Recommendation

Use hashes of name and initialization instead

keccak256(abi.encodePacked(to, address(this), _nonce[signer], keccak256(name), keccak256(initialization), implementation, signer))

Description

In HyperAccountPaidMinter, users are supposed to pay in-order to mint. And in case the parent doesn't have any gene set, they expect the TBA to be created with the passed in implementation.

function _mint(address to, bytes calldata name, bytes calldata initialization, address implementation)        internal        override        returns (address tba)    {        if (msg.value != currentPrice) revert IncorrectPayment();        return _HYPERMAP.mint(to, name, initialization, implementation);    }}

But if the parent sets a gene in between an user viewing the current state and the execution of the user's transaction, it will cause the TBA to be created with the gene instead of the passed in implementation causing users to unwillingly pay.

function mint(address to, bytes calldata label, bytes calldata initialization, address implementation)        external        isName(label)        returns (address tba)    {      .......
        if (map[parent].gene != address(0)) {            implementation = map[parent].gene;            setGene = true;        }

Recommendation

In case the implementation and gene are both non-zero and is not the same, instead of overwriting the implementation revert the execution.

Hyperware

Acknowledged. The timing is indeed an undesirable edge-case, but the behavior as it exists is clearly specified throughout our discussion of the feature (i.e. that an implementation given by the minter will be overridden).

Description

In order to perform an approved transfer, the user's total claimed amount must equal the approved requiredClaimedAmount

function transfer(address to, uint256 amount) public override(IERC20, ERC20) returns (bool) { =>      if (params.used || params.to != to || params.amount != amount || userTotalClaimed_[msg.sender] != params.requiredClaimedAmount)        {            return false;        }

This is flawed as any user can claim on behalf of another user increasing their totalClaimed[]. This will make the user unable to perform the approved transfers

function claim(address _user) external {        uint256 _balance = balanceOf(_user);        uint256 _claimed = userTotalClaimed_[_user];        uint256 _available = _availableToClaim(_balance, _claimed);        if (_available != 0) {=>         userTotalClaimed_[_user] += _available;            totalClaimed += _available;

Another way a transfer can get disabled (without the owner themselves making claims) is via an inbound transfer which increases the userTotalClaimed_[]. But this should be rare

Recommendation

Either disallow claiming on behalf of others or remove the requiredClaimedAmount condition for transfers

Description

In order to extend the duration of a binding, the user has to make use of the _transferRegistration function. But the kept preconditions are that the source binding should be expired (or be DEFAULT_REGISTRATION_NAMEHASH) and have non-zero balance

function _transferRegistration(        address _user,        uint256 _lockEndTime,        bytes32 _srcNamehash,        bytes32 _dstNamehash,        uint256 _maxAmount,        uint256 _duration    ) internal {        Bind storage srcBind = _binds[_user][_srcNamehash];
        if (_srcNamehash != DEFAULT_REGISTRATION_NAMEHASH && srcBind.endTime > block.timestamp) {            revert SourceNotExpired(_srcNamehash, srcBind.endTime);        }
        uint256 availableBalance = srcBind.amount;        if (availableBalance == 0) revert InsufficientBindAmount(_srcNamehash, 0, _maxAmount);

This disallows extending the duration of a binding in case there are no other expired binding (or DEFAULT_REGISTRATION_NAMEHASH binding) with non-zero balance

Recommendation

Add a separate function that handles just the duration extension

Description

The constructor of HyperToken tracks the total number of tokens minted via _totalSupply. However the ERC20 contract also tracks this in totalSupply(), so this could be used instead. This would simplify the code.

Recommendation

Consider replacing _totalSupply with totalSupply(), in the following way:

constructor(...) ... {    ...-   uint256 _totalSupply;    ...    for (uint256 i = 0; i < len; ) {        ...-       unchecked {-           _totalSupply += amount;-       }        ...    }-   if (_totalSupply != MAX_SUPPLY) {+   if (totalSupply() != MAX_SUPPLY) {        revert InvalidTotalSupply();    }}

Description

Function transfer() tries to limit the the sell of vHYPR tokens by limiting the transfer. However there are others ways to make the vHYPR tokens that originate from an merkle drop liquid/transferable.

The vHYPR tokens can be claim()ed into a seperate contract that is an ERC20/ERC4626/ERC6551 contract, and then ownership of these contracts, or shares can be transferred.

Recommendation

Consider adding a comment to the code for some who reuses the code and isn't aware of this.

Description

HyperVestingToken flag that a version of transferRoleParams[] is used by setting params.used = true. However the params are not used afterward so could also be deleted.

Recommendation

Consider removing transferRoleParams[] once it is used.

Description

The values of transferRoleParams[] are compared to the supplied parameters. However it isn't explicitly checked the enty isn't empty.

Due to this a transfer with to==0 and amount==0 could be accepted, alhough it doesn't do a meaningful action.

Currently the check for to==0 in _transfer() prevents this, but its safer to an explicit check.

Recommendation

Consider adding || to == 0 to the check:

-if (... || params.to != to || ... ) ...+if (... || params.to != to || to == 0 || ... ) ...

Description

If the balanceOf(msg.sender) would be 0, and then L115 would revert due to a divsion by zero without a clear error message. Although this situation is unlikely it could be detected and reverted with a better error message.

Recommendation

Consider detecting the situation where balanceOf(msg.sender) == 0.

Description

There are two functions named claim(), one in MerkleDistributor and one in HyperVestingToken.

A user should first do claim() on the MerkleDistributor and then lateron claim() on the HyperVestingToken. This might be confusing to document

Recommendation

Consider changing the name in HyperVestingToken to something like convert(). Also consider renaming all other references to claim in that contract.

Description

The function claim() uses super, but it doesn't override the function, so super isn't necessary.

Recommendation

Consider changing the code to:

-super._burn(_user, _balance);+_burn(_user, _balance);

Description

Edge case: when block.timestamp == vestingCliffTimestamp == vestingEndTimestamp then function _totalVestedTokens() will return 0. However in other situations where block.timestamp == vestingEndTimestamp it will return _vestingTokenBalance.

This will most likely be negligible because the full amount will be present in the next call, after one second.

Recommendation

If you do want to change this, consider checking for vestingEndTimestamp first.

Description

An error message in function claim() indicates all tokens are claimed. However the situation only indicates there are insufficient tokens.

Recommendation

Consider changing the code to:

-revert AllClaimed(_dIndex, root.totalAmount);+revert NotEnoughTokensLeft(_dIndex, root.totalAmount);

Description

Function does an unchecked calculation on _binds[][].amount. It is not immediately clear why this is safe.

Recommendation

Consider add a comment that _amount won't be so large to induce an overflow because that would revert in safeTransferFrom().

Description

Functions in Hypermap are not ordered in the recommended way by the Solidity style guide.

This makes the code more difficult to read.

Recommendation

Consider changing the order of the functions.

Description

Several events show the unhashed version of a variable twice, instead of showing both the hashed and the unhashed version.

Recommendation

Consider changing the events to:

-event <eventname>(..., bytes indexed labelhash, ...);+event <eventname>(..., bytes32 indexed namehash, ...);

And hash the values that are put in the emit.

Description

Genes are meant to enforce the same implementation contract for the current TBA/leaf and all TBA/leafs below it.

However some contracts allow upgrades and thus should not be used in combination with genes:

• contracts that have _authorizeUpgrade()
• contracts that have delegatecall

Recommendation

Document that contracts that enable upgrades are not used in combination with gene.

Description

The function _exec() is duplicated only to have a different modifier. Code duplication makes the code more difficult to maintain, which is especially important for a core function like _exec().

Recommendation

Consider having a function that is used in a modifier that can be overriden to prevent code duplication

Description

In several other implementation of signature checks, they allow for values of 0, 1 of v as valid values. If such a value would be present then the signature would not be interpreted correctly.

Recommendation

Consider using a different signal value for v . Check EIP 155 for possible values.

Hyperware

Acknowledged. No action at this time.

Description

The constructor of HyperAccountCommitMinter doesn't do any checks on _minCommitAge and _maxCommitAge.

However there should be a reasonable time between _minCommitAge and _maxCommitAge to be able to mint at all.

Recommendation

Consider checking something like: _maxCommitAge - _minCommitAge > 1 days.

Description

The upgradable contracts use storage starting from slot 0 and use __gaps. This is cumbersome and allows for storage collisions. Some contract are upgradeable but don't have __gaps, these are: HyperAccountCommitMinter and HyperAccountMinterUpgradable.

Most recent contracts use the EIP-7201 storage pattern. This also removes the need for __gaps.

Recommendation

Consider using the EIP-7201 storage pattern for all the contracts the use upgradability, including the HyperAccountProxy.

Description

In function mintBySignature(), when _mint() reverts, the _nonce[] increase is also undone. If the _mint() always reverts, then no further _mint()s can be done until a new signature is made using the same nonce.

_mint() could revert in different ways, see the attached code.

Recommendation

Consider documenting how a reverting _mint() should be handled by the account using mintBySignature().

Description

The import of IERC721 isn't used in Mech.

Recommendation

Consider removing the unused imports.

Description

Since the DEFAULT_ADMIN_ROLE is never granted to msg.sender, _revokeRole(DEFAULT_ADMIN_ROLE, msg.sender); is a no-op.

Recommendation

Remove the redundant line

-         _revokeRole(DEFAULT_ADMIN_ROLE, msg.sender);

Description

The BindCreated/BindAmountIncreased events are not emitted inside manageLock when the default binding is created for the first time or amount added to it

Recommendation

Emit the events or be consistent and not emit events for default binding always

Description

When a bind is removed, currently no event is emitted

Recommendation

Emit an event

Description

The InsufficientLockAmount error doesn't have named parameters

Recommendation

Add named parameters

Description

Function claim() uses MerkleProof.verify() A more gas efficient function of this function is also present.

Recommendation

Consider using verifyCalldata():

-if (!MerkleProof.verify(...)) { ... }+if (!MerkleProof.verifyCalldata(...)) { ... }

Description

There is a more gass efficient version of ReentrancyGuardUpgradeable which can be used.

Recommendation

Consider using ReentrancyGuardTransientUpgradeable.

Description

In function manageLock() calls contains() before doing add(). However add() also does this, so the check is redundant.

Recommendation

Consider changing the code to:

- if (!_userBinds[msg.sender].contains(DEFAULT_REGISTRATION_NAMEHASH)) {    _userBinds[msg.sender].add(DEFAULT_REGISTRATION_NAMEHASH);-}

Description

Function withdraw() updates the storage variabledefaultBind.amount` in a loop, which is relatively expensive.

Recommendation

Considure using a temporaty variable, for example in the following way.

function withdraw() external nonReentrant {    ...    Bind storage defaultBind = _binds[msg.sender][DEFAULT_REGISTRATION_NAMEHASH];+   uint256 amount = defaultBind.amount;    ...    for (uint256 i = 0; i < workingSize; ++i) {        ...  -       defaultBind.amount += bindAmount;+       amount += bindAmount;        ...    }-   uint256 amount = defaultBind.amount;    ... // use amount    defaultBind.amount = 0;    ...}

Description

If the namehash is a name then the third return value (entry.data) is always empty. So this function could return an empty bytes array. This makes the code easier to understand and also saves some gas.

Recommendation

Consider changing the code to:

-return (entry.tba, ownerOf(uint256(namehash)), entry.data);+return (entry.tba, ownerOf(uint256(namehash)), bytes(""));

Description

The assembly blocks don't use "memory-safe". Having these allows the compiler to generate more efficient code.

Recommendation

Consider using the following:

-assembly { ... }+assembly ("memory-safe") { ... }

Description

There are several ways to optimize the assembly code in leaf():

• the extra add(..., 0x20) on L263 doesn't seem necessary;
• the second keccak256 can use the scratch space of address 0x00..0x39;
• because label is limited to maximum 64 bytes, they can also fit in the scratch space.

Recommendation

The three optimizations can be implemented in the following way:

function leaf(bytes32 parenthash, bytes calldata label) public pure returns (bytes32 namehash) {    if (label.length >= 64) revert(); // important to prevent overwrite of address 0x40    return _leaf(parenthash,label);)
// internal version without the length check, which can be called from mint, fact, notefunction _leaf(bytes32 parenthash, bytes calldata label) internal pure returns (bytes32 namehash) {    assembly ("memory-safe") {        calldatacopy(0x00, label.offset, label.length)          let labelhash := keccak256(0x00, label.length)        mstore(0x00, parenthash)        mstore(0x20, labelhash)        namehash := keccak256(0x00, 0x40)    }}

Description

Function get() always returns an empty owner for a node, so function _isOperatorOrDelegatee() can be simplified.

Recommendation

Consider changing the code to:

-(, address owner, bytes memory data) = _HYPERMAP.get(alLeaf);+(,, bytes memory data) = _HYPERMAP.get(alLeaf);...-if (owner == address(0)) {    owner = IERC721(tokenContract).ownerOf(tokenId);-}

Description

The duplication of the last element can also be done before the loop (although execution speed difference is probably neclectible)

Recommendation

Consider changing the code to something like:

while len(level) > 1:        next_level = []+       # If there's an odd number of elements, duplicate the last one+       if len(level) % 2 == 1:+           level.append(level[-1])        for i in range(0, len(level), 2):            left = level[i]-           # If there's an odd number of elements, duplicate the last one-           right = level[i + 1] if i + 1 < len(level) else left+           right = level[i + 1]            parent = standard_node_hash(left, right)            next_level.append(parent)        level = next_level

Also consider doing this in generate_proof()

Description

Function generate_proof() uses the array indexes[] which makes the code more difficult to understand.

Also the calculation of target_index can be simplified.

Recommendation

Consider changing the code to something like:

while len(level) > 1:    next_level = []            for i in range(0, len(level), 2):        left = level[i]        right = level[i + 1] if i + 1 < len(level) else left
        # Add to the next level        next_level.append(standard_node_hash(left, right))            # If this is the target, add the sibling to the proof        if i == target_index:            proof.append(right)        elif i + 1 < len(level) and i + 1 == target_index:            proof.append(left)                target_index //= 2    level = next_levelreturn proof

Description

Since DEFAULT_REGISTRATION_NAMEHASH binding will always have endTime == 0 (which will be < block.timestamp), the single condition of srcBind.endTime > block.timestamp will disallow expired bindings while always allowing the default binding. Hence the extra condition added to explicitly allow the default binding is unnecessary

Recommendation

Remove _srcNamehash != DEFAULT_REGISTRATION_NAMEHASH