Omniscia NFTFY Audit
Shares Static Analysis Findings
Shares Static Analysis Findings
SHA-01S: Variable Shadowing
Type | Severity | Location |
---|---|---|
Language Specific | Minor | Shares.sol:L44 |
Description:
The constructor
arguments shadow the _name
and _symbol
variables of the ERC721
implementation.
Example:
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) public45{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
Type | Severity | Location |
---|---|---|
Standard Conformity | Minor | Shares.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:
85function redeem() public payable86{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.