Omniscia NFTFY Audit

Shares Static Analysis Findings

Shares Static Analysis Findings

SHA-01S: Variable Shadowing

TypeSeverityLocation
Language SpecificMinorShares.sol:L44

Description:

The constructor arguments shadow the _name and _symbol variables of the ERC721 implementation.

Example:

contracts/Shares.sol
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 these variables are renamed so as not to cause a naming collission.

Alleviation:

The shadowing issue remains and we advise it to be remediated in accordance to best practice

SHA-02S: Inapplicacy of Checks-Effects-Interactions Pattern

TypeSeverityLocation
Standard ConformityMinorShares.sol:L95

Description:

The linked segment performs an arbitrary ETH transfer to the _from address enabling the target address to re-enter the contract should it be a smart contract.

Example:

contracts/Shares.sol
85function redeem() public payable
86{
87 require(!released);
88 address payable _from = msg.sender;
89 uint256 _paymentAmount = msg.value;
90 uint256 _sharesCount = balanceOf(_from);
91 uint256 _redeemAmount = redeemAmountOf(_from);
92 if (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 }
100 released = true;
101 if (_sharesCount > 0) _burn(_from, _sharesCount);
102 wrapper._remove(_from, tokenId, remnant);
103 try wrapper.target().approve(address(this), tokenId) {
104 } catch (bytes memory /* _data */) {
105 }
106 wrapper.target().transferFrom(address(this), _from, tokenId);
107 _cleanup();
108 emit Redeem(_from, address(wrapper.target()), tokenId, address(this));
109}

Recommendation:

We advise that the change of the calculation is instead transmitted near the end of the function before the evaluation of _cleanup to ensure that all storage adjustments occur before the external call takes place, conforming to the Checks-Effects-Interactions pattern.

Alleviation:

The nonReentrant function modifier was introduced in the functions instead. We should note that this does not protect against re-entrancies in other contracts that may rely on the Shares contract's storage and thus could be misled.