Add Governor extension GovernorNoncesKeyed to use NoncesKeyed for vote by sig

[arr00]

Fixes #5565

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 4b4b43e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[tushar994]

lgtm

[Amxx]

I like the new version!

We'll need a changelog entry and full coverage, but this will be for 5.4 anyway so no rush.

[0xjingle]

what about adding stress test edge cases (e.g. signature replay on multiple proposals) or extend the extension to support batch signatures per user?

[ernestognw]

I like the idea of abstracting away the signature validation. Essentially, we're trying to add a feature to how the signature digest is generated (i.e. the keyed nonce).

Currently, the contract has 2 different typed signatures: BALLOT_TYPEHASH and EXTENDED_BALLOT_TYPEHASH, and given that it's not possible to extend EIP-712 types, then it's impossible to build a unified castVoteBySig function, therefore we have castVoteWithReasonAndParamsBySig.

Similar to how there are two functions for casting a signature vote, I feel the best abstraction is to abstract away internal _validateVoteSig and _validateExtendedVoteSig. Essentially, we would have the following in Governor.sol:

abstract contract Governor is ... {
    function castVoteBySig(
        uint256 proposalId,
        uint8 support,
        address voter,
        bytes memory signature
    ) public virtual returns (uint256) {
        bool valid = _validateVoteSig(proposalId, support, voter, signature);
        if (!valid) {
            revert GovernorInvalidSignature(voter);
        }
        return _castVote(proposalId, voter, support, "");
    }

    function castVoteWithReasonAndParamsBySig(
        uint256 proposalId,
        uint8 support,
        address voter,
        string calldata reason,
        bytes memory params,
        bytes memory signature
    ) public virtual returns (uint256) {
        bool valid = _validateExtendedVoteSig(proposalId, support, voter, reason, params, signature);
        if (!valid) {
            revert GovernorInvalidSignature(voter);
        }
        return _castVote(proposalId, voter, support, reason, params);
    }

    function _validateVoteSig(
        uint256 proposalId,
        uint8 support,
        address voter,
        bytes memory signature
    ) internal virtual returns (bool) {
        return
            SignatureChecker.isValidSignatureNow(
                voter,
                keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter))),
                signature
            );
    }

    function _validateExtendedVoteSig(
        uint256 proposalId,
        uint8 support,
        address voter,
        string memory reason,
        bytes memory params,
        bytes memory signature
    ) internal virtual returns (bool) {
        return
            SignatureChecker.isValidSignatureNow(
                voter,
                keccak256(
                    abi.encode(
                        EXTENDED_BALLOT_TYPEHASH,
                        proposalId,
                        support,
                        voter,
                        _useNonce(voter),
                        keccak256(bytes(reason)),
                        keccak256(params)
                    )
                ),
                signature
            );
    }
}

Then, building a GovernorNoncesKeyed becomes just an override to both _validateVoteSig and _validateExtendedVoteSig:

abstract contract GovernorNoncesKeyed is Governor, NoncesKeyed {
    function _useCheckedNonce(address owner, uint256 nonce) internal virtual override(Nonces, NoncesKeyed) {
        super._useCheckedNonce(owner, nonce);
    }

    function _validateVoteSignature(
        uint256 proposalId,
        uint8 support,
        address voter,
        bytes memory signature
    ) internal virtual override returns (bool) {
        bool valid = SignatureChecker.isValidSignatureNow(
            voter,
            keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, nonces(voter, uint192(proposalId)))),
            signature
        );
        if (valid) _useNonce(voter, uint192(proposalId));
        else valid = super._validateVoteSignature(proposalId, support, voter, signature);
        return valid;
    }

    function _validateExtendedVoteSignature(
        uint256 proposalId,
        uint8 support,
        address voter,
        string memory reason,
        bytes memory params,
        bytes memory signature
    ) internal virtual override returns (bool) {
        bool valid = SignatureChecker.isValidSignatureNow(
            voter,
            keccak256(
                abi.encode(
                    EXTENDED_BALLOT_TYPEHASH,
                    proposalId,
                    support,
                    voter,
                    nonces(voter, uint192(proposalId)),
                    keccak256(bytes(reason)),
                    keccak256(params)
                )
            ),
            signature
        );
        if (valid) _useNonce(voter, uint192(proposalId));
        else valid = super._validateExtendedVoteSignature(proposalId, support, voter, reason, params, signature);
        return valid;
    }
}

I acknowledge that the internal functions would be missing calling super if the first signature is valid. However, I think the pattern of internal functions that return booleans is the only pattern where we allow bypassing a call to super as long as it's noted. In this case, I think it's acceptable as well.

[Amxx]

Approving in the current format, but open to any refactor either in this PR, or in another PR before 5.4