Shares Code Style Findings

SHA-01C: Variable Mutability Specifiers

Description:

The linked variable declarations are only assigned to once during the contract's constructor.

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:

We advise that the variable declarations are set to be immutable to greatly optimize the gas cost involved with interacting with these variables.

Alleviation:

The immutable variable mutability specifiers were introduced greatly optimizing the gas cost involved in utilizing the linked variables.

SHA-02C: Inefficient Event Arguments

Description:

The Securitize event emitted during the constructor of the ERC721Shares contract utilizes arguments from storage instead of the ones already provided as arguments to the constructor in memory.

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:

We advise that the in-memory arguments are utilized over the in-storage ones to optimize the gas cost involved in the emittence of the event.

Alleviation:

The emit statement was adjusted to utilize the input arguments rather than retrieve data from storage.

SHA-03C: Inexistent Error Messages

Description:

The linked require invocations are not accompanied by any error messages.

Example:

Copy
63function redeemAmountOf(address _from) public view returns (uint256 _redeemAmount)
64{
65    require(!released);
66    uint256 _sharesCount = balanceOf(_from);
67    uint256 _exitPrice = exitPrice();
68    return _exitPrice - _sharesCount * sharePrice;
69}

Recommendation:

We advise that proper error messages are included in these invocations to ensure that the conditions imposed are properly reflected.

Alleviation:

Error messages were properly introduced to the linked require checks.

SHA-04C: Redundant approve Invocation

Description:

The linked try-catch clause attempts to approve the address(this) variable which at the time of invocation is also the owner of the ERC721 token.

Example:

Copy
102wrapper._remove(_from, tokenId, remnant);
103try wrapper.target().approve(address(this), tokenId) {
104} catch (bytes memory /* _data */) {
105}
106wrapper.target().transferFrom(address(this), _from, tokenId);

Recommendation:

We advise the try-catch clause to be fully removed from the code as the transferFrom function allows an outgoing transfer to be initiated by the owner of the token without being approved.

Alleviation:

The Nftfy team has responded by stating that their intention is to support non-complicit ERC-721 implementations like CryptoKitties thus rendering this exhibit null.

SHA-05C: Ineffectual Event Emittances

Description:

The linked events are emitted after a _cleanup invocation which may result in the contract to destroy itself using the selfdestruct operation. The selfdestruct operation immediately halts execution and as such the events will be never emitted.

Example:

Copy
111function claim() public
112{
113    require(released);
114    address payable _from = msg.sender;
115    uint256 _sharesCount = balanceOf(_from);
116    require(_sharesCount > 0);
117    uint256 _claimAmount = vaultBalanceOf(_from);
118    assert(_claimAmount > 0);
119    _burn(_from, _sharesCount);
120    if (paymentToken == IERC20(0)) _from.transfer(_claimAmount);
121    else paymentToken.safeTransfer(_from, _claimAmount);
122    _cleanup();
123    emit Claim(_from, address(wrapper.target()), tokenId, address(this), _sharesCount);
124}

Recommendation:

We advise that the events are emitted prior to the invocation of the _cleanup function to ensure they are properly relayed off-chain.

Alleviation:

The statements were reordered so that the selfdestruct is executed as the last statement of the function call thus properly emitting events in this case.

SHA-06C: Redundant assert

Description:

The linked assert statement ensures that the shares of the user multiplied by the price are above zero, however, this is guaranteed by the fact that the preceding require ensures the user has a balance and that the price should canonically be non-zero as it breaks the functionality of the contract.

Example:

Copy
111function claim() public
112{
113    require(released);
114    address payable _from = msg.sender;
115    uint256 _sharesCount = balanceOf(_from);
116    require(_sharesCount > 0);
117    uint256 _claimAmount = vaultBalanceOf(_from);
118    assert(_claimAmount > 0);
119    _burn(_from, _sharesCount);
120    if (paymentToken == IERC20(0)) _from.transfer(_claimAmount);
121    else paymentToken.safeTransfer(_from, _claimAmount);
122    _cleanup();
123    emit Claim(_from, address(wrapper.target()), tokenId, address(this), _sharesCount);
124}

Recommendation:

We advise the assert statement to be omitted entirely.

Alleviation:

The Nftfy team responded by stating that it is their intention for the assert statements to act as supplemental documentation and thus chose to retain them in the codebase.