Comment on page
3.3 Improved isWrapped() Assessment in NameWrapper
- ID: PVE-003
- Severity: Low
- Likelihood: Low
- Impact: Low
- Target: NameWrapper
- Category: Coding Practices [7]
- CWE subcategory: CWE-1041 [1]
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.
1
function is Wrapped (
2
bytes32 parentNode ,
3
bytes32 labelhash
4
) public view returns ( bool ) {
5
bytes32 node = _make Node ( parentNode , labelhash );
6
bool wrapped = _is Wrapped ( node );
7
if ( parent Node != ETH_NODE ) {
8
return wrapped ;
9
}
10
try registrar . owner Of ( uint256 ( labelhash )) returns ( address owner ) {
11
return owner == address ( this );
12
} catch {
13
return false ;
14
}
15
}
Improved the above routine to better check whether a name is wrapped or not. The issue has been fixed by this commit: 8433f8b.