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

Add require check for token owner matching _from address in 05-erc721-5.md #621

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

Conversation

cooldc314
Copy link

No description provided.

@andreipope
Copy link
Collaborator

@cooldc314 I'm a bit lost. Please explain what this pull request aims to fix.

@andreipope andreipope added the question Further information is requested label Feb 24, 2022
@cooldc314
Copy link
Author

It should always be invalid to transfer from an address that does not match the token owner's address. Even with the additional require checks implemented for the public functions, currently the final solution allows someone to transfer a token they own but spoof the _from address, thus decreasing the zombie count incorrectly for any address.

@nonik0
Copy link

nonik0 commented Apr 7, 2022

+1 to fixing this, I noticed this as well.

Here's a high-level example: if I call transferFrom(myEnemyAddress, mySecondaryAddress, myZombie) I can transfer my zombie to another account I own, but I also decrement the ownerZombieCount of my enemy. I can do this because there's no check on the _from argument (myEnemyAddress), only the _tokenId (myZombie) argument. Rinse and repeat.

Regarding the actual changes to the contract code, I think it makes most sense to update the transferFrom function which already has the existing require conditions for the token owner/approval. Updated function would look like:

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

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

Successfully merging this pull request may close these issues.

3 participants