3.3 Improved isWrapped() Assessment in NameWrapper

• ID: PVE-003

• Severity: Low

• Likelihood: Low

• Impact: Low

• Target: NameWrapper

• Category: Coding Practices []

• CWE subcategory: CWE-1041 []

Description

The Pop protocol has a NameWrapper contract that can wrap POP names and add new functionality to them. While reviewing the current method to determine whether a name is wrapped, we notice the current implementation may be improved.

To elaborate, we show below the related isWrapped() routine. It is a simple routine to check whether a given name is wrapped in a more gas efficient way. It comes to our attention that in the scenario when parentNode == ETH_NODE, the current logic validates whether the NameWrapper contract is the owner of the given labelhash (line 878). In fact, we also need to additionally check the current NameWrapper-maintained owner is not address(0), i.e., ownerOf(uint256(node))!= address(0). By doing so, it blocks a rare case where the naming NFT is transferred to the NameWrapper contract without being wrapped.

function is Wrapped (
bytes32 parentNode ,
bytes32 labelhash
) public view returns ( bool ) {
bytes32 node = _make Node ( parentNode , labelhash );
bool wrapped = _is Wrapped ( node );
if ( parent Node != ETH_NODE ) {
return wrapped ;
}
try registrar . owner Of ( uint256 ( labelhash )) returns ( address owner ) {
return owner == address ( this );
} catch {
return false ;
}
}

Recommendation

Improved the above routine to better check whether a name is wrapped or not. The issue has been fixed by this commit: 8433f8b.