Overriding onlyGovernance modied functions with a role via onlyRole()

DINDX · February 3, 2022, 8:45pm

Hello, firstly I wanted to thank everyone at OpenZeppelin for the incredible work you guys do, and for all of help you provide through this forum! I apologize if I screw anything up, this is my first post anywhere.

I have a few questions:

Does it make sense to create a role with the intention of not giving it to anybody, and replacing the onlyGovernance modifier on functions with this role so those functions can't be called at all through proposal executions? Is there a better way of doing this?
If I understand correctly, since I'm not using a timelock I shouldn't need to override _executor() in Governor.sol as my contract's address will already be set as the _executor() through inheriting Governor.sol?
Along the same lines, if I do not need to override _executor() then I shouldn't be worried about relay() within Governor.sol, as it is only useful in interacting with separate timelock contracts?

Below is my code, modified after using the Governor tab of the contract creation wizard. In order to get GovernorPreventLateQuorum.sol working I manually copied it to my node_modules directory.

Code to reproduce

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.2;

import "@openzeppelin/contracts/access/AccessControl.sol"; 
import "@openzeppelin/contracts/governance/Governor.sol";
import "@openzeppelin/contracts/governance/extensions/GovernorCountingSimple.sol";
import "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol";
import "@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFraction.sol";
import "@openzeppelin/contracts/governance/extensions/GovernorPreventLateQuorum.sol";

contract Voting is AccessControl, Governor, GovernorCountingSimple, GovernorVotes, GovernorVotesQuorumFraction, GovernorPreventLateQuorum {
    
    // Give BLOCKED_ROLE to function overrides of functions using onlyGovernance that I do not want to be changed
    bytes32 public constant BLOCKED_ROLE = keccak256("BLOCKED_ROLE");

    constructor(ERC20Votes _DINDX_TOKEN_ADDRESS)
        Governor("Voting")
        GovernorVotes(_DINDX_TOKEN_ADDRESS)
        GovernorVotesQuorumFraction(20)
        // 2 days = 13091 blocks
        GovernorPreventLateQuorum(13091)
    {}

    function votingDelay() public pure override returns (uint256) {
        return 45818; // 1 week 
    }

    function votingPeriod() public pure override returns (uint256) {
        return 91636; // 2 weeks
    }

    // The following functions are overrides required by Solidity
    function quorum(uint256 blockNumber)
        public
        view
        override(IGovernor, GovernorVotesQuorumFraction)
        returns (uint256)
    {
        // Explicit override
        return GovernorVotesQuorumFraction.quorum(blockNumber);
    }

    function getVotes(address account, uint256 blockNumber)
        public
        view
        override(IGovernor, GovernorVotes)
        returns (uint256)
    {
        // Explicit override
        return GovernorVotes.getVotes(account, blockNumber);
    }

    function proposalDeadline(uint256 proposalId)
        public
        view
        override(Governor, GovernorPreventLateQuorum)
        returns (uint256)
    {
        // Explicit override
        return GovernorPreventLateQuorum.proposalDeadline(proposalId);
    }

    function _castVote(uint256 proposalId, address account, uint8 support, string memory reason)
        internal
        override(Governor, GovernorPreventLateQuorum)
        returns (uint256)
    {
        // Explicit override
        return GovernorPreventLateQuorum._castVote(proposalId, account, support, reason);
    }

    // Set the _executor() address of the onlyGovernance modifier to this contract's address, as Voting.sol will execute
    // updateQuorumNumerator via successful proposals
    // Is this required? I think inheriting Governor.sol would set this automatically?
    function _executor()
        internal
        view
        override(Governor)
        returns (address)
    {
        return address(this);
    }
    
    // Allow proposals to provide a newQuorumNumerator between 4% and 20%, and onlyGovernance is used
    function updateQuorumNumerator(uint256 newQuorumNumerator)
        external
        onlyGovernance
        override(GovernorVotesQuorumFraction)
    {
        require(newQuorumNumerator <= 20, "MAXQ"); // MAX QUORUM
        require(newQuorumNumerator >= 4, "MINQ"); // MIN QUORUM
        _updateQuorumNumerator(newQuorumNumerator);
    }

    // Override supportsInterface() of AccessControl.sol
    function supportsInterface(bytes4 interfaceId)
        public
        view
        override(AccessControl, Governor)
        returns (bool)
    {
        return Governor.supportsInterface(interfaceId);
    }   

    // Is overriding this redundant if the _executor() address is this contract's address and a timelock is not used?
    // Replace onlyGovernance with onlyRole(BLOCKED_ROLE) so it can't be modified via proposals
    function relay(address target, uint256 value, bytes calldata data)
        external
        onlyRole(BLOCKED_ROLE)
        //override(Governor) // Why is this override of a Governor.sol function not required, when it was for
        //_executor() above? Generates the error: "TypeError: Invalid contract specified in override list: "Governor"."
    {
        Address.functionCallWithValue(target, data, value);
    }

    // Replace onlyGovernance with onlyRole(BLOCKED_ROLE) so it can't be modified via proposals
    function setLateQuorumVoteExtension(uint64 newVoteExtension)
        public
        onlyRole(BLOCKED_ROLE)
        override(GovernorPreventLateQuorum)
    {
        _setLateQuorumVoteExtension(newVoteExtension);
    }
        
}

Environment

Hardhat & Waffle

Amxx · February 4, 2022, 9:39am

Hello @DINDX

Possibly. I'm not sure I understand what you are trying to achieve. If you want to disable a function from ever being executed by anyone it might be clearer to not use AccessControl and just do

function someFunction(...) public override {
    revert("disabled");
}

