Omniscia Arcade XYZ Audit

PromissoryNote Manual Review Findings

PromissoryNote Manual Review Findings

PNE-01M: Misleading Documentation of Contract

Description:

The contract's documentation specifies that it is built off ERC721PresetMinterPauserAutoId, however, it solely derives from ERC721Enumerable and the base ERC721 implementation.

Additionally, the PromissoryNote::_beforeTokenTransfer function override specifies that it will not let tokens be transferred when the contract is paused, a state it cannot be in.

Impact:

The severity of this exhibit will be adjusted according to the remediation action taken by the Arcade XYZ team.

Example:

contracts/PromissoryNote.sol
200/**
201 * @dev Hook that is called before any token transfer.
202 *      This notifies the promissory note about the ownership transfer.
203 *
204 * @dev Does not let tokens be transferred when contract is paused.
205 *
206 * @param from                  The previous owner of the token.
207 * @param to                    The owner of the token after transfer.
208 * @param tokenId               The token ID.
209 */
210function _beforeTokenTransfer(
211    address from,
212    address to,
213    uint256 tokenId
214) internal virtual override(ERC721, ERC721Enumerable) {
215    super._beforeTokenTransfer(from, to, tokenId);
216}

Recommendation:

We advise either the documentation or the inheritance structure of the contract to be revised, either of which we consider an adequate remediation to this exhibit.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The documentation was corrected, removing the statement in regard to a paused state and thus addressing this exhibit.

PNE-02M: Misleading Token ID Counter

Description:

The _tokenIdTracker variable of the contract is specified as private and is solely utilized in the PromissoryNote::constructor to increment it by one.

Example:

contracts/PromissoryNote.sol
82constructor(
83    string memory name,
84    string memory symbol,
85    address _descriptor
86) ERC721(name, symbol) ERC721Permit(name) {
87    if (_descriptor == address(0)) revert PN_ZeroAddress("descriptor");
88

89    descriptor = INFTDescriptor(_descriptor);
90

91    _setupRole(ADMIN_ROLE, msg.sender);
92    _setupRole(RESOURCE_MANAGER_ROLE, msg.sender);
93

94    // Allow admin to set mint/burn role, which they will do
95    // during initialize. After initialize, admin role is
96    // permanently revoked, so mint/burn role becomes immutable
97    // and initialize cannot be called again.
98    // Do not set role admin for admin role.
99    _setRoleAdmin(MINT_BURN_ROLE, ADMIN_ROLE);
100    _setRoleAdmin(RESOURCE_MANAGER_ROLE, RESOURCE_MANAGER_ROLE);
101

102    // We don't want token IDs of 0
103    _tokenIdTracker.increment();
104}

Recommendation:

As PromissoryNote NFT IDs are dictated by the caller of PromissoryNote::mint, we advise the code to omit all code relating to _tokenIdTracker as it is misleading.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The _tokenIdTracker entry of the contract and all instances of it have been properly omitted from the codebase as advised.

Prev

OwnableERC721.sol (OER-M)

Next

PunksVerifier.sol (PVR-M)

On this page