Change default owner/admin accounts to tx.origin rather than msg.sender

[pcaversaccio]

🧐 Motivation
Some background: I'm the author of the Hardhat plugin xdeployer that helps to deploy smart contracts across multiple EVM chains with the same deterministic address (keyword CREATE2 opcode). Since opcodes can only be called via smart contracts, the default owner of a deployed contract with CREATE2 (and my plugin) will be a contract address (using your default npm package as dependency) which is obviously not good. How about setting the default owner to tx.origin and make everything more agnostic? Like that it can be ensured that the original EOA user will always be the owner. The same applies to the default roles in your OpenZeppelin Wizard for DEFAULT_ADMIN_ROLE, PAUSER_ROLE, MINTER_ROLE:

    constructor() ERC20("MyToken", "MTK") {
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(PAUSER_ROLE, msg.sender);
        _grantRole(MINTER_ROLE, msg.sender);
    }

I'm fully aware of the security considerations re tx.origin (https://docs.soliditylang.org/en/v0.8.11/security-considerations.html?highlight=tx.origin#tx-origin), however, in this case, we would not touch the authorization but rather "only" the constructor argument.

[panpansh]

yes using tx.origin for constructor only seems ok

[Amxx]

Hello @pcaversaccio

The wizard's role is to produce templates / first draft, that users can modify depending on their needs. In this case, it would be easy to do any of the following:

• replace msg.sender for tx.origin
• add a argument to the constructor, that is the admin to use

@frangio: In the wizard when access control is enabled, maybe we want to add an optional field "admin" that would replace msg.sender ?

[pcaversaccio]

@Amxx yes agreed - but a template / first draft should be generic as much as possible (same goes for the default npm package) and my proposal would make everything more generic so that contracts can't be owners by default (contracts can never be the tx.origin).

For Ownable.sol I would suggest the following change (happy to submit a PR later):

    /**
     * @dev Initializes the contract setting the deployer as the initial owner.
     */
    constructor() {
        _transferOwnership(tx.origin);
    }

As far as I've seen, there is no need to change Context.sol, since for meta-transactions msgSender() is only used. If however it is accessed via other inheritances at some places, we could simply add the following line as well to Context.sol:

    function _txOrigin() internal view virtual returns (address) {
        return tx.origin;
    }

This would change the first snippet in the following way:

    /**
     * @dev Initializes the contract setting the deployer as the initial owner.
     */
    constructor() {
        _transferOwnership(_txOrigin());
    }

I've thought through all tx.origin vulnerabilities and I don't think we introduce one here, but any other opinion on this matter is highly appreciated.

[frangio]

We disagree that it's okay to use tx.origin in the constructor, at least in general. What you really want for the scenario you describe is to use the address of the caller to the factory contract, but this is not the same as tx.origin, because the caller to the factory contract may be another smart contract that should become the owner of the newly created contract.

This is always going to be a problem with factories and the factory created contract needs to be aware of that and/or offer a way to initialize it to the right values. For example, with an Ownable contract you can initialize the contract with a call to transferOwnership.

This is a proposal for a more general solution for the Wizard:

• Add a toggle to use msg.sender or constructor parameter for roles/ownership contracts-wizard#73

[Amxx]

We already had some request to change the Ownable constructor to

constructor(address admin) {
        _transferOwnership(admin);
}

IMO this would be much better then using tx.origin, yet we rejected this proposition because it would be a breaking change. We are VERY careful about any breaking change, which you proposal would be. Also we are, like most of the community, against using tx.origin. This has been the source of a lot of bugs.

If you really want to set tx.origin has the original owner (I would advice against it) you always have the possibility to do.

contract YourContract is Ownable {
    constructor() {
        transferOwnership(tx.origin);
    }
}

[defido]

IMO this would be much better then using tx.origin, yet we rejected this proposition because it would be a breaking change. We are VERY careful about any breaking change, which you proposal would be. Also we are, like most of the community, against using tx.origin. This has been the source of a lot of bugs.

Think OP has a good proposal. Think it would be worth the change. +1 Setting owner should be simpler. Setting tx.origin as owner for factories is a must.