Skip to content

Commit

Permalink
Merge branch 'master' into Governor--typo-error-triggered-#3273
Browse files Browse the repository at this point in the history
  • Loading branch information
Amxx committed Mar 25, 2022
2 parents 4f4ca8f + bfff03c commit e1fb6d6
Show file tree
Hide file tree
Showing 38 changed files with 917 additions and 578 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/docs.yml
Expand Up @@ -12,7 +12,7 @@ jobs:
- uses: actions/setup-node@v3
with:
node-version: 12.x
- uses: actions/cache@v2
- uses: actions/cache@v3
id: cache
with:
path: '**/node_modules'
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/test.yml
Expand Up @@ -16,7 +16,7 @@ jobs:
- uses: actions/setup-node@v3
with:
node-version: 12.x
- uses: actions/cache@v2
- uses: actions/cache@v3
id: cache
with:
path: '**/node_modules'
Expand All @@ -42,7 +42,7 @@ jobs:
- uses: actions/setup-node@v3
with:
node-version: 12.x
- uses: actions/cache@v2
- uses: actions/cache@v3
id: cache
with:
path: '**/node_modules'
Expand All @@ -62,7 +62,7 @@ jobs:
- uses: actions/setup-node@v3
with:
node-version: 12.x
- uses: actions/cache@v2
- uses: actions/cache@v3
id: cache
with:
path: '**/node_modules'
Expand Down
8 changes: 6 additions & 2 deletions CHANGELOG.md
Expand Up @@ -4,14 +4,18 @@

