0

I'm trying to mint an NFT token in contract B by calling a method on contract A, but I want msg.sender (in contract B) to be the address that calls the function on A, not contract A's address.

I thought this should work, but it doesn't. (it reverts on onlyOwner(), etherscan says: Although one or more Error Occurred [Reverted] Contract Execution Completed)

What am I doing wrong here? If I remove onlyOwner on safeMint, it works. So it's definitely a problem with delegatecall. Any help appreciated. Thanks.

    contract A {
        using Counters for Counters.Counter;
        using SafeMath for uint256;
        Counters.Counter private _tokenIds;
        address public nftContract;

        constructor(address _nftContract) public {
         nftContract = _nftContract;
        }

        function mintItem() public returns (uint256) {
                _tokenIds.increment();
        
                uint256 newItemId = _tokenIds.current();
    
    nftContract.delegatecall(abi.encodeWithSignature("safeMint(address,uint256)",msg.sender,newItemid));
                }
}

contract B is ERC721 {

    address public owner;
    modifier onlyOwner() {
        require(owner == msg.sender, "Ownable: caller is not the owner");
        _;
    }

    constructor(string memory name,string memory ticker,string memory uri) ERC721(name,ticker) public {
      owner = msg.sender;
      _setBaseURI(uri);
    }

    function safeMint(address to, uint256 tokenId) public onlyOwner{
      _mint(to, tokenId);
    }

}
9
  • You're passing msg.sender as the first argument to function safeMint, not as its actual caller! What made you think it would somehow "become" the msg.sender when that function is called? For example, what did you think would happen to the second argument that you are passing? – goodvibration Sep 3 '20 at 16:52
  • And BTW, you're obviously much better off that way, otherwise, anyone else could just as well "forge" the caller (msg.sender) and call your onlyOwner protected function. – goodvibration Sep 3 '20 at 16:54
  • How to pass it as it's caller? I do need to pass it as the first argument too... – hdries Sep 3 '20 at 16:55
  • Read my second comment above. – goodvibration Sep 3 '20 at 16:55
  • If you want contract A to be the owner of the contract which implements this function, then you need to create that contract from a function in contract A. – goodvibration Sep 3 '20 at 16:57
1

You should not use delegatecall for your use case except if you want to modify contract A storage and not contract B. Generally, do not use delegatecall unless you really know what you are doing.

You should use tx.origin if you want the initial sender of the tx and not the function caller (here contract B).

You can modify your require like that:

modifier onlyOwner() {
     require(owner == msg.sender || owner == tx.origin, "Ownable: caller is not the owner");
     _;
}
2
  • 2
    Vitalik himself, in the flesh (or at least in the digitized flash), recommended not to use tx.origin. – goodvibration Sep 4 '20 at 14:46
  • I ended up not using delegatecall, and in contract B I've set contract A address as allowed to call safeMint in contract B. – hdries Sep 6 '20 at 7:39

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.