If you are using a timelock, you should have a look at the GovernorTimelockCompound and GovernorTimelockControl modules. They will bind you governor to only run operations through the timelock, and will override the _executor() function accordingly.
If you don't use these module, you governor will be able to send transaction directly to any target, or through the timelock to any contract controlled by the timelock. I'm not sure what you'd like the executor to be in that situation.

relay() is onlyGovernance, which means that in the current version is will only accept calls made by the executor (there are planes to slightly modify the restrictions). If the executor is the governor, than that function can be called when executing a proposal (though it is useless in that case). If the executor is the timelock (for example using one of the timelock module), then it allows a proposal to go through the timelock, and come back to the governor to do things like transfer ERC20 token held by the governor.

I hope this helps you.

Amxx · February 4, 2022, 9:42am

Also note that in your code:

You shouldn't need to override _executor as it is already defined in Governor and already returns address(this).
In many cases you override name the module explicitly rather then doing a super call. While that is not necessarily an issue, I believe it is safer to just use super calls. If you find a case where doing super doesn't achieve the expected behavior, please file an issue / bug report !

DINDX · February 4, 2022, 5:54pm

Thank you very much for the quick and detailed replies @Amxx! I will reply to both here:

Amxx:

Possibly. I'm not sure I understand what you are trying to achieve. If you want to disable a function from ever being executed by anyone it might be clearer to not use AccessControl and just do
function someFunction(...) public override {
    revert("disabled");
}

This was exactly what I was trying to do, and using revert() is a much better solution than using AccessControl.

I was confused before regarding the timelock, but I am not using one and your second reply answered it perfectly- there is no need to override _executor() in my case.

As I'm not using a timelock you are correct in that overriding replay() has no purpose.

I have had no issues with using super, this was just a case of me trying to make it easier to read and follow along, but I will take your thoughts regarding safety into consideration!

Your responses have been immensely helpful to me, and thank you again for taking the time to do so @Amxx!

eeshenggoh · December 10, 2023, 3:37am

Will there be any issues if you use a Core contract to handle Access Control? And in the Governor Contract I set as Timelock instance and not GuildTimelockController . Will there be any issues?

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {CoreRef} from "@src/core/CoreRef.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol";

/// @title An override of the regular OZ governance/TimelockController to allow uniform
/// access control in the ECG system based on roles defined in Core.
/// @dev The roles and roles management from OZ access/AccessControl.sol are ignored, we
/// chose not to fork TimelockController and just bypass its access control system, to
/// introduce as few code changes as possible on top of OpenZeppelin's governance code.
/// @author eswak

    contract GuildTimelockController is TimelockController, CoreRef {
    constructor(
        address _core,
        uint256 _minDelay
    )
        CoreRef(_core)
        TimelockController(
            _minDelay,
            new address[](0), //  Should this be 0?
            new address[](0),
            address(0)
        )
    {}

    /// @dev override of OZ access/AccessControl.sol inherited by governance/TimelockController.sol
    /// This will check roles with Core, and not with the storage mapping from AccessControl.sol
    function hasRole(
        bytes32 role,
        address account
    ) public view virtual override returns (bool) {
        return core().hasRole(role, account);
    }

    /// @dev override of OZ access/AccessControl.sol, noop because role management is handled in Core.
    function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal override {}

    /// @dev override of OZ access/AccessControl.sol, noop because role management is handled in Core.
    function _grantRole(bytes32 role, address account) internal override {}

    /// @dev override of OZ access/AccessControl.sol, noop because role management is handled in Core.
    function _revokeRole(bytes32 role, address account) internal override {}
}

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {Governor, IGovernor} from "@openzeppelin/contracts/governance/Governor.sol";
import {GovernorSettings} from "@openzeppelin/contracts/governance/extensions/GovernorSettings.sol";
import {GovernorTimelockControl} from "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol";
import {GovernorVotes, IERC165} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol";
import {GovernorCountingSimple} from "@openzeppelin/contracts/governance/extensions/GovernorCountingSimple.sol";
import {IVotes} from "@openzeppelin/contracts/governance/utils/IVotes.sol";
import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol";
import {CoreRef} from "@src/core/CoreRef.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";

/// @title Governor for on-chain governance of Ethereum Credit Guild, based on the OZ implementation.
/// @author eswak
contract GuildGovernor is
    CoreRef,
    Governor,
    GovernorVotes,
    GovernorTimelockControl,
    GovernorSettings,
    GovernorCountingSimple
{
    /// @notice Private storage variable for quorum (the minimum number of votes needed for a vote to pass).
    uint256 private _quorum;

    /// @notice Emitted when quorum is updated.
    event QuorumUpdated(uint256 oldQuorum, uint256 newQuorum);

    constructor(
        address _core,
        address _timelock,
        address _token,
        uint256 initialVotingDelay,
        uint256 initialVotingPeriod,
        uint256 initialProposalThreshold,
        uint256 initialQuorum
    )
        CoreRef(_core)
        Governor("ECG Governor")
        GovernorVotes(IVotes(_token))

        GovernorTimelockControl(TimelockController(payable(_timelock)))
        GovernorSettings(
            initialVotingDelay,
            initialVotingPeriod,
            initialProposalThreshold
        )
    {
        _setQuorum(initialQuorum);
    }

Topic		Replies	Views
How to restrict function calls to only governance process Contracts	11	843	April 27, 2023
If I use the 'optimizer' to optimize the code, is there any problem in using all the functions provided by Openzeppelin's Governor? Contracts	1	462	July 15, 2022
Clarification of OpenZeppelin Governor contract is required Contracts	1	463	August 4, 2022
Help with Governor 5.x contracts Contracts	4	445	December 12, 2023
Openzeppelin Governance Module Queuing Revert Issue Contracts	4	676	April 4, 2022

Overriding onlyGovernance modied functions with a role via onlyRole()

Code to reproduce

Environment

Related topics