* `AccessControl`: add a virtual `_checkRole(bytes32)` function that can be overridden to alter the `onlyRole` modifier behavior. ([#3137](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3137))
* `EnumerableMap`: add new `AddressToUintMap` map type. ([#3150](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3150))
* `EnumerableMap`: add new `Bytes32ToBytes32Map` map type. ([#3192](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3192))
* `ERC1155`: Add a `_afterTokenTransfer` hook for improved extensibility. ([#3166](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3166))
* `DoubleEndedQueue`: a new data structure that supports efficient push and pop to both front and back, useful for FIFO and LIFO queues. ([#3153](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3153))
* `Governor`: improved security of `onlyGovernance` modifier when using an external executor contract (e.g. a timelock) that can operate without necessarily going through the governance protocol. ([#3147](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3147))
* `Governor`: Add a way to parameterize votes. This can be used to implement voting systems such as fractionalized voting, ERC721 based voting, or any number of other systems. The `params` argument added to `_countVote` method, and included in the newly added `_getVotes` method, can be used by counting and voting modules respectively for such purposes.
* `Governor`: Add a way to parameterize votes. This can be used to implement voting systems such as fractionalized voting, ERC721 based voting, or any number of other systems. The `params` argument added to `_countVote` method, and included in the newly added `_getVotes` method, can be used by counting and voting modules respectively for such purposes. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043))
* `Governor`: rewording of revert reason for consistency. ([#3275](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3275))
* `ERC20FlashMint`: support infinite allowance when paying back a flash loan. ([#3226](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3226))
* `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165))
* `draft-ERC20Permit`: replace `immutable` with `constant` for `_PERMIT_TYPEHASH` since the `keccak256` of string literals is treated specially and the hash is evaluated at compile time. ([#3196](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3196))
* `ERC20Wrapper`: the `decimals()` function now tries to fetch the value from the underlying token instance. If that calls revert, then the default value is used. ([#3259](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3259))
* `Governor`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by governors. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230))
* `TimelockController`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by timelocks. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230))

### Breaking changes

Expand Down Expand Up @@ -457,7 +461,7 @@ Refer to the table below to adjust your inheritance list.
* `SignedSafeMath`: added overflow-safe operations for signed integers (`int256`). ([#1559](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1559), [#1588](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1588))

### Improvements
* The compiler version required by `Array` was behind the rest of the libray so it was updated to `v0.4.24`. ([#1553](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1553))
* The compiler version required by `Array` was behind the rest of the library so it was updated to `v0.4.24`. ([#1553](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1553))
* Now conforming to a 4-space indentation code style. ([1508](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1508))
* `ERC20`: more gas efficient due to removed redundant `require`s. ([#1409](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1409))
* `ERC721`: fixed a bug that prevented internal data structures from being properly cleaned, missing potential gas refunds. ([#1539](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1539) and [#1549](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1549))
Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Expand Up @@ -51,7 +51,7 @@ refer to some very important conditions that your PR must meet in order to be ac

6) Maintainers will review your code and possibly ask for changes before your code is pulled in to the main repository. We'll check that all tests pass, review the coding style, and check for general code correctness. If everything is OK, we'll merge your pull request and your code will be part of OpenZeppelin Contracts.

*IMPORTANT* Please pay attention to the maintainer's feedback, since its a necessary step to keep up with the standards OpenZeppelin Contracts attains to.
*IMPORTANT* Please pay attention to the maintainer's feedback, since it's a necessary step to keep up with the standards OpenZeppelin Contracts attains to.

## All set!

Expand Down
2 changes: 1 addition & 1 deletion audit/2017-03.md
Expand Up @@ -133,7 +133,7 @@ I presume that the goal of this contract is to allow and annotate a migration to

We like these pauses! Note that these allow significant griefing potential by owners, and that this might not be obvious to participants in smart contracts using the OpenZeppelin framework. We would recommend that additional sample logic be added to for instance the TokenContract showing safer use of the pause and resume functions. In particular, we would recommend a timelock after which anyone could unpause the contract.

The modifers use the pattern `if(bool){_;}`. This is fine for functions that return false upon failure, but could be problematic for functions expected to throw upon failure. See our comments above on standardizing on `throw` or `return(false)`.
The modifiers use the pattern `if(bool){_;}`. This is fine for functions that return false upon failure, but could be problematic for functions expected to throw upon failure. See our comments above on standardizing on `throw` or `return(false)`.

## Ownership

Expand Down
2 changes: 1 addition & 1 deletion certora/applyHarness.patch
Expand Up @@ -71,7 +71,7 @@ diff -ruN governance/Governor.sol governance/Governor.sol
+
/**
* @dev Restrict access to governor executing address. Some module might override the _executor function to make
* sure this modifier is consistant with the execution model.
* sure this modifier is consistent with the execution model.
@@ -167,12 +167,12 @@
/**
* @dev Amount of votes already cast passes the threshold limit.
Expand Down
6 changes: 3 additions & 3 deletions certora/specs/GovernorBase.spec
Expand Up @@ -173,11 +173,11 @@ rule executionOnlyIfQuoromReachedAndVoteSucceeded(uint256 pId, env e, method f){
/*
* A user cannot vote twice
*/
// Checked for castVote only. all 3 castVote functions call _castVote, so the completness of the verification is counted on
// the fact that the 3 functions themselves makes no chages, but rather call an internal function to execute.
// Checked for castVote only. all 3 castVote functions call _castVote, so the completeness of the verification is counted on
// the fact that the 3 functions themselves makes no changes, but rather call an internal function to execute.
// That means that we do not check those 3 functions directly, however for castVote & castVoteWithReason it is quite trivial
// to understand why this is ok. For castVoteBySig we basically assume that the signature referendum is correct without checking it.
// We could check each function seperately and pass the rule, but that would have uglyfied the code with no concrete
// We could check each function separately and pass the rule, but that would have uglyfied the code with no concrete
// benefit, as it is evident that nothing is happening in the first 2 functions (calling a view function), and we do not desire to check the signature verification.
rule doubleVoting(uint256 pId, uint8 sup, method f) {
env e;
Expand Down
10 changes: 5 additions & 5 deletions certora/specs/GovernorCountingSimple.spec
Expand Up @@ -128,11 +128,11 @@ invariant OneIsNotMoreThanAll(uint256 pId)
/*
* Only sender's voting status can be changed by execution of any cast vote function
*/
// Checked for castVote only. all 3 castVote functions call _castVote, so the completness of the verification is counted on
// the fact that the 3 functions themselves makes no chages, but rather call an internal function to execute.
// Checked for castVote only. all 3 castVote functions call _castVote, so the completeness of the verification is counted on
// the fact that the 3 functions themselves makes no changes, but rather call an internal function to execute.
// That means that we do not check those 3 functions directly, however for castVote & castVoteWithReason it is quite trivial
// to understand why this is ok. For castVoteBySig we basically assume that the signature referendum is correct without checking it.
// We could check each function seperately and pass the rule, but that would have uglyfied the code with no concrete
// We could check each function separately and pass the rule, but that would have uglyfied the code with no concrete
// benefit, as it is evident that nothing is happening in the first 2 functions (calling a view function), and we do not desire to check the signature verification.
rule noVoteForSomeoneElse(uint256 pId, uint8 sup, method f) {
env e; calldataarg args;
Expand Down Expand Up @@ -205,7 +205,7 @@ rule privilegedOnlyNumerator(method f, uint256 newQuorumNumerator){
uint256 quorumNumAfter = quorumNumerator(e);
address executorCheck = getExecutor(e);

assert quorumNumBefore != quorumNumAfter => e.msg.sender == executorCheck, "non priveleged user changed quorum numerator";
assert quorumNumBefore != quorumNumAfter => e.msg.sender == executorCheck, "non privileged user changed quorum numerator";
}

rule privilegedOnlyTimelock(method f, uint256 newQuorumNumerator){
Expand All @@ -217,5 +217,5 @@ rule privilegedOnlyTimelock(method f, uint256 newQuorumNumerator){

uint256 timelockAfter = timelock(e);

assert timelockBefore != timelockAfter => e.msg.sender == timelockBefore, "non priveleged user changed timelock";
assert timelockBefore != timelockAfter => e.msg.sender == timelockBefore, "non privileged user changed timelock";
}
2 changes: 1 addition & 1 deletion contracts/finance/VestingWallet.sol
Expand Up @@ -120,7 +120,7 @@ contract VestingWallet is Context {
}

/**
* @dev Virtual implementation of the vesting formula. This returns the amout vested, as a function of time, for
* @dev Virtual implementation of the vesting formula. This returns the amount vested, as a function of time, for
* an asset given its total historical allocation.
*/
function _vestingSchedule(uint256 totalAllocation, uint64 timestamp) internal view virtual returns (uint256) {
Expand Down
57 changes: 49 additions & 8 deletions contracts/governance/Governor.sol
Expand Up @@ -3,6 +3,8 @@

pragma solidity ^0.8.0;

import "../token/ERC721/IERC721Receiver.sol";
import "../token/ERC1155/IERC1155Receiver.sol";
import "../utils/cryptography/ECDSA.sol";
import "../utils/cryptography/draft-EIP712.sol";
import "../utils/introspection/ERC165.sol";
Expand All @@ -24,7 +26,7 @@ import "./IGovernor.sol";
*
* _Available since v4.3._
*/
abstract contract Governor is Context, ERC165, EIP712, IGovernor {
abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver {
using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
using SafeCast for uint256;
using Timers for Timers.BlockNumber;
Expand All @@ -46,7 +48,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {

// This queue keeps track of the governor operating on itself. Calls to functions protected by the
// {onlyGovernance} modifier needs to be whitelisted in this queue. Whitelisting is set in {_beforeExecute},
// consummed by the {onlyGovernance} modifier and eventually reset in {_afterExecute}. This ensures that the
// consumed by the {onlyGovernance} modifier and eventually reset in {_afterExecute}. This ensures that the
// execution of {onlyGovernance} protected calls can only be achieved through successful proposals.
DoubleEndedQueue.Bytes32Deque private _governanceCall;

Expand All @@ -64,7 +66,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
require(_msgSender() == _executor(), "Governor: onlyGovernance");
if (_executor() != address(this)) {
bytes32 msgDataHash = keccak256(_msgData());
// loop until poping the expected operation - throw if deque is empty (operation not authorized)
// loop until popping the expected operation - throw if deque is empty (operation not authorized)
while (_governanceCall.popFront() != msgDataHash) {}
}
_;
Expand Down Expand Up @@ -97,6 +99,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
this.castVoteWithReasonAndParamsBySig.selector ^
this.getVotesWithParams.selector) ||
interfaceId == type(IGovernor).interfaceId ||
interfaceId == type(IERC1155Receiver).interfaceId ||
super.supportsInterface(interfaceId);
}

Expand Down Expand Up @@ -124,7 +127,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
*
* Note that the chainId and the governor address are not part of the proposal id computation. Consequently, the
* same proposal (with same operation and same description) will have the same id if submitted on multiple governors
* accross multiple networks. This also means that in order to execute the same operation twice (on the same
* across multiple networks. This also means that in order to execute the same operation twice (on the same
* governor) the proposer will have to change the description in order to avoid proposal id conflicts.
*/
function hashProposal(
Expand Down Expand Up @@ -229,7 +232,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
/**
* @dev Default additional encoded parameters used by castVote methods that don't include them
*
* Note: Should be overriden by specific implementations to use an appropriate value, the
* Note: Should be overridden by specific implementations to use an appropriate value, the
* meaning of the additional params, in the context of that implementation
*/
function _defaultParams() internal view virtual returns (bytes memory) {
Expand Down Expand Up @@ -308,7 +311,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
}

/**
* @dev Internal execution mechanism. Can be overriden to implement different execution mechanism
* @dev Internal execution mechanism. Can be overridden to implement different execution mechanism
*/
function _execute(
uint256, /* proposalId */
Expand All @@ -325,7 +328,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
}

/**
* @dev Hook before execution is trigerred.
* @dev Hook before execution is triggered.
*/
function _beforeExecute(
uint256, /* proposalId */
Expand All @@ -344,7 +347,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
}

/**
* @dev Hook after execution is trigerred.
* @dev Hook after execution is triggered.
*/
function _afterExecute(
uint256, /* proposalId */
Expand Down Expand Up @@ -552,4 +555,42 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
function _executor() internal view virtual returns (address) {
return address(this);
}

/**
* @dev See {IERC721Receiver-onERC721Received}.
*/
function onERC721Received(
address,
address,
uint256,
bytes memory
) public virtual override returns (bytes4) {
return this.onERC721Received.selector;
}

/**
* @dev See {IERC1155Receiver-onERC1155Received}.
*/
function onERC1155Received(
address,
address,
uint256,
uint256,
bytes memory
) public virtual override returns (bytes4) {
return this.onERC1155Received.selector;
}

/**
* @dev See {IERC1155Receiver-onERC1155BatchReceived}.
*/
function onERC1155BatchReceived(
address,
address,
uint256[] memory,
uint256[] memory,
bytes memory
) public virtual override returns (bytes4) {
return this.onERC1155BatchReceived.selector;
}
}
4 changes: 2 additions & 2 deletions contracts/governance/IGovernor.sol
Expand Up @@ -158,8 +158,8 @@ abstract contract IGovernor is IERC165 {
* @notice module:user-config
* @dev Minimum number of cast voted required for a proposal to be successful.
*
* Note: The `blockNumber` parameter corresponds to the snaphot used for counting vote. This allows to scale the
* quroum depending on values such as the totalSupply of a token at this block (see {ERC20Votes}).
* Note: The `blockNumber` parameter corresponds to the snapshot used for counting vote. This allows to scale the
* quorum depending on values such as the totalSupply of a token at this block (see {ERC20Votes}).
*/
function quorum(uint256 blockNumber) public view virtual returns (uint256);

Expand Down
2 changes: 1 addition & 1 deletion contracts/governance/README.adoc
Expand Up @@ -40,7 +40,7 @@ Other extensions can customize the behavior or interface in multiple ways.

* {GovernorCompatibilityBravo}: Extends the interface to be fully `GovernorBravo`-compatible. Note that events are compatible regardless of whether this extension is included or not.

* {GovernorSettings}: Manages some of the settings (voting delay, voting period duration, and proposal threshold) in a way that can be updated through a governance proposal, without requiering an upgrade.
* {GovernorSettings}: Manages some of the settings (voting delay, voting period duration, and proposal threshold) in a way that can be updated through a governance proposal, without requiring an upgrade.

* {GovernorPreventLateQuorum}: Ensures there is a minimum voting period after quorum is reached as a security protection against large voters.

Expand Down

0 comments on commit e1fb6d6

Please sign in to comment.