Internal ownedTokensIndex not properly cleared when removing a token from its owner in ERC721

[spalladino]

🎉 Description

See this comment for more info. In the scenario that a token is removed from its owner, if that token was in the last index of the owner's array, the ownedTokensIndex keeps a reference to the token that should be deleted.

This does not cause any issues in the external interface of the contract, but the data structure it should be properly cleared. Thanks @mudgen for catching it.

• 🐛 This is a bug report.

[nventuro]

I'm inclining towards not fixing this 'bug', for the following reasons:

1. I argue there isn't really a bug in the first place, since the Solidity interface is not affected by it.
2. The 'issue' is that a deleted token's index in its owner's array (_ownedTokensIndex) is not cleared. However, 'clear' simply means 'setting to 0', and 0 is a valid index value. This means that there's no potential for this 'tyding-up' to prevent future bugs.
3. This data structure is only ever used (read from) when deleting tokens in _removeTokenFrom. Any deleted token must obviously first be created, at which point the index will be set to the correct value, yielding zero negative consequences for not clearing the index, and increased gas costs when doing it (both for the SSTORE that sets it to 0, and the SSTORE that changes it from 0 to a different value, which is even more expensive).

[nventuro]

This is related to #1541, where I propose getting rid of more cleanup of this sort.

[frangio]

Actually the additional SSTORE should result in a cheaper transaction because it's zeroing out a storage slot. @nventuro and I have been doing some tests though and it doesn't seem to be that way. We're still not sure why but will keep looking into it.

[nventuro]

Yup, my comment was based on tests, not my understanding of the platform (which is clearly wrong, since I'd expect the zero-ing out to refund 10k gas).

Continuing the discussion on #1541 to avoid having two threads.