diff --git a/.changeset/small-cars-appear.md b/.changeset/small-cars-appear.md new file mode 100644 index 00000000000..0263bcd1854 --- /dev/null +++ b/.changeset/small-cars-appear.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ECDSA`: Add a function `toDataWithIntendedValidatorHash` that encodes data with version 0x00 following EIP-191. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index e3691a3ed3a..1483da6d2b3 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -336,10 +336,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive /** * @dev See {IGovernor-cancel}. */ - function cancel(uint256 proposalId) public virtual override { + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual override returns (uint256) { + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); require(state(proposalId) == ProposalState.Pending, "Governor: too late to cancel"); require(_msgSender() == _proposals[proposalId].proposer, "Governor: only proposer can cancel"); - _cancel(proposalId); + return _cancel(targets, values, calldatas, descriptionHash); } /** @@ -407,16 +413,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes[] memory calldatas, bytes32 descriptionHash ) internal virtual returns (uint256) { - return _cancel(hashProposal(targets, values, calldatas, descriptionHash)); - } + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - /** - * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as - * canceled to allow distinguishing it from executed proposals. - * - * Emits a {IGovernor-ProposalCanceled} event. - */ - function _cancel(uint256 proposalId) internal virtual returns (uint256) { ProposalState status = state(proposalId); require( diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index b12d2f63227..70f81efe815 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -235,12 +235,17 @@ abstract contract IGovernor is IERC165, IERC6372 { ) public payable virtual returns (uint256 proposalId); /** - * @dev Cancel a proposal. This is restricted to Pending proposal (before the vote starts) and is restricted to - * the proposal's proposer. + * @dev Cancel a proposal. A proposal is cancellable by the proposer, but only while it is Pending state, i.e. + * before the vote starts. * * Emits a {ProposalCanceled} event. */ - function cancel(uint256 proposalId) public virtual; + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual returns (uint256 proposalId); /** * @dev Cast a vote diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 1b8c0936817..39c895bf092 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -77,29 +77,55 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp * @dev See {IGovernorCompatibilityBravo-queue}. */ function queue(uint256 proposalId) public virtual override { - ProposalDetails storage details = _proposalDetails[proposalId]; - queue( - details.targets, - details.values, - _encodeCalldata(details.signatures, details.calldatas), - details.descriptionHash - ); + ( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) = _getProposalParameters(proposalId); + + queue(targets, values, calldatas, descriptionHash); } /** * @dev See {IGovernorCompatibilityBravo-execute}. */ function execute(uint256 proposalId) public payable virtual override { - ProposalDetails storage details = _proposalDetails[proposalId]; - execute( - details.targets, - details.values, - _encodeCalldata(details.signatures, details.calldatas), - details.descriptionHash - ); + ( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) = _getProposalParameters(proposalId); + + execute(targets, values, calldatas, descriptionHash); } - function cancel(uint256 proposalId) public virtual override(IGovernor, Governor) { + /** + * @dev Cancel a proposal with GovernorBravo logic. + */ + function cancel(uint256 proposalId) public virtual { + ( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) = _getProposalParameters(proposalId); + + cancel(targets, values, calldatas, descriptionHash); + } + + /** + * @dev Cancel a proposal with GovernorBravo logic. At any moment a proposal can be cancelled, either by the + * proposer, or by third parties if the proposer's voting power has dropped below the proposal threshold. + */ + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual override(IGovernor, Governor) returns (uint256) { + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); address proposer = _proposalDetails[proposalId].proposer; require( @@ -107,7 +133,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp "GovernorBravo: proposer above threshold" ); - _cancel(proposalId); + return _cancel(targets, values, calldatas, descriptionHash); } /** @@ -128,6 +154,25 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp return fullcalldatas; } + /** + * @dev Retrieve proposal parameters by id, with fully encoded calldatas. + */ + function _getProposalParameters( + uint256 proposalId + ) + private + view + returns (address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash) + { + ProposalDetails storage details = _proposalDetails[proposalId]; + return ( + details.targets, + details.values, + _encodeCalldata(details.signatures, details.calldatas), + details.descriptionHash + ); + } + /** * @dev Store proposal metadata for later lookup */ diff --git a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol index 2727794f62c..1b87d143390 100644 --- a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol +++ b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol @@ -66,8 +66,13 @@ abstract contract GovernorCompatibilityBravoMock is return super.execute(targets, values, calldatas, salt); } - function cancel(uint256 proposalId) public override(Governor, GovernorCompatibilityBravo, IGovernor) { - super.cancel(proposalId); + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) { + return super.cancel(targets, values, calldatas, descriptionHash); } function _execute( diff --git a/contracts/mocks/wizard/MyGovernor3.sol b/contracts/mocks/wizard/MyGovernor3.sol index 4192cae9442..f4d29515627 100644 --- a/contracts/mocks/wizard/MyGovernor3.sol +++ b/contracts/mocks/wizard/MyGovernor3.sol @@ -54,8 +54,13 @@ contract MyGovernor is return super.propose(targets, values, calldatas, description); } - function cancel(uint256 proposalId) public override(Governor, GovernorCompatibilityBravo, IGovernor) { - super.cancel(proposalId); + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) { + return super.cancel(targets, values, calldatas, descriptionHash); } function _execute( diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index a499b546b91..77279eb4f18 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -204,4 +204,14 @@ library ECDSA { data := keccak256(ptr, 0x42) } } + + /** + * @dev Returns an Ethereum Signed Data with intended validator, created from a + * `validator` and `data` according to the version 0 of EIP-191. + * + * See {recover}. + */ + function toDataWithIntendedValidatorHash(address validator, bytes memory data) internal pure returns (bytes32) { + return keccak256(abi.encodePacked("\x19\x00", validator, data)); + } } diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 1ffa086cbd8..4b38b7588e4 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -68,7 +68,13 @@ class GovernorHelper { switch (visibility) { case 'external': - return this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)); + if (proposal.useCompatibilityInterface) { + return this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)); + } else { + return this.governor.methods['cancel(address[],uint256[],bytes[],bytes32)']( + ...concatOpts(proposal.shortProposal, opts), + ); + } case 'internal': return this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( ...concatOpts(proposal.shortProposal, opts), diff --git a/test/helpers/sign.js b/test/helpers/sign.js index 417ef591dbb..d537116bbb8 100644 --- a/test/helpers/sign.js +++ b/test/helpers/sign.js @@ -4,6 +4,21 @@ function toEthSignedMessageHash(messageHex) { return web3.utils.sha3(Buffer.concat([prefix, messageBuffer])); } +/** + * Create a signed data with intended validator according to the version 0 of EIP-191 + * @param validatorAddress The address of the validator + * @param dataHex The data to be concatenated with the prefix and signed + */ +function toDataWithIntendedValidatorHash(validatorAddress, dataHex) { + const validatorBuffer = Buffer.from(web3.utils.hexToBytes(validatorAddress)); + const dataBuffer = Buffer.from(web3.utils.hexToBytes(dataHex)); + const preambleBuffer = Buffer.from('\x19'); + const versionBuffer = Buffer.from('\x00'); + const ethMessage = Buffer.concat([preambleBuffer, versionBuffer, validatorBuffer, dataBuffer]); + + return web3.utils.sha3(ethMessage); +} + /** * Create a signer between a contract and a signer for a voucher of method, args, and redeemer * Note that `method` is the web3 method, not the truffle-contract method @@ -43,5 +58,6 @@ const getSignFor = module.exports = { toEthSignedMessageHash, + toDataWithIntendedValidatorHash, getSignFor, }; diff --git a/test/token/ERC20/extensions/ERC4626.t.sol b/test/token/ERC20/extensions/ERC4626.t.sol index 03786b27179..95514531c3a 100644 --- a/test/token/ERC20/extensions/ERC4626.t.sol +++ b/test/token/ERC20/extensions/ERC4626.t.sol @@ -12,7 +12,7 @@ contract ERC4626StdTest is ERC4626Test { _underlying_ = address(new ERC20Mock()); _vault_ = address(new ERC4626Mock(_underlying_)); _delta_ = 0; - _vaultMayBeEmpty = false; + _vaultMayBeEmpty = true; _unlimitedAmount = true; } } diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index 3b19cde60dd..ae737086b12 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -1,5 +1,5 @@ const { expectRevert } = require('@openzeppelin/test-helpers'); -const { toEthSignedMessageHash } = require('../../helpers/sign'); +const { toEthSignedMessageHash, toDataWithIntendedValidatorHash } = require('../../helpers/sign'); const { expect } = require('chai'); @@ -8,6 +8,7 @@ const ECDSA = artifacts.require('$ECDSA'); const TEST_MESSAGE = web3.utils.sha3('OpenZeppelin'); const WRONG_MESSAGE = web3.utils.sha3('Nope'); const NON_HASH_MESSAGE = '0x' + Buffer.from('abcd').toString('hex'); +const RANDOM_ADDRESS = web3.utils.toChecksumAddress(web3.utils.randomHex(20)); function to2098Format(signature) { const long = web3.utils.hexToBytes(signature); @@ -248,4 +249,12 @@ contract('ECDSA', function (accounts) { ); }); }); + + context('toDataWithIntendedValidatorHash', function () { + it('returns the hash correctly', async function () { + expect( + await this.ecdsa.methods['$toDataWithIntendedValidatorHash(address,bytes)'](RANDOM_ADDRESS, NON_HASH_MESSAGE), + ).to.equal(toDataWithIntendedValidatorHash(RANDOM_ADDRESS, NON_HASH_MESSAGE)); + }); + }); });