Nftfy Code Style Findings

NFT-01C: Variable Visibility Specifiers

Description:

The linked variable declarations do not contain an explicitly defined visibility specifier, meaning that compilation outputs may differ based on the compiler utilized.

Example:

Copy
13mapping (IERC721 => bool) wraps;
14mapping (IERC721 => ERC721Wrapper) public wrappers;

Recommendation:

We advise that they are strictly set to ensure that no discrepancy can occur between compiler versions.

Alleviation:

The linked variable was declared as private thus providing a definitive attribute for its visibility.

NFT-02C: Inefficient mapping Relations

Description:

The wraps and wrappers mappings of the Nftfy contract are inefficiently utilized as an ERC721Wrapper instance will remain unutilized in the wrappers key space and each wrapped IERC721 will remain unutilized in the wraps keyspace.

Example:

Copy
16function ensureWrapper(IERC721 _target) public returns (ERC721Wrapper _wrapper)
17{
18    require(!wraps[_target]);
19    _wrapper = wrappers[_target];
20    if (_wrapper == ERC721Wrapper(0)) {
21        _wrapper = Wrapper.create(_target);
22        wrappers[_target] = _wrapper;
23        wraps[_wrapper] = true;
24    }
25    return _wrapper;
26}

Recommendation:

We advise that a single mapping is utilized instead in a similar lieu to Uniswap whereby a bilateral connection is instead formed with the ERC721Wrapper pointing to the IERC721 it wraps and the IERC721 pointing to its wrapper. This would greatly optimize the code segments that utilize the mapping relations.

Alleviation:

The Nftfy team stated that they intend to keep the current design as they deem it simplistic. We should note that utilizing a single mapping instead of two would increase the legibility of the codebase in contrast to the current wraps and wrappers naming conventions.

NFT-03C: Mislocated Input Sanitization

Description:

The linked require checks ensure that the arguments of the created ERC721Shares are sound, however, the ERC721Shares implementation iself does not sanitize its inputs.

Example:

Copy
28function securitize(IERC721 _target, uint256 _tokenId, uint256 _sharesCount, uint8 _decimals, uint256 _exitPrice, IERC20 _paymentToken, bool _remnant) public
29{
30    address _from = msg.sender;
31    ERC721Wrapper _wrapper = ensureWrapper(_target);
32    require(_exitPrice > 0);
33    require(_sharesCount > 0);
34    require(_exitPrice % _sharesCount == 0);
35    uint256 _sharePrice = _exitPrice / _sharesCount;
36    ERC721Shares _shares = Shares.create(_wrapper, _tokenId, _from, _sharesCount, _decimals, _sharePrice, _paymentToken, _remnant);
37    _target.transferFrom(_from, address(_shares), _tokenId);
38    _wrapper._insert(_from, _tokenId, _remnant, _shares);
39}

Recommendation:

We advise that the require checks are relocated to the underlying ERC721Shares implementation conforming to a security-by-design paradigm.

Alleviation:

As mentioned under the relative exhibit in the Shares contract, this is a style choice by Nftfy whereby input sanitization is performed in this contract centrally and the data should be considered valid when entering the constructor of the other contracts of the system.