Overriding onlyGovernance modied functions with a role via onlyRole()

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:

  1. 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?

  2. 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?

  3. 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.

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

:computer: Environment

Hardhat & Waffle

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.

1 Like

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 !

1 Like

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

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!

1 Like