NFTPermissions Manual Review Findings

NFT-01M: Unsupported Pragma Version Specification

Type	Severity	Location
Language Specific		NFTPermissions.sol:L2

Description:

The overall codebase of the NFTPermissions system makes use of user-defined value types that were introduced in Solidity 0.8.8, however, the pragma statements of the code indicate support for versions of 0.8.0 and up which is incorrect.

Example:

Copy
2pragma solidity >=0.8.0;

Recommendation:

We advise the pragma versions to be corrected, either locked to the actual version in use by the codebase (=0.8.20) or updated to a more accurate range (>=0.8.8).

Alleviation:

All pragma versions throughout the codebase including the referenced one have been updated to mandate a Solidity version of 0.8.8 and higher, addressing this exhibit.

NFT-02M: Inexistent Support of Direct Mints

Type	Severity	Location
Standard Conformity		NFTPermissions.sol:L21

Description:

The NFTPermissions contract contains incorrect specification as, based on RFC-2119 terminology, it states that:

• Underlying contracts should not call _mint directly. Instead, they should call _mintWithPermissions

As the NFTPermissions::_mintWithPermissions function makes use of a _positionCounter that is incremented in the said function, utilizing NFTPermissions::_mint is incorrect and non-compatible with the contract as the NFTPermissions::totalSupply function would fail to register these mint operations.

Impact:

The contract presently indicates support of a particular execution path that is, in reality, unsupported and would cause malformed data points in the contract.

Example:

Copy
20* @dev There are some technical details that devs should take into account when using this contract. For example:
21*      - Underlying contracts should not call _mint directly. Instead, they should call `_mintWithPermissions`
22*      - Underlying contracts should probably override `tokenURI` to provide the correct URI for their positions
23*      - Permissions are represented by a `uint8` but there can only be 192 different permisisons (0 <= permission ids < 192)
24*      - Permissions that were granted before or in the same block where the position was last transferred will be lost

Recommendation:

We advise the ERC721::_mint function to be properly overwritten by either reverting (indicating that it is not meant to be used directly) or by properly updating the _positionCounter, either of which we consider an adequate resolution to this exhibit.

Alleviation:

The ERC721::_mint function is now properly overwritten in the NFTPermissions contract, preventing a direct invocation of it and thus ensuring that the contract is consistently utilized.

NFT-03M: Incorrect Ownership Change Maintenance

Type	Severity	Location
Logical Fault		NFTPermissions.sol:L157

Description:

The NFTPermissions::_update function will cause an NFT that is burned to retain the permissions it last assigned as the _assignedPermissions mapping could still point to valid AssignedPermissions structures with non-zero lastUpdated entries.

While the present NFTPermissions::hasPermission implementation would properly fail in such a case due to the ERC721::ownerOf invocation, containing permission entries that would otherwise succeed is an invalid trait that can misbehave in a variety of ways - including the re-creation of a particular NFT that has been burned.

Impact:

Given that this exhibit represents a potential compromise of the permission system the NFTPermissions contract is meant to establish, we consider it of "major" severity.

In the code, we have demonstrated how this particular vulnerability can manifest in a supported case by EIP-721 (a burn and consequent mint of the same NFT ID). A case whereby a burned NFT would remain usable for permissions would solely arise if derivative contracts of NFTPermissions would implement some form of pre-mint system and override the NFTPermissions::hasPermission or NFTPermissions::ownerOf function to not fail for un-minted NFTs.

Example:

Copy
152/// @dev We are overriding this function to update ownership values on transfers
153function _update(address _to, uint256 _positionId, address _auth) internal override returns (address _from) {
154  _from = super._update(_to, _positionId, _auth);
155  if (_to == address(0)) {
156    // When token is being burned, we can delete this entry on the mapping
157    delete lastOwnershipChange[_positionId];
158    _burnCounter++;
159  } else if (_from != address(0)) {
160    // If the token is being minted, then no there is no need to need to write this
161    lastOwnershipChange[_positionId] = block.number;
162  }
163}

Recommendation:

We advise the code to instead assign the maximum value possible for a block.number in the NFTPermissions system (which would be represented by type(uint64).max) to ensure that any previously-assigned permissions before an NFT is burned do not remain active.

Additionally and to avoid this vulnerability manifesting in re-minted NFTs, we advise the else if block present in the NFTPermissions::_update function to be set to a simple else block ensuring that the lastOwnershipChange is correctly overwritten from the previously-set maximum.

Alleviation:

As part of the remediation efforts for NFT-02M, it is no longer possible to re-mint a previously burned NFT rendering the latter of the two recommendation paragraphs void.

To ensure this exhibit is properly dealt with, the Mean Finance team proceeded with assigning the maximum value possible for a block.number to the lastOwnershipChange of the _positionId when it is burned, rendering this exhibit fully alleviated in every regard.