0

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

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.

  • 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 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. – AnAllergyToAnalogy Jul 9 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 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. – AnAllergyToAnalogy Jul 9 at 20:56
  • That makes sense. All clear now thanks. – bbusdriver Jul 9 at 20:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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