Shares Manual Review Findings

SHA-01M: Refund to require Check

Description:

The linked if clause evaluates whether a user has invoked redeem by supplying ether along with the call when they will ultimately pay with an ERC20 token and refunds them the ether sent.

Example:

Copy
89uint256 _paymentAmount = msg.value;
90uint256 _sharesCount = balanceOf(_from);
91uint256 _redeemAmount = redeemAmountOf(_from);
92if (paymentToken == IERC20(0)) {
93    require(_paymentAmount >= _redeemAmount);
94    uint256 _changeAmount = _paymentAmount - _redeemAmount;
95    if (_changeAmount > 0) _from.transfer(_changeAmount);
96} else {
97    if (_paymentAmount > 0) _from.transfer(_paymentAmount);
98    if (_redeemAmount > 0) paymentToken.safeTransferFrom(_from, address(this), _redeemAmount);
99}

Recommendation:

We advise that this instead is converted to a require check to prevent the re-entrancy that is achievable by the refund.

Alleviation:

The Nftfy team stated that they fixed this issue by introducing the nonReentrant modifier, however we should note that the same issue as the Checks-Effects-Interactions pattern of static analysis remains.

SHA-02M: Inexistent Input Sanitization

Description:

The constructor of the contract does not properly sanitize the input variables _from, _sharePrice and _sharesCount potentially causing unwanted side effects.

Example:

Copy
44constructor (string memory _name, string memory _symbol, ERC721Wrapper _wrapper, uint256 _tokenId, address _from, uint256 _sharesCount, uint8 _decimals, uint256 _sharePrice, IERC20 _paymentToken, bool _remnant) ERC20(_name, _symbol) public
45{
46    wrapper = _wrapper;
47    tokenId = _tokenId;
48    sharesCount = _sharesCount;
49    sharePrice = _sharePrice;
50    paymentToken = _paymentToken;
51    remnant = _remnant;
52    released = false;
53    _setupDecimals(_decimals);
54    _mint(_from, _sharesCount);
55    emit Securitize(_from, address(wrapper.target()), tokenId, address(this));
56}

Recommendation:

Sanitization should be imposed for these variables by ensuring that the _from address is non-zero (statically detected), that the _sharesCount multiplied by the _sharePrice does not overflow and finally that the _sharePrice is non-zero as claim would permanently fail and the contract would misbehave.

Alleviation:

The Nftfy team responded by stating that the sanitization at this point would not matter as a contract independently deployed would be a rogue one disassociated with the system and the appropriate sanitization checks are imposed on Nftfy.