Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update zombieownership.sol #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

webcrafters
Copy link

For our specified use cases, this should help in preventing honest mistakes of users of the contract.

I may invoke transferFrom(A, B, Z) and be the owner of Z, but if A is not my correct address, the code will reduce someone else's zombieCount and also emit a Transfer event that's not accurate in what it says.

For our specified use cases, this should help in preventing honest mistakes of users of the contract.

I may invoke transferFrom(A, B, Z) and be the owner of Z, but if A is not my correct address, the code will reduce someone else's zombieCount and also emit a Transfer event that's not accurate in what it says.
@BrandonNoad
Copy link

👍 I noticed this bug as well while completing the lesson.

@@ -23,7 +23,7 @@ contract ZombieOwnership is ZombieAttack, ERC721 {
}

function transferFrom(address _from, address _to, uint256 _tokenId) external payable {
require (zombieToOwner[_tokenId] == msg.sender || zombieApprovals[_tokenId] == msg.sender);
require ((zombieToOwner[_tokenId] == msg.sender && _from == msg.sender) || (zombieApprovals[_tokenId] == msg.sender && _to == msg.sender));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in both cases you need to check zombieToOwner[_tokenId] == _from.

So the fix would be:

require (zombieToOwner[_tokenId] == _from && (msg.sender == _from || (msg.sender == _to && zombieApprovals[_tokenId] == msg.sender)));

@luizchagasjardim
Copy link

I noticed this also and came to see if it was discussed. I think the best place to verify the ownership is in the _transfer function, that way developers don't have to remember to check before calling it:
require(zombieToOwner[_tokenId] == _from)
then for transferFrom, you just need _from == msg.sender in the first half

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants