1

While I was skimming through open zeppelin's ERC721BasicToken.sol I came across a problem.

When setting the approval of a given operator

  function setApprovalForAll(address _to, bool _approved) public {
    require(_to != msg.sender);
    operatorApprovals[msg.sender][_to] = _approved;
    emit ApprovalForAll(msg.sender, _to, _approved);
  }

I've noticed that the visibility of the function is public. Doesn't it mean that anyone can modify an operator which was previously set before?

For example, I would create a contract that can control over a deployed ERC721Token contract.

contract TokenAccessor { 
  ERC721Token public nft;

  constructor(address _tokenAddress) public {
    nft= ERC721Token(_tokenAddress);
  }
}

And from the migrations script, I would pass in the address of someone's deployed ERC721 token contract.

module.exports = function (deployer) {
  deployer.deploy(TokenAccessor, "0x......")
    .then(() => {

This way, I think anyone can access setApprovalForAll to change the operator?

contract TokenAccessor { 
  ERC721Token public nft;

  constructor(address _tokenAddress) public {
    nft= ERC721Token(_tokenAddress);
  }

   function changeOperator() public {
    nft.setApprovalForAll(msg.sender, true);
   }
}

2 Answers 2

3

I've noticed that the visibility of the function is public. Doesn't it mean that anyone can modify an operator which was previously set before?

The public visibility just means that the function can be called from outside of the contract, ie, by another contract or from a wallet.

The function you mentioned, setApprovalForAll, is used to give another address permission to transfer all of your tokens. If it weren't public (or external) then nobody could use this function.

But the _to address specifies the address to which you want to grant permission, it doesn't specify the address that owns the tokens. The latter is inferred inside the function using msg.sender.

So no, you can't use this function to arbitrarily assign permissions, because you can't arbitrarily change msg.sender.

7
  • Thanks, I just noticed that I shouldn't have passed msg.sender. But you can still pass in the address of a TokenAccessor contract to _to address which works right?
    – bbusdriver
    Jul 9, 2019 at 20:10
  • 1
    Yes, in the setApprovalForAll function you provided, you can pass any address as _to as long as it's not the same as msg.sender. That includes a contract address. Jul 9, 2019 at 20:31
  • Ok, so I can change my changeOperator() to take address type changeOperator(address _contract) and pass the value to nft.setApprovalForAll(_contract, true); which means TokenAccessor contract is now the operator. If this is true, anyone can still change the existing operator right?
    – bbusdriver
    Jul 9, 2019 at 20:52
  • I'm not entirely sure what you're asking, or what you're trying to achieve. But it's worth noting that msg.sender is not preserved if a contract executes a function on another contract. So when your TokenAccessor calls nft.setApprovalForAll, the NFT contract will have TokenAccessor's address as msg.sender. Jul 9, 2019 at 20:56
  • That makes sense. All clear now thanks.
    – bbusdriver
    Jul 9, 2019 at 20:58
0

I think it's important to remember that:

require(_to != msg.sender);
operatorApprovals[msg.sender][_to] = _approved;

Basically prevents you from adding random addresses to be given permissions over other peoples tokens.

operatorApprovals[msg.sender][_to] means that you can only approve the token permissions, over tokens which YOU own. You can't just call the contract for other tokens owned by other addresses.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.