A security hole!

[euglena]

Hello Loom team,
I like your course very much. Thanks. But I found a security hole in this code which could probably cause fatal problems in an application.
Look at the code in Lesson 5 Chapter 8.
Imagine there are 3 persons: A, B and C. A owned a zombie. First, A approved that zombie to B, and then A transferred it to C in an exchange. What if B executed "takeOwnership" of the same zombie after that? Yes! He/she would succeed and got ownership of it! And poor C lost his/her zombie without knowing why....
We should add some code to cancel the approval of the zombie in the same time it is transferred.

[zoeleesss]

Great discovery, bro.
I think i know to how to fix the issue you mentioned.

1. Add mapping (uint=>boolean) isApproved. in zombiefactory.sol

2. Add isApproved[_tokenId] = false; every time we create a new zombie to make sure the initial boolean value is false;

3. Add isApproved[_tokenId] = true;when this token is in approvedstatus in function approve(..)

4. Add if ( msg.sender==zombieToOwner[_tokenId] ) {require( isApproved[_tokenId]== false); }
   when this token is being transferred in function _transfer(..) . This will avoid the owner to transfer again.

5. Add isApproved[_tokenId] == false; to the last line of function takeOwnership(..). This will make sure the new owner can transfer this token now.

If there is any question to this fix or if just want wanna talk about technologies of blockchain, feel free to contact me [email protected]. Thx!

[zoeleesss]

The solution above is to make sure the approved user has the right to own the token and the owner does not have the right to transfer again.
But a possible scenario is that owner can override the function approve(..) before the approved person uses function takeOwnership(..). So in this case, we need to devise another solution to the problem.

And this is way more easy with much cleaner code:

function _transfer(address _from, address _to, uint256 _tokenId) private {
    ownerZombieCount[_to] = ownerZombieCount[_to].add(1);
    ownerZombieCount[msg.sender] = ownerZombieCount[msg.sender].sub(1);
    zombieToOwner[_tokenId] = _to;

    // This is where we add:
    delete zombieApprovals[_tokenId];
    // That's it! Only one line code to make sure the any previous approval info about this token is gone. 

    Transfer(_from, _to, _tokenId);
  }

We also need to add this line delete zombieApprovals[_tokenId]; in function takeOwnership (..)
just in case the new owner ( who uses this function to become the new owner ) calls this function again and again and end up costing unnecessary 💵 💰

The idea was made by cryptokitties.co

The original code below:

 /// @dev Assigns ownership of a specific Kitty to an address.
    function _transfer(address _from, address _to, uint256 _tokenId) internal {
        // Since the number of kittens is capped to 2^32 we can't overflow this
        ownershipTokenCount[_to]++;
        // transfer ownership
        kittyIndexToOwner[_tokenId] = _to;
        // When creating new kittens _from is 0x0, but we can't account that address.
        if (_from != address(0)) {
            ownershipTokenCount[_from]--;
            // once the kitten is transferred also clear sire allowances
            delete sireAllowedToAddress[_tokenId];
            // clear any previously approved ownership exchange
            delete kittyIndexToApproved[_tokenId];
        }
        // Emit the transfer event.
        Transfer(_from, _to, _tokenId);
    }

Don't forget to thumps up! 👍 👍 💯 💯 👍 👍
Thx again!

[euglena]

I think it works just adding
delete zombieApprovals[_tokenId];
in both transfer and takeOwnership.

[zoeleesss]

@euglena Ha. Yes.