OwnableERC721 Manual Review Findings

OER-01M: Inexistent Validation of Ownership Existence

Type	Severity	Location
Input Sanitization		OwnableERC721.sol:L44-L46

Description:

The OwnableERC721::_setNFT function does not validate that the NFT ID that evaluates to ownership (i.e. uint256(uint160(address(this)))) is in circulation by the specified token.

Impact:

While a renunciation of ownership rights may be desirable, it is better to expose it via a dedicated function akin to the Ownable standards by OpenZeppelin.

Example:

Copy
25/**
26 * @notice Specifies the owner of the underlying token ID, derived
27 *         from the contract address of the contract implementing.
28 *
29 * @return ownerAddress         The owner of the underlying token derived from
30 *                              the calling address.
31 */
32function owner() public view virtual returns (address ownerAddress) {
33    return IERC721(ownershipToken).ownerOf(uint256(uint160(address(this))));
34}
35
36// ============================================ HELPERS =============================================
37
38/**
39 * @dev Set the ownership token - the ERC721 that specified who controls
40 *      defined addresses.
41 *
42 * @param _ownershipToken       The address of the ERC721 token that defines ownership.
43 */
44function _setNFT(address _ownershipToken) internal {
45    ownershipToken = _ownershipToken;
46}

Recommendation:

We advise the code to enforce sanity checks as all OwnableERC721::onlyOwner functions could fail to execute if the ownershipToken is misconfigured.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The Arcade XYZ team has introduced documentation in the AssetVault that specifies a deployment of the contract by a non-ERC721 contract would cause ownership functionality to fail. In conjunction with the fact that the OwnableERC721 contract is solely utilized by the AssetVault, we consider this action an adequate resolution to the described exhibit.