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);
}




}
hdries
  • 31
  • 6
  • 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 03 '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 03 '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 03 '20 at 16:55
  • Read my second comment above. – goodvibration Sep 03 '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 03 '20 at 16:57
  • If someone else would call it, it would revert, because the caller is not owner. Anyway, this is a mock example, on contract A there is a check, it's actually an internal function, but that's not really relevant for why delegatecall does not do what I expect it to do. – hdries Sep 03 '20 at 16:58
  • Yes, but if you could forge it like the way you are trying to, then so could anyone else. They would simply pass your address instead of their own address! I've explained to you what your delegate call does, and why you cannot forge the address of the caller (aka msg.sender). Hope you understand that, because it's a pretty fundamental concept in the Ethereum blockchain. – goodvibration Sep 03 '20 at 17:05
  • I think you are confusing call with delegatecall, see the schematics here: https://medium.com/coinmonks/delegatecall-calling-another-contract-function-in-solidity-b579f804178c#:~:text=DelegateCall%2C%20as%20the%20name%20implies,contract%20but%20on%20caller%20contract. – hdries Sep 03 '20 at 18:16
  • Let us continue this discussion in chat. – hdries Sep 03 '20 at 18:25

1 Answers1

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");
     _;
}
Tclairet
  • 730
  • 4
  • 10