NFTPermissions Code Style Findings

NFT-01C: Inefficient Increment Statements

Type	Severity	Location
Gas Optimization		NFTPermissions.sol:L115, L158

Description:

The _positionCounter and _burnCounter variables in the system represent sequentially incrementing uint256 variables that will never realistically overflow from such operations.

Example:

Copy
114function _mintWithPermissions(address _owner, PermissionSet[] calldata _permissions) internal returns (uint256 _positionId) {
115  _positionId = ++_positionCounter;
116  _mint(_owner, _positionId);
117  _setPermissions(_positionId, _permissions);
118}

Recommendation:

We advise the code to instead perform unchecked increments of those variables, optimizing their gas cost.

Alleviation:

Both inefficient increment statements have been wrapped in unchecked code blocks, optimizing their gas cost.

NFT-02C: Inefficient Loop Iteration

Type	Severity	Location
Gas Optimization		NFTPermissions.sol:L93-L103

Description:

The loop contained within the NFTPermissions::permissionPermit function will inefficiently evaluate the i iterator and perform a jump operation for each iterator value beyond the first.

Example:

Copy
82/// @inheritdoc INFTPermissions
83function permissionPermit(PositionPermissions[] calldata _permissions, uint256 _deadline, bytes calldata _signature) external {
84  // slither-disable-next-line timestamp
85  if (block.timestamp > _deadline) revert ExpiredDeadline();
86
87  // Note: will fail if _permissions is empty, and we are ok with it
88  address _owner = ownerOf(_permissions[0].positionId);
89  bool _isSignatureValid =
90    SignatureChecker.isValidSignatureNow(_owner, _hashTypedDataV4(PermissionHash.hash(_permissions, nextNonce[_owner]++, _deadline)), _signature);
91  if (!_isSignatureValid) revert InvalidSignature();
92
93  for (uint256 i = 0; i < _permissions.length;) {
94    uint256 _positionId = _permissions[i].positionId;
95    if (i > 0) {
96      // Make sure that all positions belong to the same owner
97      if (_owner != ownerOf(_positionId)) revert NotOwner();
98    }
99    _modify(_positionId, _permissions[i].permissionSets);
100    unchecked {
101      i++;
102    }
103  }
104}

Recommendation:

We advise the code to instead invoke NFTPermissions::_modify right after validating the signature's validity and to begin the for loop with an iterator of 1, rendering the if structure redundant and permitting the code to directly evaluate the owner's equivalence on each iteration.

Alleviation:

The loop has been refactored as advised, ensuring it begins iteration at 1 and optimizing its owner validation for all permission adjustments beyond the first.

NFT-03C: Potential Combination of Mappings

Type	Severity	Location
Gas Optimization		NFTPermissions.sol:L42, L44

Description:

The lastOwnershipChange mapping is accessed simultaneously with the _assignedPermissions mapping in the main use-case of the contract; the NFTPermissions::hasPermission function.

As such, it may be more optimal to combine the two mapping declarations into a single one that points to a struct with a uint256 member representing the last ownership change and the AssignedPermissions mapping.

Example:

Copy
60/// @inheritdoc INFTPermissions
61function hasPermission(uint256 _positionId, address _account, Permission _permission) public view returns (bool) {
62  if (ownerOf(_positionId) == _account) {
63    return true;
64  }
65  AssignedPermissions memory _assigned = _assignedPermissions[_positionId][_account];
66  // If there was an ownership change after the permission was last updated, then the address doesn't have the permission
67  return _assigned.permissions.hasPermission(_permission) && lastOwnershipChange[_positionId] < _assigned.lastUpdated;
68}

Recommendation:

We advise this change to be evaluated and potentially applied to the codebase, optimizing the gas cost of the NFTPermissions::hasPermission function with no impact on the gas cost of the NFTPermissions::_update function.

Alleviation:

Both mapping entries have been combined to a single _positions mapping as per our recommendation, greatly optimizing the common-use NFTPermissions::hasPermission function's gas cost.

NFT-04C: Potential Enhancement of Permission Maintenance

Type	Severity	Location
Gas Optimization		NFTPermissions.sol:L71-L80

Description:

The NFTPermissions contract presently exposes a single function for modifying the permissions of a particular positionId and in doing so it solely accepts a PositionPermissions array which is inefficient for certain use cases.

Example:

Copy
71function modifyPermissions(PositionPermissions[] calldata _positionPermissions) external {
72  for (uint256 i = 0; i < _positionPermissions.length;) {
73    uint256 _positionId = _positionPermissions[i].positionId;
74    if (msg.sender != ownerOf(_positionId)) revert NotOwner();
75    _modify(_positionId, _positionPermissions[i].permissionSets);
76    unchecked {
77      i++;
78    }
79  }
80}

Recommendation:

We advise the code to introduce two more functions; one that permits an atomic permission adjustment for a single member (i.e. single PositionPermissions argument) as well as one that permits multiple permissions for the same positionId to be modified (i.e. a single uint256 argument representing the positionId and an array of PermissionSet entries).

By introducing these functions, we consider that the usability of the NFTPermissions contract will significantly increase without affecting its security and we consider the latter of the two recommended additions to be a frequent scenario (i.e. assigning different permissions for the same position ID across multiple parties at once).

Alleviation:

The Mean Finance team evaluated this exhibit and specified that they wish to retain this contract's footprint as minimal as possible given that it is meant to be inherited by other, more complex contracts.

As the contract is feature-complete and this exhibit was merely a recommendation, we advise it safely acknowledged.