Always clearApproval on removeNFToken #50

[dafky2000]

Fixed #50

[dafky2000]

I just noticed that this could have been exploited via NFTokenEnumerable->removeNFToken as well.

[xpepermint]

Thanks, @dafky2000. Can you please clarify your last comment?

[dafky2000]

If NFTokenEnumerable is used (NFTokenEnumerableMock for example) and NFTokenEnumerable->removeNFToken is executed, the previous approval will not be removed (without this PR) .

  function removeNFToken(
    address _from,
    uint256 _tokenId
  )
   internal
  {
    super.removeNFToken(_from, _tokenId);
    assert(ownerToIds[_from].length > 0);

    uint256 tokenToRemoveIndex = idToOwnerIndex[_tokenId];
    uint256 lastTokenIndex = ownerToIds[_from].length.sub(1);
    uint256 lastToken = ownerToIds[_from][lastTokenIndex];

    ownerToIds[_from][tokenToRemoveIndex] = lastToken;
    ownerToIds[_from][lastTokenIndex] = 0;

    ownerToIds[_from].length--;
    idToOwnerIndex[_tokenId] = 0;
    idToOwnerIndex[lastToken] = tokenToRemoveIndex;
  }

[dafky2000]

So this PR should either be accepted OR change the NFTokenEnumerable->removeNFToken to read

function removeNFToken(
    address _from,
    uint256 _tokenId
  )
   internal
  {
+    clearApproval(_from, _tokenId);
    super.removeNFToken(_from, _tokenId);
    assert(ownerToIds[_from].length > 0);

    uint256 tokenToRemoveIndex = idToOwnerIndex[_tokenId];
    uint256 lastTokenIndex = ownerToIds[_from].length.sub(1);
    uint256 lastToken = ownerToIds[_from][lastTokenIndex];

    ownerToIds[_from][tokenToRemoveIndex] = lastToken;
    ownerToIds[_from][lastTokenIndex] = 0;

    ownerToIds[_from].length--;
    idToOwnerIndex[_tokenId] = 0;
    idToOwnerIndex[lastToken] = tokenToRemoveIndex;
  }

[xpepermint]

We had a long discussion about this small change. Though the proposal is valid it does not follow the single responsibility principle which we try to follow. Private and internal methods are desired to have a single responsibility. Methods clearApproval and removeNFToken are both internal and thus represent helper methods.

It's really up to the developer to make sure his/her code is secure. A developer can always override methods and do bad things :). We decided to update the documentation with a @notice which warns developers to be cautious (e.g. Use this function with caution ...).

[xpepermint]

@dafky2000 here's the PR: #54