Check Zombie ownership in Lesson 5 Chapter 6

[kashish5599]

An additional check is needed to check whether zombie belong to owner with address _from.
Consider the case:
Owner A owns zombie with id Z.
A wants to transfer Z to a new owner B.
C is another owner (victim).
He can call the function transfer(address of C, address of B, Z).
This is a vulnerability not mentioned anywhere.
We can prevent this by changing _transfer function to:

function _transfer(address _from, address _to, uint256 _tokenId) private {
   require(zombieToOwner[_tokenId] == _from);
   ownerZombieCount[_to]++;
   .
   .
   .
}

[0xdx-xb]

I was curious about this scenario as well, so what a coincidence it was at the top of the issues list! Took some time to think through it, and here's where I ended up. I'm still learning, so take this all with a grain of salt.

Functionality is wrapped up by Chapter 8, so I'll write this comment based on the state of code there. I think this does cause an incorrect message to be emitted by Transfer, but doesn't actually cause the token to be sent to the wrong address. I would probably classify this as bug instead of a vulnerability.

Here's my reasoning:

1. _transfer is private, so it's not callable directly.
2. transferFrom(_from, _to, _tokenId) requires that the _tokenId being transferred is either owned by msg.sender or has been approved for transfer by msg.sender. This means that the _tokenId cannot be sent by an address other than the owner or an address the owner has approved.
3. approve can only be called by _tokenId's owner.

We could fix the bug by including require(zombieToOwner[_tokenId] == _from); as you suggest by elaborating on the logic in Chapter 5 or by adding another chapter to draw attention to subtle bugs like this afterward we finish approval logic in Chapter 8.

Now there's no restriction that receiving address has to be approved, so an approved sender could send the token to a third address by passing a different address than their own. Depending on the implementer's intention, this could be either a bug (owners can only approve someone else to claim for themselves) or a feature (third-party management). Could be interesting and helpful for someone new to programming to have the nuance discussed, but may be more detailed at this point in the tutorial than helpful. Bonus section perhaps?