From 3458c1e8541ce0a0cd935828c9db8f9cbca988a0 Mon Sep 17 00:00:00 2001 From: rotcivegaf Date: Wed, 12 Jan 2022 16:08:59 -0300 Subject: [PATCH 01/26] Add SignedMath with math utilities for signed integers (#2686) * add contract and tests * avoid implicit cast * add test cases * fix test names * modify avarage and add tests * improve signed average formula * fix lint * better average formula * refactor signed average testing * add doc and changelog entry * Update contracts/utils/math/SignedMath.sol Co-authored-by: Francisco Giordano * remove ceilDiv Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- CHANGELOG.md | 1 + contracts/mocks/SignedMathMock.sol | 19 +++++++ contracts/utils/README.adoc | 2 + contracts/utils/math/SignedMath.sol | 32 ++++++++++++ test/utils/math/SignedMath.test.js | 77 +++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+) create mode 100644 contracts/mocks/SignedMathMock.sol create mode 100644 contracts/utils/math/SignedMath.sol create mode 100644 test/utils/math/SignedMath.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 53a765f44fe..a96813b386c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ * `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056)) * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) + * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) ### Breaking change diff --git a/contracts/mocks/SignedMathMock.sol b/contracts/mocks/SignedMathMock.sol new file mode 100644 index 00000000000..8d82e137e0c --- /dev/null +++ b/contracts/mocks/SignedMathMock.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../utils/math/SignedMath.sol"; + +contract SignedMathMock { + function max(int256 a, int256 b) public pure returns (int256) { + return SignedMath.max(a, b); + } + + function min(int256 a, int256 b) public pure returns (int256) { + return SignedMath.min(a, b); + } + + function average(int256 a, int256 b) public pure returns (int256) { + return SignedMath.average(a, b); + } +} diff --git a/contracts/utils/README.adoc b/contracts/utils/README.adoc index 4f19f1ac224..83e30e78cff 100644 --- a/contracts/utils/README.adoc +++ b/contracts/utils/README.adoc @@ -27,6 +27,8 @@ Finally, {Create2} contains all necessary utilities to safely use the https://bl {{Math}} +{{SignedMath}} + {{SafeCast}} {{SafeMath}} diff --git a/contracts/utils/math/SignedMath.sol b/contracts/utils/math/SignedMath.sol new file mode 100644 index 00000000000..96160910444 --- /dev/null +++ b/contracts/utils/math/SignedMath.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +/** + * @dev Standard signed math utilities missing in the Solidity language. + */ +library SignedMath { + /** + * @dev Returns the largest of two signed numbers. + */ + function max(int256 a, int256 b) internal pure returns (int256) { + return a >= b ? a : b; + } + + /** + * @dev Returns the smallest of two signed numbers. + */ + function min(int256 a, int256 b) internal pure returns (int256) { + return a < b ? a : b; + } + + /** + * @dev Returns the average of two signed numbers without overflow. + * The result is rounded towards zero. + */ + function average(int256 a, int256 b) internal pure returns (int256) { + // Formula from the book "Hacker's Delight" + int256 x = (a & b) + ((a ^ b) >> 1); + return x + (int256(uint256(x) >> 255) & (a ^ b)); + } +} diff --git a/test/utils/math/SignedMath.test.js b/test/utils/math/SignedMath.test.js new file mode 100644 index 00000000000..0aef556dfbe --- /dev/null +++ b/test/utils/math/SignedMath.test.js @@ -0,0 +1,77 @@ +const { BN, constants } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); +const { MIN_INT256, MAX_INT256 } = constants; + +const SignedMathMock = artifacts.require('SignedMathMock'); + +contract('SignedMath', function (accounts) { + const min = new BN('-1234'); + const max = new BN('5678'); + + beforeEach(async function () { + this.math = await SignedMathMock.new(); + }); + + describe('max', function () { + it('is correctly detected in first argument position', async function () { + expect(await this.math.max(max, min)).to.be.bignumber.equal(max); + }); + + it('is correctly detected in second argument position', async function () { + expect(await this.math.max(min, max)).to.be.bignumber.equal(max); + }); + }); + + describe('min', function () { + it('is correctly detected in first argument position', async function () { + expect(await this.math.min(min, max)).to.be.bignumber.equal(min); + }); + + it('is correctly detected in second argument position', async function () { + expect(await this.math.min(max, min)).to.be.bignumber.equal(min); + }); + }); + + describe('average', function () { + function bnAverage (a, b) { + return a.add(b).divn(2); + } + + it('is correctly calculated with various input', async function () { + const valuesX = [ + new BN('0'), + new BN('3'), + new BN('-3'), + new BN('4'), + new BN('-4'), + new BN('57417'), + new BN('-57417'), + new BN('42304'), + new BN('-42304'), + MIN_INT256, + MAX_INT256, + ]; + + const valuesY = [ + new BN('0'), + new BN('5'), + new BN('-5'), + new BN('2'), + new BN('-2'), + new BN('57417'), + new BN('-57417'), + new BN('42304'), + new BN('-42304'), + MIN_INT256, + MAX_INT256, + ]; + + for (const x of valuesX) { + for (const y of valuesY) { + expect(await this.math.average(x, y)) + .to.be.bignumber.equal(bnAverage(x, y), `Bad result for average(${x}, ${y})`); + } + } + }); + }); +}); From e192fac2769386b7d4b61a3541073ab47bb7723a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jan 2022 19:46:55 +0100 Subject: [PATCH 02/26] Simplify UUPSUpgradeable along the lines of ERC1822 (#3021) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 7 ++- contracts/interfaces/draft-IERC1822.sol | 20 +++++++ contracts/mocks/UUPS/UUPSLegacy.sol | 58 +++++++++++++++++++ ...TestInProd.sol => UUPSUpgradeableMock.sol} | 10 ---- contracts/proxy/ERC1967/ERC1967Upgrade.sol | 37 +++++------- contracts/proxy/README.adoc | 6 +- contracts/proxy/utils/UUPSUpgradeable.sol | 28 ++++++++- test/helpers/erc1967.js | 24 ++++++++ test/proxy/Proxy.behaviour.js | 16 ++--- test/proxy/beacon/BeaconProxy.test.js | 15 ++--- .../TransparentUpgradeableProxy.behaviour.js | 21 +++---- test/proxy/utils/UUPSUpgradeable.test.js | 37 ++++++++---- 12 files changed, 191 insertions(+), 88 deletions(-) create mode 100644 contracts/interfaces/draft-IERC1822.sol create mode 100644 contracts/mocks/UUPS/UUPSLegacy.sol rename contracts/mocks/UUPS/{TestInProd.sol => UUPSUpgradeableMock.sol} (75%) create mode 100644 test/helpers/erc1967.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a96813b386c..7fdf8dd9c69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,10 +17,13 @@ * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) + * `ERC1967Upgrade`: Refactor the secure upgrade to use `ERC1822` instead of the previous rollback mechanism. This reduces code complexity and attack surface with similar security guarantees. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) + * `UUPSUpgradeable`: Add `ERC1822` compliance to support the updated secure upgrade mechanism. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) -### Breaking change +### Breaking changes -Solidity pragma in `utils/Address.sol` is increased from `^0.8.0` to `^0.8.1`. This is required by the `account.code.length` syntax that replaces inline assembly. This may require users to bump their compiler version from `0.8.0` to `0.8.1` or later. Note that other parts of the code already include stricter requirements. +* `ERC1967Upgrade`: The function `_upgradeToAndCallSecure` was renamed to `_upgradeToAndCallUUPS`, along with the change in security mechanism described above. +* `Address`: The Solidity pragma is increased from `^0.8.0` to `^0.8.1`. This is required by the `account.code.length` syntax that replaces inline assembly. This may require users to bump their compiler version from `0.8.0` to `0.8.1` or later. Note that other parts of the code already include stricter requirements. ## 4.4.2 (2022-01-11) diff --git a/contracts/interfaces/draft-IERC1822.sol b/contracts/interfaces/draft-IERC1822.sol new file mode 100644 index 00000000000..51d8f41ddcc --- /dev/null +++ b/contracts/interfaces/draft-IERC1822.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts v4.x.0 (proxy/ERC1822/IProxiable.sol) + +pragma solidity ^0.8.0; + +/** + * @dev ERC1822: Universal Upgradeable Proxy Standard (UUPS) documents a method for upgradeability through a simplified + * proxy whose upgrades are fully controlled by the current implementation. + */ +interface IERC1822Proxiable { + /** + * @dev Returns the storage slot that the proxiable contract assumes is being used to store the implementation + * address. + * + * IMPORTANT: A proxy pointing at a proxiable contract should not be considered proxiable itself, because this risks + * bricking a proxy that upgrades to it, by delegating to itself until out of gas. Thus it is critical that this + * function revert if invoked through a proxy. + */ + function proxiableUUID() external view returns (bytes32); +} diff --git a/contracts/mocks/UUPS/UUPSLegacy.sol b/contracts/mocks/UUPS/UUPSLegacy.sol new file mode 100644 index 00000000000..550a6221b48 --- /dev/null +++ b/contracts/mocks/UUPS/UUPSLegacy.sol @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./UUPSUpgradeableMock.sol"; + +// This contract implements the pre-4.5 UUPS upgrade function with a rollback test. +// It's used to test that newer UUPS contracts are considered valid upgrades by older UUPS contracts. +contract UUPSUpgradeableLegacyMock is UUPSUpgradeableMock { + // Inlined from ERC1967Upgrade + bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143; + + // ERC1967Upgrade._setImplementation is private so we reproduce it here. + // An extra underscore prevents a name clash error. + function __setImplementation(address newImplementation) private { + require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract"); + StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; + } + + function _upgradeToAndCallSecureLegacyV1( + address newImplementation, + bytes memory data, + bool forceCall + ) internal { + address oldImplementation = _getImplementation(); + + // Initial upgrade and setup call + __setImplementation(newImplementation); + if (data.length > 0 || forceCall) { + Address.functionDelegateCall(newImplementation, data); + } + + // Perform rollback test if not already in progress + StorageSlot.BooleanSlot storage rollbackTesting = StorageSlot.getBooleanSlot(_ROLLBACK_SLOT); + if (!rollbackTesting.value) { + // Trigger rollback using upgradeTo from the new implementation + rollbackTesting.value = true; + Address.functionDelegateCall( + newImplementation, + abi.encodeWithSignature("upgradeTo(address)", oldImplementation) + ); + rollbackTesting.value = false; + // Check rollback was effective + require(oldImplementation == _getImplementation(), "ERC1967Upgrade: upgrade breaks further upgrades"); + // Finally reset to the new implementation and log the upgrade + _upgradeTo(newImplementation); + } + } + + // hooking into the old mechanism + function upgradeTo(address newImplementation) external virtual override { + _upgradeToAndCallSecureLegacyV1(newImplementation, bytes(""), false); + } + + function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual override { + _upgradeToAndCallSecureLegacyV1(newImplementation, data, false); + } +} diff --git a/contracts/mocks/UUPS/TestInProd.sol b/contracts/mocks/UUPS/UUPSUpgradeableMock.sol similarity index 75% rename from contracts/mocks/UUPS/TestInProd.sol rename to contracts/mocks/UUPS/UUPSUpgradeableMock.sol index bbb610300fa..367303ec0b0 100644 --- a/contracts/mocks/UUPS/TestInProd.sol +++ b/contracts/mocks/UUPS/UUPSUpgradeableMock.sol @@ -19,13 +19,3 @@ contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock { ERC1967Upgrade._upgradeToAndCall(newImplementation, data, false); } } - -contract UUPSUpgradeableBrokenMock is UUPSUpgradeableMock { - function upgradeTo(address) external virtual override { - // pass - } - - function upgradeToAndCall(address, bytes memory) external payable virtual override { - // pass - } -} diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index 036782fc7a0..45a45991676 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.2; import "../beacon/IBeacon.sol"; +import "../../interfaces/draft-IERC1822.sol"; import "../../utils/Address.sol"; import "../../utils/StorageSlot.sol"; @@ -77,33 +78,23 @@ abstract contract ERC1967Upgrade { * * Emits an {Upgraded} event. */ - function _upgradeToAndCallSecure( + function _upgradeToAndCallUUPS( address newImplementation, bytes memory data, bool forceCall ) internal { - address oldImplementation = _getImplementation(); - - // Initial upgrade and setup call - _setImplementation(newImplementation); - if (data.length > 0 || forceCall) { - Address.functionDelegateCall(newImplementation, data); - } - - // Perform rollback test if not already in progress - StorageSlot.BooleanSlot storage rollbackTesting = StorageSlot.getBooleanSlot(_ROLLBACK_SLOT); - if (!rollbackTesting.value) { - // Trigger rollback using upgradeTo from the new implementation - rollbackTesting.value = true; - Address.functionDelegateCall( - newImplementation, - abi.encodeWithSignature("upgradeTo(address)", oldImplementation) - ); - rollbackTesting.value = false; - // Check rollback was effective - require(oldImplementation == _getImplementation(), "ERC1967Upgrade: upgrade breaks further upgrades"); - // Finally reset to the new implementation and log the upgrade - _upgradeTo(newImplementation); + // Upgrades from old implementations will perform a rollback test. This test requires the new + // implementation to upgrade back to the old, non-ERC1822 compliant, implementation. Removing + // this special case will break upgrade paths from old UUPS implementation to new ones. + if (StorageSlot.getBooleanSlot(_ROLLBACK_SLOT).value) { + _setImplementation(newImplementation); + } else { + try IERC1822Proxiable(newImplementation).proxiableUUID() returns (bytes32 slot) { + require(slot == _IMPLEMENTATION_SLOT, "ERC1967Upgrade: unsupported proxiableUUID"); + } catch { + revert("ERC1967Upgrade: new implementation is not UUPS"); + } + _upgradeToAndCall(newImplementation, data, forceCall); } } diff --git a/contracts/proxy/README.adoc b/contracts/proxy/README.adoc index ae278b083ce..9f37b267f90 100644 --- a/contracts/proxy/README.adoc +++ b/contracts/proxy/README.adoc @@ -17,14 +17,14 @@ In order to avoid clashes with the storage variables of the implementation contr There are two alternative ways to add upgradeability to an ERC1967 proxy. Their differences are explained below in <>. - {TransparentUpgradeableProxy}: A proxy with a built in admin and upgrade interface. -- {UUPSUpgradeable}: An upgradeability mechanism to be included in the implementation for an ERC1967 proxy. +- {UUPSUpgradeable}: An upgradeability mechanism to be included in the implementation contract. CAUTION: Using upgradeable proxies correctly and securely is a difficult task that requires deep knowledge of the proxy pattern, Solidity, and the EVM. Unless you want a lot of low level control, we recommend using the xref:upgrades-plugins::index.adoc[OpenZeppelin Upgrades Plugins] for Truffle and Hardhat. A different family of proxies are beacon proxies. This pattern, popularized by Dharma, allows multiple proxies to be upgraded to a different implementation in a single transaction. - {BeaconProxy}: A proxy that retreives its implementation from a beacon contract. -- {UpgradeableBeacon}: A beacon contract that can be upgraded. +- {UpgradeableBeacon}: A beacon contract with a built in admin that can upgrade the {BeaconProxy} pointing to it. In this pattern, the proxy contract doesn't hold the implementation address in storage like an ERC1967 proxy, instead the address is stored in a separate beacon contract. The `upgrade` operations that are sent to the beacon instead of to the proxy contract, and all proxies that follow that beacon are automatically upgraded. @@ -48,6 +48,8 @@ By default, the upgrade functionality included in {UUPSUpgradeable} contains a s - Adding a flag mechanism in the implementation that will disable the upgrade function when triggered. - Upgrading to an implementation that features an upgrade mechanism without the additional security check, and then upgrading again to another implementation without the upgrade mechanism. +The current implementation of this security mechanism uses https://eips.ethereum.org/EIPS/eip-1822[EIP1822] to detect the storage slot used by the implementation. A previous implementation, now deprecated, relied on a rollback check. It is possible to upgrade from a contract using the old mechanism to a new one. The inverse is however not possible, as old implementations (before version 4.5) did not include the `ERC1822` interface. + == Core {{Proxy}} diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index 1887872d821..5d544c4f016 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; +import "../../interfaces/draft-IERC1822.sol"; import "../ERC1967/ERC1967Upgrade.sol"; /** @@ -17,7 +18,7 @@ import "../ERC1967/ERC1967Upgrade.sol"; * * _Available since v4.1._ */ -abstract contract UUPSUpgradeable is ERC1967Upgrade { +abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment address private immutable __self = address(this); @@ -34,6 +35,27 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade { _; } + /** + * @dev Check that the execution is not being performed through a delegate call. This allows a function to be + * callable on the implementing contract but not through proxies. + */ + modifier notDelegated() { + require(address(this) == __self, "UUPSUpgradeable: must not be called through delegatecall"); + _; + } + + /** + * @dev Implementation of the ERC1822 {proxiableUUID} function. This returns the storage slot used by the + * implementation. It is used to validate that the this implementation remains valid after an upgrade. + * + * IMPORTANT: A proxy pointing at a proxiable contract should not be considered proxiable itself, because this risks + * bricking a proxy that upgrades to it, by delegating to itself until out of gas. Thus it is critical that this + * function revert if invoked through a proxy. This is guaranteed by the `notDelegated` modifier. + */ + function proxiableUUID() external view virtual override notDelegated returns (bytes32) { + return _IMPLEMENTATION_SLOT; + } + /** * @dev Upgrade the implementation of the proxy to `newImplementation`. * @@ -43,7 +65,7 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade { */ function upgradeTo(address newImplementation) external virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallSecure(newImplementation, new bytes(0), false); + _upgradeToAndCallUUPS(newImplementation, new bytes(0), false); } /** @@ -56,7 +78,7 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade { */ function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallSecure(newImplementation, data, true); + _upgradeToAndCallUUPS(newImplementation, data, true); } /** diff --git a/test/helpers/erc1967.js b/test/helpers/erc1967.js new file mode 100644 index 00000000000..aab0e2288a3 --- /dev/null +++ b/test/helpers/erc1967.js @@ -0,0 +1,24 @@ +const ImplementationLabel = 'eip1967.proxy.implementation'; +const AdminLabel = 'eip1967.proxy.admin'; +const BeaconLabel = 'eip1967.proxy.beacon'; + +function labelToSlot (label) { + return '0x' + web3.utils.toBN(web3.utils.keccak256(label)).subn(1).toString(16); +} + +function getSlot (address, slot) { + return web3.eth.getStorageAt( + web3.utils.isAddress(address) ? address : address.address, + web3.utils.isHex(slot) ? slot : labelToSlot(slot), + ); +} + +module.exports = { + ImplementationLabel, + AdminLabel, + BeaconLabel, + ImplementationSlot: labelToSlot(ImplementationLabel), + AdminSlot: labelToSlot(AdminLabel), + BeaconSlot: labelToSlot(BeaconLabel), + getSlot, +}; diff --git a/test/proxy/Proxy.behaviour.js b/test/proxy/Proxy.behaviour.js index 8cb457b06c6..435792f2302 100644 --- a/test/proxy/Proxy.behaviour.js +++ b/test/proxy/Proxy.behaviour.js @@ -1,16 +1,10 @@ -const { BN, expectRevert } = require('@openzeppelin/test-helpers'); -const ethereumjsUtil = require('ethereumjs-util'); +const { expectRevert } = require('@openzeppelin/test-helpers'); +const { getSlot, ImplementationSlot } = require('../helpers/erc1967'); const { expect } = require('chai'); const DummyImplementation = artifacts.require('DummyImplementation'); -const IMPLEMENTATION_LABEL = 'eip1967.proxy.implementation'; - -function toChecksumAddress (address) { - return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0')); -} - module.exports = function shouldBehaveLikeProxy (createProxy, proxyAdminAddress, proxyCreator) { it('cannot be initialized with a non-contract address', async function () { const nonContractAddress = proxyCreator; @@ -28,9 +22,9 @@ module.exports = function shouldBehaveLikeProxy (createProxy, proxyAdminAddress, const assertProxyInitialization = function ({ value, balance }) { it('sets the implementation address', async function () { - const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(IMPLEMENTATION_LABEL))).subn(1).toString(16); - const implementation = toChecksumAddress((await web3.eth.getStorageAt(this.proxy, slot)).substr(-40)); - expect(implementation).to.be.equal(this.implementation); + const implementationSlot = await getSlot(this.proxy, ImplementationSlot); + const implementationAddress = web3.utils.toChecksumAddress(implementationSlot.substr(-40)); + expect(implementationAddress).to.be.equal(this.implementation); }); it('initializes the proxy', async function () { diff --git a/test/proxy/beacon/BeaconProxy.test.js b/test/proxy/beacon/BeaconProxy.test.js index e02bb3b297a..0a4a8d0cfb9 100644 --- a/test/proxy/beacon/BeaconProxy.test.js +++ b/test/proxy/beacon/BeaconProxy.test.js @@ -1,6 +1,5 @@ -const { BN, expectRevert } = require('@openzeppelin/test-helpers'); -const ethereumjsUtil = require('ethereumjs-util'); -const { keccak256 } = ethereumjsUtil; +const { expectRevert } = require('@openzeppelin/test-helpers'); +const { getSlot, BeaconSlot } = require('../../helpers/erc1967'); const { expect } = require('chai'); @@ -11,13 +10,6 @@ const DummyImplementationV2 = artifacts.require('DummyImplementationV2'); const BadBeaconNoImpl = artifacts.require('BadBeaconNoImpl'); const BadBeaconNotContract = artifacts.require('BadBeaconNotContract'); -function toChecksumAddress (address) { - return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0').substr(-40)); -} - -const BEACON_LABEL = 'eip1967.proxy.beacon'; -const BEACON_SLOT = '0x' + new BN(keccak256(Buffer.from(BEACON_LABEL))).subn(1).toString(16); - contract('BeaconProxy', function (accounts) { const [anotherAccount] = accounts; @@ -53,7 +45,8 @@ contract('BeaconProxy', function (accounts) { describe('initialization', function () { before(function () { this.assertInitialized = async ({ value, balance }) => { - const beaconAddress = toChecksumAddress(await web3.eth.getStorageAt(this.proxy.address, BEACON_SLOT)); + const beaconSlot = await getSlot(this.proxy, BeaconSlot); + const beaconAddress = web3.utils.toChecksumAddress(beaconSlot.substr(-40)); expect(beaconAddress).to.equal(this.beacon.address); const dummy = new DummyImplementation(this.proxy.address); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 54b78e06420..33fef6f4119 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -1,6 +1,6 @@ const { BN, expectRevert, expectEvent, constants } = require('@openzeppelin/test-helpers'); const { ZERO_ADDRESS } = constants; -const ethereumjsUtil = require('ethereumjs-util'); +const { getSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967'); const { expect } = require('chai'); @@ -16,13 +16,6 @@ const InitializableMock = artifacts.require('InitializableMock'); const DummyImplementation = artifacts.require('DummyImplementation'); const ClashingImplementation = artifacts.require('ClashingImplementation'); -const IMPLEMENTATION_LABEL = 'eip1967.proxy.implementation'; -const ADMIN_LABEL = 'eip1967.proxy.admin'; - -function toChecksumAddress (address) { - return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0').substr(-40)); -} - module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createProxy, accounts) { const [proxyAdminAddress, proxyAdminOwner, anotherAccount] = accounts; @@ -312,15 +305,15 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro describe('storage', function () { it('should store the implementation address in specified location', async function () { - const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(IMPLEMENTATION_LABEL))).subn(1).toString(16); - const implementation = toChecksumAddress(await web3.eth.getStorageAt(this.proxyAddress, slot)); - expect(implementation).to.be.equal(this.implementationV0); + const implementationSlot = await getSlot(this.proxy, ImplementationSlot); + const implementationAddress = web3.utils.toChecksumAddress(implementationSlot.substr(-40)); + expect(implementationAddress).to.be.equal(this.implementationV0); }); it('should store the admin proxy in specified location', async function () { - const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(ADMIN_LABEL))).subn(1).toString(16); - const proxyAdmin = toChecksumAddress(await web3.eth.getStorageAt(this.proxyAddress, slot)); - expect(proxyAdmin).to.be.equal(proxyAdminAddress); + const proxyAdminSlot = await getSlot(this.proxy, AdminSlot); + const proxyAdminAddress = web3.utils.toChecksumAddress(proxyAdminSlot.substr(-40)); + expect(proxyAdminAddress).to.be.equal(proxyAdminAddress); }); }); diff --git a/test/proxy/utils/UUPSUpgradeable.test.js b/test/proxy/utils/UUPSUpgradeable.test.js index 3351ddcb872..466081d2050 100644 --- a/test/proxy/utils/UUPSUpgradeable.test.js +++ b/test/proxy/utils/UUPSUpgradeable.test.js @@ -1,9 +1,11 @@ const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { web3 } = require('@openzeppelin/test-helpers/src/setup'); +const { getSlot, ImplementationSlot } = require('../../helpers/erc1967'); const ERC1967Proxy = artifacts.require('ERC1967Proxy'); const UUPSUpgradeableMock = artifacts.require('UUPSUpgradeableMock'); const UUPSUpgradeableUnsafeMock = artifacts.require('UUPSUpgradeableUnsafeMock'); -const UUPSUpgradeableBrokenMock = artifacts.require('UUPSUpgradeableBrokenMock'); +const UUPSUpgradeableLegacyMock = artifacts.require('UUPSUpgradeableLegacyMock'); const CountersImpl = artifacts.require('CountersImpl'); contract('UUPSUpgradeable', function (accounts) { @@ -11,7 +13,6 @@ contract('UUPSUpgradeable', function (accounts) { this.implInitial = await UUPSUpgradeableMock.new(); this.implUpgradeOk = await UUPSUpgradeableMock.new(); this.implUpgradeUnsafe = await UUPSUpgradeableUnsafeMock.new(); - this.implUpgradeBroken = await UUPSUpgradeableBrokenMock.new(); this.implUpgradeNonUUPS = await CountersImpl.new(); }); @@ -44,18 +45,11 @@ contract('UUPSUpgradeable', function (accounts) { expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeUnsafe.address }); }); - it('reject upgrade to broken upgradeable implementation', async function () { - await expectRevert( - this.instance.upgradeTo(this.implUpgradeBroken.address), - 'ERC1967Upgrade: upgrade breaks further upgrades', - ); - }); - // delegate to a non existing upgradeTo function causes a low level revert it('reject upgrade to non uups implementation', async function () { await expectRevert( this.instance.upgradeTo(this.implUpgradeNonUUPS.address), - 'Address: low-level delegate call failed', + 'ERC1967Upgrade: new implementation is not UUPS', ); }); @@ -63,10 +57,29 @@ contract('UUPSUpgradeable', function (accounts) { const { address } = await ERC1967Proxy.new(this.implInitial.address, '0x'); const otherInstance = await UUPSUpgradeableMock.at(address); - // infinite loop reverts when a nested call is out-of-gas await expectRevert( this.instance.upgradeTo(otherInstance.address), - 'Address: low-level delegate call failed', + 'ERC1967Upgrade: new implementation is not UUPS', ); }); + + it('can upgrade from legacy implementations', async function () { + const legacyImpl = await UUPSUpgradeableLegacyMock.new(); + const legacyInstance = await ERC1967Proxy.new(legacyImpl.address, '0x') + .then(({ address }) => UUPSUpgradeableLegacyMock.at(address)); + + const receipt = await legacyInstance.upgradeTo(this.implInitial.address); + + const UpgradedEvents = receipt.logs.filter(({ address, event }) => + address === legacyInstance.address && + event === 'Upgraded', + ); + expect(UpgradedEvents.length).to.be.equal(1); + + expectEvent(receipt, 'Upgraded', { implementation: this.implInitial.address }); + + const implementationSlot = await getSlot(legacyInstance, ImplementationSlot); + const implementationAddress = web3.utils.toChecksumAddress(implementationSlot.substr(-40)); + expect(implementationAddress).to.be.equal(this.implInitial.address); + }); }); From ecae978cb557d28937e180ea96d54e312a7a9fad Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jan 2022 19:56:36 +0100 Subject: [PATCH 03/26] Make more functions virtual (#3078) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 1 + contracts/access/AccessControl.sol | 6 +++--- contracts/access/AccessControlEnumerable.sol | 4 ++-- contracts/governance/Governor.sol | 2 +- contracts/token/ERC20/extensions/ERC20FlashMint.sol | 3 ++- contracts/token/ERC20/extensions/ERC20Votes.sol | 6 +++--- contracts/token/ERC20/extensions/ERC20VotesComp.sol | 4 ++-- contracts/token/ERC777/ERC777.sol | 2 +- contracts/utils/Multicall.sol | 2 +- 9 files changed, 16 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fdf8dd9c69..724af7ce22a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) * `ERC1967Upgrade`: Refactor the secure upgrade to use `ERC1822` instead of the previous rollback mechanism. This reduces code complexity and attack surface with similar security guarantees. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * `UUPSUpgradeable`: Add `ERC1822` compliance to support the updated secure upgrade mechanism. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) + * Some more functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. ### Breaking changes diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 95f53b3303d..70c81f6c268 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -81,7 +81,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { /** * @dev Returns `true` if `account` has been granted `role`. */ - function hasRole(bytes32 role, address account) public view override returns (bool) { + function hasRole(bytes32 role, address account) public view virtual override returns (bool) { return _roles[role].members[account]; } @@ -92,7 +92,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { * * /^AccessControl: account (0x[0-9a-f]{40}) is missing role (0x[0-9a-f]{64})$/ */ - function _checkRole(bytes32 role, address account) internal view { + function _checkRole(bytes32 role, address account) internal view virtual { if (!hasRole(role, account)) { revert( string( @@ -113,7 +113,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { * * To change a role's admin, use {_setRoleAdmin}. */ - function getRoleAdmin(bytes32 role) public view override returns (bytes32) { + function getRoleAdmin(bytes32 role) public view virtual override returns (bytes32) { return _roles[role].adminRole; } diff --git a/contracts/access/AccessControlEnumerable.sol b/contracts/access/AccessControlEnumerable.sol index cb1c88b65f1..153981e1345 100644 --- a/contracts/access/AccessControlEnumerable.sol +++ b/contracts/access/AccessControlEnumerable.sol @@ -34,7 +34,7 @@ abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessCon * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post] * for more information. */ - function getRoleMember(bytes32 role, uint256 index) public view override returns (address) { + function getRoleMember(bytes32 role, uint256 index) public view virtual override returns (address) { return _roleMembers[role].at(index); } @@ -42,7 +42,7 @@ abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessCon * @dev Returns the number of accounts that have `role`. Can be used * together with {getRoleMember} to enumerate all bearers of a role. */ - function getRoleMemberCount(bytes32 role) public view override returns (uint256) { + function getRoleMemberCount(bytes32 role) public view virtual override returns (uint256) { return _roleMembers[role].length(); } diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index e2cdc9c8e75..19a930ddb5d 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -370,7 +370,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { address target, uint256 value, bytes calldata data - ) external onlyGovernance { + ) external virtual onlyGovernance { Address.functionCallWithValue(target, data, value); } diff --git a/contracts/token/ERC20/extensions/ERC20FlashMint.sol b/contracts/token/ERC20/extensions/ERC20FlashMint.sol index da3780b25b0..f7b205bf7fe 100644 --- a/contracts/token/ERC20/extensions/ERC20FlashMint.sol +++ b/contracts/token/ERC20/extensions/ERC20FlashMint.sol @@ -23,7 +23,7 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender { * @param token The address of the token that is requested. * @return The amont of token that can be loaned. */ - function maxFlashLoan(address token) public view override returns (uint256) { + function maxFlashLoan(address token) public view virtual override returns (uint256) { return token == address(this) ? type(uint256).max - ERC20.totalSupply() : 0; } @@ -62,6 +62,7 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender { uint256 amount, bytes calldata data ) public virtual override returns (bool) { + require(amount <= maxFlashLoan(token), "ERC20FlashMint: amount exceeds maxFlashLoan"); uint256 fee = flashFee(token, amount); _mint(address(receiver), amount); require( diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index 90e9a251233..f6ae124d1c0 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -61,7 +61,7 @@ abstract contract ERC20Votes is IVotes, ERC20Permit { /** * @dev Gets the current votes balance for `account` */ - function getVotes(address account) public view override returns (uint256) { + function getVotes(address account) public view virtual override returns (uint256) { uint256 pos = _checkpoints[account].length; return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes; } @@ -73,7 +73,7 @@ abstract contract ERC20Votes is IVotes, ERC20Permit { * * - `blockNumber` must have been already mined */ - function getPastVotes(address account, uint256 blockNumber) public view override returns (uint256) { + function getPastVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) { require(blockNumber < block.number, "ERC20Votes: block not yet mined"); return _checkpointsLookup(_checkpoints[account], blockNumber); } @@ -86,7 +86,7 @@ abstract contract ERC20Votes is IVotes, ERC20Permit { * * - `blockNumber` must have been already mined */ - function getPastTotalSupply(uint256 blockNumber) public view override returns (uint256) { + function getPastTotalSupply(uint256 blockNumber) public view virtual override returns (uint256) { require(blockNumber < block.number, "ERC20Votes: block not yet mined"); return _checkpointsLookup(_totalSupplyCheckpoints, blockNumber); } diff --git a/contracts/token/ERC20/extensions/ERC20VotesComp.sol b/contracts/token/ERC20/extensions/ERC20VotesComp.sol index 422318bb8e8..3c122797cfa 100644 --- a/contracts/token/ERC20/extensions/ERC20VotesComp.sol +++ b/contracts/token/ERC20/extensions/ERC20VotesComp.sol @@ -26,14 +26,14 @@ abstract contract ERC20VotesComp is ERC20Votes { /** * @dev Comp version of the {getVotes} accessor, with `uint96` return type. */ - function getCurrentVotes(address account) external view returns (uint96) { + function getCurrentVotes(address account) external view virtual returns (uint96) { return SafeCast.toUint96(getVotes(account)); } /** * @dev Comp version of the {getPastVotes} accessor, with `uint96` return type. */ - function getPriorVotes(address account, uint256 blockNumber) external view returns (uint96) { + function getPriorVotes(address account, uint256 blockNumber) external view virtual returns (uint96) { return SafeCast.toUint96(getPastVotes(account, blockNumber)); } diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 302e3f1722e..9a77828118c 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -467,7 +467,7 @@ contract ERC777 is Context, IERC777, IERC20 { address holder, address spender, uint256 value - ) internal { + ) internal virtual { require(holder != address(0), "ERC777: approve from the zero address"); require(spender != address(0), "ERC777: approve to the zero address"); diff --git a/contracts/utils/Multicall.sol b/contracts/utils/Multicall.sol index 59291748bf1..d1c974c57c2 100644 --- a/contracts/utils/Multicall.sol +++ b/contracts/utils/Multicall.sol @@ -14,7 +14,7 @@ abstract contract Multicall { /** * @dev Receives and executes a batch of function calls on this contract. */ - function multicall(bytes[] calldata data) external returns (bytes[] memory results) { + function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) { results = new bytes[](data.length); for (uint256 i = 0; i < data.length; i++) { results[i] = Address.functionDelegateCall(address(this), data[i]); From 3eb2d43b0610758c6b85bf4b8ae160db3679c34d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jan 2022 23:27:04 +0100 Subject: [PATCH 04/26] Move abs(int256) from Math to SafeMath (#3110) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 2 +- contracts/mocks/MathMock.sol | 4 ---- contracts/mocks/SignedMathMock.sol | 4 ++++ contracts/utils/math/Math.sol | 10 ---------- contracts/utils/math/SignedMath.sol | 10 ++++++++++ test/utils/math/Math.test.js | 18 +----------------- test/utils/math/SignedMath.test.js | 16 ++++++++++++++++ 7 files changed, 32 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 724af7ce22a..4a70ffd7a68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,6 @@ * `ERC2891`: add implementation of the royalty standard, and the respective extensions for `ERC721` and `ERC1155`. ([#3012](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3012)) * `GovernorTimelockControl`: improve the `state()` function to have it reflect cases where a proposal has been canceled directly on the timelock. ([#2977](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2977)) - * `Math`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) * Preset contracts are now deprecated in favor of [Contracts Wizard](https://wizard.openzeppelin.com). ([#2986](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2986)) * `Governor`: add a relay function to help recover assets sent to a governor that is not its own executor (e.g. when using a timelock). ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) * `GovernorPreventLateQuorum`: add new module to ensure a minimum voting duration is available after the quorum is reached. ([#2973](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2973)) @@ -17,6 +16,7 @@ * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) + * `SignedMath`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) * `ERC1967Upgrade`: Refactor the secure upgrade to use `ERC1822` instead of the previous rollback mechanism. This reduces code complexity and attack surface with similar security guarantees. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * `UUPSUpgradeable`: Add `ERC1822` compliance to support the updated secure upgrade mechanism. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) * Some more functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. diff --git a/contracts/mocks/MathMock.sol b/contracts/mocks/MathMock.sol index f6c11acb4a6..c651b6bb1fd 100644 --- a/contracts/mocks/MathMock.sol +++ b/contracts/mocks/MathMock.sol @@ -20,8 +20,4 @@ contract MathMock { function ceilDiv(uint256 a, uint256 b) public pure returns (uint256) { return Math.ceilDiv(a, b); } - - function abs(int256 n) public pure returns (uint256) { - return Math.abs(n); - } } diff --git a/contracts/mocks/SignedMathMock.sol b/contracts/mocks/SignedMathMock.sol index 8d82e137e0c..5a0b2709654 100644 --- a/contracts/mocks/SignedMathMock.sol +++ b/contracts/mocks/SignedMathMock.sol @@ -16,4 +16,8 @@ contract SignedMathMock { function average(int256 a, int256 b) public pure returns (int256) { return SignedMath.average(a, b); } + + function abs(int256 n) public pure returns (uint256) { + return SignedMath.abs(n); + } } diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index c25134321c0..03d5218453b 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -40,14 +40,4 @@ library Math { // (a + b - 1) / b can overflow on addition, so we distribute. return a / b + (a % b == 0 ? 0 : 1); } - - /** - * @dev Returns the absolute unsigned value of a signed value. - */ - function abs(int256 n) internal pure returns (uint256) { - unchecked { - // must be unchecked in order to support `n = type(int256).min` - return uint256(n >= 0 ? n : -n); - } - } } diff --git a/contracts/utils/math/SignedMath.sol b/contracts/utils/math/SignedMath.sol index 96160910444..2c47aa0bfaf 100644 --- a/contracts/utils/math/SignedMath.sol +++ b/contracts/utils/math/SignedMath.sol @@ -29,4 +29,14 @@ library SignedMath { int256 x = (a & b) + ((a ^ b) >> 1); return x + (int256(uint256(x) >> 255) & (a ^ b)); } + + /** + * @dev Returns the absolute unsigned value of a signed value. + */ + function abs(int256 n) internal pure returns (uint256) { + unchecked { + // must be unchecked in order to support `n = type(int256).min` + return uint256(n >= 0 ? n : -n); + } + } } diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index eff64b72097..7e194dec713 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -1,6 +1,6 @@ const { BN, constants } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { MAX_UINT256, MAX_INT256, MIN_INT256 } = constants; +const { MAX_UINT256 } = constants; const MathMock = artifacts.require('MathMock'); @@ -85,20 +85,4 @@ contract('Math', function (accounts) { expect(await this.math.ceilDiv(MAX_UINT256, b)).to.be.bignumber.equal(MAX_UINT256); }); }); - - describe('abs', function () { - for (const n of [ - MIN_INT256, - MIN_INT256.addn(1), - new BN('-1'), - new BN('0'), - new BN('1'), - MAX_INT256.subn(1), - MAX_INT256, - ]) { - it(`correctly computes the absolute value of ${n}`, async function () { - expect(await this.math.abs(n)).to.be.bignumber.equal(n.abs()); - }); - } - }); }); diff --git a/test/utils/math/SignedMath.test.js b/test/utils/math/SignedMath.test.js index 0aef556dfbe..8e9826f028a 100644 --- a/test/utils/math/SignedMath.test.js +++ b/test/utils/math/SignedMath.test.js @@ -74,4 +74,20 @@ contract('SignedMath', function (accounts) { } }); }); + + describe('abs', function () { + for (const n of [ + MIN_INT256, + MIN_INT256.addn(1), + new BN('-1'), + new BN('0'), + new BN('1'), + MAX_INT256.subn(1), + MAX_INT256, + ]) { + it(`correctly computes the absolute value of ${n}`, async function () { + expect(await this.math.abs(n)).to.be.bignumber.equal(n.abs()); + }); + } + }); }); From 25eeb80b188876b951e592a810785173495097fc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 16 Jan 2022 01:02:27 +0100 Subject: [PATCH 05/26] Fix broken pull request links in change log (#3114) Co-authored-by: Han Lin Yap --- CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a70ffd7a68..b99c31689c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,10 @@ * `Votes`: Added a base contract for vote tracking with delegation. ([#2944](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2944)) * `ERC721Votes`: Added an extension of ERC721 enabled with vote tracking and delegation. ([#2944](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2944)) * `ERC2771Context`: use immutable storage to store the forwarder address, no longer an issue since Solidity >=0.8.8 allows reading immutable variables in the constructor. ([#2917](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2917)) - * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs`. ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) - * `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056)) - * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) - * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) + * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs`. ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2884)) + * `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3056)) + * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3085)) + * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3085)) * `SignedMath`: a new signed version of the Math library with `max`, `min`, and `average`. ([#2686](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2686)) * `SignedMath`: add a `abs(int256)` method that returns the unsigned absolute value of a signed value. ([#2984](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2984)) * `ERC1967Upgrade`: Refactor the secure upgrade to use `ERC1822` instead of the previous rollback mechanism. This reduces code complexity and attack surface with similar security guarantees. ([#3021](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3021)) @@ -29,7 +29,7 @@ ## 4.4.2 (2022-01-11) ### Bugfixes - * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposals submitted through the compatibility interface with explicit signatures. ([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3100)) + * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposals submitted through the compatibility interface with explicit signatures. ([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3100)) ## 4.4.1 (2021-12-14) From 783ac759a902a7b4a218c2d026a77e6a26b6c42d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 18 Jan 2022 04:05:17 +0100 Subject: [PATCH 06/26] upgrade solidity-coverage to 0.7.18 to support ERC165 coverage (#3117) --- package-lock.json | 14 +++++++------- package.json | 2 +- .../introspection/SupportsInterface.behavior.js | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4e6493a1971..aaf68d17c7c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -43,7 +43,7 @@ "semver": "^7.3.5", "solhint": "^3.3.6", "solidity-ast": "^0.4.25", - "solidity-coverage": "^0.7.11", + "solidity-coverage": "^0.7.18", "solidity-docgen": "^0.5.3", "web3": "^1.3.0", "yargs": "^16.2.0" @@ -16579,9 +16579,9 @@ "dev": true }, "node_modules/solidity-coverage": { - "version": "0.7.17", - "resolved": "https://registry.npmjs.org/solidity-coverage/-/solidity-coverage-0.7.17.tgz", - "integrity": "sha512-Erw2hd2xdACAvDX8jUdYkmgJlIIazGznwDJA5dhRaw4def2SisXN9jUjneeyOZnl/E7j6D3XJYug4Zg9iwodsg==", + "version": "0.7.18", + "resolved": "https://registry.npmjs.org/solidity-coverage/-/solidity-coverage-0.7.18.tgz", + "integrity": "sha512-H1UhB9QqLISJPgttaulnStUyOaJm0wZXvBGWUN9+HH3dl2hYjSmo3RBBFRm7U+dBNpPysS835XFGe+hG4ZhMrA==", "dev": true, "dependencies": { "@solidity-parser/parser": "^0.13.2", @@ -33568,9 +33568,9 @@ "dev": true }, "solidity-coverage": { - "version": "0.7.17", - "resolved": "https://registry.npmjs.org/solidity-coverage/-/solidity-coverage-0.7.17.tgz", - "integrity": "sha512-Erw2hd2xdACAvDX8jUdYkmgJlIIazGznwDJA5dhRaw4def2SisXN9jUjneeyOZnl/E7j6D3XJYug4Zg9iwodsg==", + "version": "0.7.18", + "resolved": "https://registry.npmjs.org/solidity-coverage/-/solidity-coverage-0.7.18.tgz", + "integrity": "sha512-H1UhB9QqLISJPgttaulnStUyOaJm0wZXvBGWUN9+HH3dl2hYjSmo3RBBFRm7U+dBNpPysS835XFGe+hG4ZhMrA==", "dev": true, "requires": { "@solidity-parser/parser": "^0.13.2", diff --git a/package.json b/package.json index 0b0abb33afa..8e3c041efbb 100644 --- a/package.json +++ b/package.json @@ -81,7 +81,7 @@ "semver": "^7.3.5", "solhint": "^3.3.6", "solidity-ast": "^0.4.25", - "solidity-coverage": "^0.7.11", + "solidity-coverage": "^0.7.18", "solidity-docgen": "^0.5.3", "web3": "^1.3.0", "yargs": "^16.2.0" diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 3f6f5228b64..4f29f0e00d9 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -99,11 +99,11 @@ function shouldSupportInterfaces (interfaces = []) { const interfaceId = INTERFACE_IDS[k]; describe(k, function () { describe('ERC165\'s supportsInterface(bytes4)', function () { - it('uses less than 30k gas [skip-on-coverage]', async function () { + it('uses less than 30k gas', async function () { expect(await this.contractUnderTest.supportsInterface.estimateGas(interfaceId)).to.be.lte(30000); }); - it('claims support [skip-on-coverage]', async function () { + it('claims support', async function () { expect(await this.contractUnderTest.supportsInterface(interfaceId)).to.equal(true); }); }); From b3b83b558ebb9982e27ae5ee0bb5f33f278863dd Mon Sep 17 00:00:00 2001 From: JulissaDantes Date: Thu, 20 Jan 2022 20:15:54 -0400 Subject: [PATCH 07/26] Add Slither reentrancy check in CI (#3047) Co-authored-by: Francisco Giordano --- .github/workflows/test.yml | 25 ++++++++++++++++++- contracts/governance/TimelockController.sol | 3 +++ .../extensions/GovernorTimelockControl.sol | 3 +++ .../token/ERC20/extensions/ERC20FlashMint.sol | 3 +++ package.json | 3 ++- 5 files changed, 35 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8d13372bf55..a39fc9534e6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,7 +29,7 @@ jobs: env: FORCE_COLOR: 1 ENABLE_GAS_REPORT: true - - run: npm run test:inheritance + - run: npm run test:inheritance - name: Print gas report run: cat gas-report.txt @@ -54,3 +54,26 @@ jobs: env: NODE_OPTIONS: --max_old_space_size=4096 - uses: codecov/codecov-action@v2 + + slither: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions/setup-node@v2 + with: + node-version: 12.x + - uses: actions/cache@v2 + id: cache + with: + path: '**/node_modules' + key: npm-v2-${{ hashFiles('**/package-lock.json') }} + restore-keys: npm-v2- + - run: npm ci + if: steps.cache.outputs.cache-hit != 'true' + - name: Set up Python + uses: actions/setup-python@v2 + + - name: Install dependencies + run: pip3 install slither-analyzer + - name: Summary of static analysis + run: npm run slither diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 6e2f7a55d22..c375f074456 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -261,6 +261,9 @@ contract TimelockController is AccessControl { * * - the caller must have the 'executor' role. */ + // This function can reenter, but it doesn't pose a risk because _afterCall checks that the proposal is pending, + // thus any modifications to the operation during reentrancy should be caught. + // slither-disable-next-line reentrancy-eth function execute( address target, uint256 value, diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 5be6e9c81bc..21b481781a2 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -122,6 +122,9 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { * @dev Overriden version of the {Governor-_cancel} function to cancel the timelocked proposal if it as already * been queued. */ + // This function can reenter through the external call to the timelock, but we assume the timelock is trusted and + // well behaved (according to TimelockController) and this will not happen. + // slither-disable-next-line reentrancy-no-eth function _cancel( address[] memory targets, uint256[] memory values, diff --git a/contracts/token/ERC20/extensions/ERC20FlashMint.sol b/contracts/token/ERC20/extensions/ERC20FlashMint.sol index f7b205bf7fe..a1ebbc9e657 100644 --- a/contracts/token/ERC20/extensions/ERC20FlashMint.sol +++ b/contracts/token/ERC20/extensions/ERC20FlashMint.sol @@ -56,6 +56,9 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender { * @param data An arbitrary datafield that is passed to the receiver. * @return `true` is the flash loan was successful. */ + // This function can reenter, but it doesn't pose a risk because it always preserves the property that the amount + // minted at the beginning is always recovered and burned at the end, or else the entire function will revert. + // slither-disable-next-line reentrancy-no-eth function flashLoan( IERC3156FlashBorrower receiver, address token, diff --git a/package.json b/package.json index 8e3c041efbb..81a3fb8ab5c 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,8 @@ "version": "scripts/release/version.sh", "test": "hardhat test", "test:inheritance": "node scripts/inheritanceOrdering artifacts/build-info/*", - "gas-report": "env ENABLE_GAS_REPORT=true npm run test" + "gas-report": "env ENABLE_GAS_REPORT=true npm run test", + "slither": "npm run clean && slither . --detect reentrancy-eth,reentrancy-no-eth,reentrancy-unlimited-gas" }, "repository": { "type": "git", From 8f70c8867e31d2d2f212ecea079b1f1afecb0440 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Baig Date: Sun, 23 Jan 2022 15:05:41 +0500 Subject: [PATCH 08/26] Fix typo in ERC721.sol (#3127) --- contracts/token/ERC721/ERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 7b26343ac66..b88a30bbb7b 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -100,7 +100,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { /** * @dev Base URI for computing {tokenURI}. If set, the resulting URI for each * token will be the concatenation of the `baseURI` and the `tokenId`. Empty - * by default, can be overriden in child contracts. + * by default, can be overridden in child contracts. */ function _baseURI() internal view virtual returns (string memory) { return ""; From 7c47ac71939e2c525cce5206563b7bf3f6d03e23 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 23 Jan 2022 14:25:48 -0300 Subject: [PATCH 09/26] Add workflow to generate and update docs branches --- .github/workflows/docs.yml | 25 ++++++++++++++++ scripts/git-user-config.sh | 6 ++++ scripts/update-docs-branch.js | 55 +++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+) create mode 100644 .github/workflows/docs.yml create mode 100644 scripts/git-user-config.sh create mode 100644 scripts/update-docs-branch.js diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml new file mode 100644 index 00000000000..6f5ca62d6dc --- /dev/null +++ b/.github/workflows/docs.yml @@ -0,0 +1,25 @@ +name: Build Docs + +on: + push: + branches: [release-v*] + +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions/setup-node@v2 + with: + node-version: 12.x + - uses: actions/cache@v2 + id: cache + with: + path: '**/node_modules' + key: npm-v2-${{ hashFiles('**/package-lock.json') }} + restore-keys: npm-v2- + - run: npm ci + if: steps.cache.outputs.cache-hit != 'true' + - run: bash scripts/git-user-config.sh + - run: node scripts/update-docs-branch.js + - run: git push --all origin diff --git a/scripts/git-user-config.sh b/scripts/git-user-config.sh new file mode 100644 index 00000000000..e7b81c3eb3e --- /dev/null +++ b/scripts/git-user-config.sh @@ -0,0 +1,6 @@ +#!/usr/bin/env bash + +set -euo pipefail -x + +git config user.name 'github-actions' +git config user.email '41898282+github-actions[bot]@users.noreply.github.com' diff --git a/scripts/update-docs-branch.js b/scripts/update-docs-branch.js new file mode 100644 index 00000000000..9a99b5c9abf --- /dev/null +++ b/scripts/update-docs-branch.js @@ -0,0 +1,55 @@ +const proc = require('child_process'); +const read = cmd => proc.execSync(cmd, { encoding: 'utf8' }).trim(); +const run = cmd => { proc.execSync(cmd, { stdio: 'inherit' }); }; +const tryRead = cmd => { try { return read(cmd); } catch (e) { return undefined; } }; + +const releaseBranchRegex = /^release-v(?(?\d+)\.(?\d+)(?:\.(?\d+))?)$/; + +const currentBranch = read(`git rev-parse --abbrev-ref HEAD`); +const match = currentBranch.match(releaseBranchRegex); + +if (!match) { + console.error(`Not currently on a release branch`); + process.exit(1); +} + +if (/-.*$/.test(require('../package.json').version)) { + console.error(`Refusing to update docs: prerelease detected`); + process.exit(0); +} + +const current = match.groups; +const docsBranch = `docs-v${current.major}.x`; + +// Fetch remotes and find the docs branch if it exists +run(`git fetch --all --no-tags`); +const matchingDocsBranches = tryRead(`git rev-parse --glob='*/${docsBranch}'`); + +if (!matchingDocsBranches) { + // Create the branch + run(`git checkout --orphan ${docsBranch}`); +} else { + const [publishedRef, ...others] = new Set(matchingDocsBranches.split('\n')); + if (others.length > 0) { + console.error( + `Found conflicting ${docsBranch} branches.\n` + + `Either local branch is outdated or there are multiple matching remote branches.` + ); + process.exit(1); + } + const publishedVersion = JSON.parse(read(`git show ${publishedRef}:package.json`)).version; + const publishedMinor = publishedVersion.match(/\d+\.(?\d+)\.\d+/).groups.minor; + if (current.minor < publishedMinor) { + console.error(`Refusing to update docs: newer version is published`); + process.exit(0); + } + + run(`git checkout --quiet --detach`); + run(`git reset --soft ${publishedRef}`); + run(`git checkout ${docsBranch}`); +} + +run(`npm run prepare-docs`); +run(`git add -f docs`); // --force needed because generated docs files are gitignored +run(`git commit -m "Update docs"`); +run(`git checkout ${currentBranch}`); From d57593c148dad16abe675083464787ca10f789ec Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 24 Jan 2022 00:42:17 -0300 Subject: [PATCH 10/26] Lint --- scripts/update-docs-branch.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/scripts/update-docs-branch.js b/scripts/update-docs-branch.js index 9a99b5c9abf..82bb7e0606c 100644 --- a/scripts/update-docs-branch.js +++ b/scripts/update-docs-branch.js @@ -5,16 +5,16 @@ const tryRead = cmd => { try { return read(cmd); } catch (e) { return undefined; const releaseBranchRegex = /^release-v(?(?\d+)\.(?\d+)(?:\.(?\d+))?)$/; -const currentBranch = read(`git rev-parse --abbrev-ref HEAD`); +const currentBranch = read('git rev-parse --abbrev-ref HEAD'); const match = currentBranch.match(releaseBranchRegex); if (!match) { - console.error(`Not currently on a release branch`); + console.error('Not currently on a release branch'); process.exit(1); } if (/-.*$/.test(require('../package.json').version)) { - console.error(`Refusing to update docs: prerelease detected`); + console.error('Refusing to update docs: prerelease detected'); process.exit(0); } @@ -22,7 +22,7 @@ const current = match.groups; const docsBranch = `docs-v${current.major}.x`; // Fetch remotes and find the docs branch if it exists -run(`git fetch --all --no-tags`); +run('git fetch --all --no-tags'); const matchingDocsBranches = tryRead(`git rev-parse --glob='*/${docsBranch}'`); if (!matchingDocsBranches) { @@ -32,24 +32,24 @@ if (!matchingDocsBranches) { const [publishedRef, ...others] = new Set(matchingDocsBranches.split('\n')); if (others.length > 0) { console.error( - `Found conflicting ${docsBranch} branches.\n` - + `Either local branch is outdated or there are multiple matching remote branches.` + `Found conflicting ${docsBranch} branches.\n` + + 'Either local branch is outdated or there are multiple matching remote branches.', ); process.exit(1); } const publishedVersion = JSON.parse(read(`git show ${publishedRef}:package.json`)).version; const publishedMinor = publishedVersion.match(/\d+\.(?\d+)\.\d+/).groups.minor; if (current.minor < publishedMinor) { - console.error(`Refusing to update docs: newer version is published`); + console.error('Refusing to update docs: newer version is published'); process.exit(0); } - run(`git checkout --quiet --detach`); + run('git checkout --quiet --detach'); run(`git reset --soft ${publishedRef}`); run(`git checkout ${docsBranch}`); } -run(`npm run prepare-docs`); -run(`git add -f docs`); // --force needed because generated docs files are gitignored -run(`git commit -m "Update docs"`); +run('npm run prepare-docs'); +run('git add -f docs'); // --force needed because generated docs files are gitignored +run('git commit -m "Update docs"'); run(`git checkout ${currentBranch}`); From a5e042cedf6205513466280a5b778c4d81ea7a9d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 24 Jan 2022 22:55:12 +0100 Subject: [PATCH 11/26] Fix use of ^ (xor) instead of ** (power) (#3130) --- docs/modules/ROOT/pages/erc20.adoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/modules/ROOT/pages/erc20.adoc b/docs/modules/ROOT/pages/erc20.adoc index 11b650a1eb1..97cb1ac6749 100644 --- a/docs/modules/ROOT/pages/erc20.adoc +++ b/docs/modules/ROOT/pages/erc20.adoc @@ -58,9 +58,9 @@ To work around this, xref:api:token/ERC20.adoc#ERC20[`ERC20`] provides a xref:ap How can this be achieved? It's actually very simple: a token contract can use larger integer values, so that a balance of `50` will represent `5 GLD`, a transfer of `15` will correspond to `1.5 GLD` being sent, and so on. -It is important to understand that `decimals` is _only used for display purposes_. All arithmetic inside the contract is still performed on integers, and it is the different user interfaces (wallets, exchanges, etc.) that must adjust the displayed values according to `decimals`. The total token supply and balance of each account are not specified in `GLD`: you need to divide by `10^decimals` to get the actual `GLD` amount. +It is important to understand that `decimals` is _only used for display purposes_. All arithmetic inside the contract is still performed on integers, and it is the different user interfaces (wallets, exchanges, etc.) that must adjust the displayed values according to `decimals`. The total token supply and balance of each account are not specified in `GLD`: you need to divide by `10 ** decimals` to get the actual `GLD` amount. -You'll probably want to use a `decimals` value of `18`, just like Ether and most ERC20 token contracts in use, unless you have a very special reason not to. When minting tokens or transferring them around, you will be actually sending the number `num GLD * 10^decimals`. +You'll probably want to use a `decimals` value of `18`, just like Ether and most ERC20 token contracts in use, unless you have a very special reason not to. When minting tokens or transferring them around, you will be actually sending the number `num GLD * (10 ** decimals)`. NOTE: By default, `ERC20` uses a value of `18` for `decimals`. To use a different value, you will need to override the `decimals()` function in your contract. @@ -73,7 +73,7 @@ function decimals() public view virtual override returns (uint8) { So if you want to send `5` tokens using a token contract with 18 decimals, the method to call will actually be: ```solidity -transfer(recipient, 5 * 10^18); +transfer(recipient, 5 * (10 ** 18)); ``` [[Presets]] From e298476a906d36c8db6cb5bc3c7d0c4a5ddf74f9 Mon Sep 17 00:00:00 2001 From: Kevin Upton Date: Wed, 26 Jan 2022 00:56:13 +1100 Subject: [PATCH 12/26] Simplification of ERC777's transfer & transferFrom by using _send (#3128) * Update ERC777.sol * Update ERC777.sol * Update ERC777.sol * Update ERC777.sol * fix revert reasons Co-authored-by: Hadrien Croubois --- contracts/token/ERC777/ERC777.sol | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 9a77828118c..1ef6aaf55fd 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -144,16 +144,7 @@ contract ERC777 is Context, IERC777, IERC20 { * Also emits a {Sent} event. */ function transfer(address recipient, uint256 amount) public virtual override returns (bool) { - require(recipient != address(0), "ERC777: transfer to the zero address"); - - address from = _msgSender(); - - _callTokensToSend(from, from, recipient, amount, "", ""); - - _move(from, from, recipient, amount, "", ""); - - _callTokensReceived(from, from, recipient, amount, "", "", false); - + _send(_msgSender(), recipient, amount, "", "", false); return true; } @@ -286,13 +277,7 @@ contract ERC777 is Context, IERC777, IERC20 { address recipient, uint256 amount ) public virtual override returns (bool) { - require(recipient != address(0), "ERC777: transfer to the zero address"); - require(holder != address(0), "ERC777: transfer from the zero address"); - address spender = _msgSender(); - - _callTokensToSend(spender, holder, recipient, amount, "", ""); - uint256 currentAllowance = _allowances[holder][spender]; if (currentAllowance != type(uint256).max) { require(currentAllowance >= amount, "ERC777: transfer amount exceeds allowance"); @@ -301,9 +286,7 @@ contract ERC777 is Context, IERC777, IERC20 { } } - _move(spender, holder, recipient, amount, "", ""); - - _callTokensReceived(spender, holder, recipient, amount, "", "", false); + _send(holder, recipient, amount, "", "", false); return true; } @@ -392,8 +375,8 @@ contract ERC777 is Context, IERC777, IERC20 { bytes memory operatorData, bool requireReceptionAck ) internal virtual { - require(from != address(0), "ERC777: send from the zero address"); - require(to != address(0), "ERC777: send to the zero address"); + require(from != address(0), "ERC777: transfer from the zero address"); + require(to != address(0), "ERC777: transfer to the zero address"); address operator = _msgSender(); From 78deae5a7678ea76c4af9f264fc7d4ac72b42118 Mon Sep 17 00:00:00 2001 From: GitHubPang <61439577+GitHubPang@users.noreply.github.com> Date: Wed, 26 Jan 2022 16:30:53 +0800 Subject: [PATCH 13/26] Fix typo in CHANGELOG (#3135) Change `ERC2891` > `ERC2981`. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b99c31689c9..d3d72db4fa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased - * `ERC2891`: add implementation of the royalty standard, and the respective extensions for `ERC721` and `ERC1155`. ([#3012](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3012)) + * `ERC2981`: add implementation of the royalty standard, and the respective extensions for `ERC721` and `ERC1155`. ([#3012](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3012)) * `GovernorTimelockControl`: improve the `state()` function to have it reflect cases where a proposal has been canceled directly on the timelock. ([#2977](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2977)) * Preset contracts are now deprecated in favor of [Contracts Wizard](https://wizard.openzeppelin.com). ([#2986](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2986)) * `Governor`: add a relay function to help recover assets sent to a governor that is not its own executor (e.g. when using a timelock). ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) From 6fb1e843cf05e1cf894a68ffd52c9cb89fd9496f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 26 Jan 2022 17:35:05 +0100 Subject: [PATCH 14/26] Make royaltyInfo(uint256 _tokenId, uint256 _salePrice) virtual (#3133) * Make royaltyInfo(uint256 _tokenId, uint256 _salePrice) virtual Should be cherrypicked in release 4.5 * fix lint --- contracts/token/common/ERC2981.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/token/common/ERC2981.sol b/contracts/token/common/ERC2981.sol index 56260a25aed..f6617bbbb3f 100644 --- a/contracts/token/common/ERC2981.sol +++ b/contracts/token/common/ERC2981.sol @@ -40,7 +40,13 @@ abstract contract ERC2981 is IERC2981, ERC165 { /** * @inheritdoc IERC2981 */ - function royaltyInfo(uint256 _tokenId, uint256 _salePrice) external view override returns (address, uint256) { + function royaltyInfo(uint256 _tokenId, uint256 _salePrice) + external + view + virtual + override + returns (address, uint256) + { RoyaltyInfo memory royalty = _tokenRoyaltyInfo[_tokenId]; if (royalty.receiver == address(0)) { From fb950c616694a6fb213fb1e784fadbfed4ec394c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 26 Jan 2022 21:36:10 +0100 Subject: [PATCH 15/26] Add a virtual `_checkRole(bytes32)` internal function to `AccessControl` (#3137) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add a virtual _onlyRole(bytes32) modifier * _onlyRole(role) → _checkRole(role) * update doc --- CHANGELOG.md | 4 ++++ contracts/access/AccessControl.sol | 14 +++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3d72db4fa0..94e575810a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + + * `AccessControl`: add a virtual `_checkRole(bytes32)` function that can be overriden to alter the the `onlyRole` modifier behavior. ([#3137](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3137)) + ## Unreleased * `ERC2981`: add implementation of the royalty standard, and the respective extensions for `ERC721` and `ERC1155`. ([#3012](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3012)) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 70c81f6c268..f18448770bc 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -67,7 +67,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { * _Available since v4.1._ */ modifier onlyRole(bytes32 role) { - _checkRole(role, _msgSender()); + _checkRole(role); _; } @@ -85,6 +85,18 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { return _roles[role].members[account]; } + /** + * @dev Revert with a standard message if `_msgSender()` is missing `role`. + * Overriding this function changes the behavior of the {onlyRole} modifier. + * + * Format of the revert message is described in {_checkRole}. + * + * _Available since v4.6._ + */ + function _checkRole(bytes32 role) internal view virtual { + _checkRole(role, _msgSender()); + } + /** * @dev Revert with a standard message if `account` is missing `role`. * From ae54e6de1d77c60881e4c85ffbdb7f9d76b71efe Mon Sep 17 00:00:00 2001 From: GitHubPang <61439577+GitHubPang@users.noreply.github.com> Date: Thu, 27 Jan 2022 16:59:11 +0800 Subject: [PATCH 16/26] Fix typo in CHANGELOG (#3138) Remove repeated word "the". --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94e575810a6..e878c2c43b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased - * `AccessControl`: add a virtual `_checkRole(bytes32)` function that can be overriden to alter the the `onlyRole` modifier behavior. ([#3137](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3137)) + * `AccessControl`: add a virtual `_checkRole(bytes32)` function that can be overriden to alter the `onlyRole` modifier behavior. ([#3137](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3137)) ## Unreleased From f55d2716a8fa67a30e191bac78d7531ebfae838d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 28 Jan 2022 22:44:32 -0300 Subject: [PATCH 17/26] Add function documentation for SignatureChecker. --- .../utils/cryptography/SignatureChecker.sol | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index fe1bebc8e08..4c39facc41b 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -8,16 +8,20 @@ import "../Address.sol"; import "../../interfaces/IERC1271.sol"; /** - * @dev Signature verification helper: Provide a single mechanism to verify both private-key (EOA) ECDSA signature and - * ERC1271 contract signatures. Using this instead of ECDSA.recover in your contract will make them compatible with - * smart contract wallets such as Argent and Gnosis. - * - * Note: unlike ECDSA signatures, contract signature's are revocable, and the outcome of this function can thus change - * through time. It could return true at block N and false at block N+1 (or the opposite). + * @dev Signature verification helper that can be used instead of `ECDSA.recover` to seamlessly support both ECDSA + * signatures from externally owned accounts (EOAs) as well as ERC1271 signatures from smart contract wallets like + * Argent and Gnosis Safe. * * _Available since v4.1._ */ library SignatureChecker { + /** + * @dev Checks if a signature is valid for a given signer and data hash. If the signer is a smart contract, the + * signature is validated against that smart contract using ERC1271, otherwise it's validated using `ECDSA.recover`. + * + * NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus + * change through time. It could return true at block N and false at block N+1 (or the opposite). + */ function isValidSignatureNow( address signer, bytes32 hash, From a81b07ce910b288886faf28453a1575094e65298 Mon Sep 17 00:00:00 2001 From: Harsh Vakharia <2311316+harshjv@users.noreply.github.com> Date: Mon, 31 Jan 2022 15:17:56 +0530 Subject: [PATCH 18/26] Fix typo in retrieval of onERC721Received selector (#3151) --- contracts/token/ERC721/IERC721Receiver.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/IERC721Receiver.sol b/contracts/token/ERC721/IERC721Receiver.sol index a42cb52ff3c..298e3565f75 100644 --- a/contracts/token/ERC721/IERC721Receiver.sol +++ b/contracts/token/ERC721/IERC721Receiver.sol @@ -16,7 +16,7 @@ interface IERC721Receiver { * It must return its Solidity selector to confirm the token transfer. * If any other value is returned or the interface is not implemented by the recipient, the transfer will be reverted. * - * The selector can be obtained in Solidity with `IERC721.onERC721Received.selector`. + * The selector can be obtained in Solidity with `IERC721Receiver.onERC721Received.selector`. */ function onERC721Received( address operator, From 4f8af2dceb0fbc36cb32eb2cc14f80c340b9022e Mon Sep 17 00:00:00 2001 From: Doug Hoyte Date: Mon, 31 Jan 2022 10:10:13 -0500 Subject: [PATCH 19/26] Add test and docs describing a misuse of MerkleProof (#3090) Co-authored-by: Francisco Giordano --- contracts/utils/cryptography/MerkleProof.sol | 7 ++++++- test/utils/cryptography/MerkleProof.test.js | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/contracts/utils/cryptography/MerkleProof.sol b/contracts/utils/cryptography/MerkleProof.sol index b0fe49416b8..c0621a73b30 100644 --- a/contracts/utils/cryptography/MerkleProof.sol +++ b/contracts/utils/cryptography/MerkleProof.sol @@ -11,6 +11,11 @@ pragma solidity ^0.8.0; * Note: the hashing algorithm should be keccak256 and pair sorting should be enabled. * * See `test/utils/cryptography/MerkleProof.test.js` for some examples. + * + * WARNING: You should avoid using leaf values that are 64 bytes long prior to + * hashing, or use a hash function other than keccak256 for hashing leaves. + * This is because the concatenation of a sorted pair of internal nodes in + * the merkle tree could be reinterpreted as a leaf value. */ library MerkleProof { /** @@ -28,7 +33,7 @@ library MerkleProof { } /** - * @dev Returns the rebuilt hash obtained by traversing a Merklee tree up + * @dev Returns the rebuilt hash obtained by traversing a Merkle tree up * from `leaf` using `proof`. A `proof` is valid if and only if the rebuilt * hash matches the root of the tree. When processing the proof, the pairs * of leafs & pre-images are assumed to be sorted. diff --git a/test/utils/cryptography/MerkleProof.test.js b/test/utils/cryptography/MerkleProof.test.js index dab2062d499..61fa45c3ee6 100644 --- a/test/utils/cryptography/MerkleProof.test.js +++ b/test/utils/cryptography/MerkleProof.test.js @@ -24,6 +24,12 @@ contract('MerkleProof', function (accounts) { const proof = merkleTree.getHexProof(leaf); expect(await this.merkleProof.verify(proof, root, leaf)).to.equal(true); + + // For demonstration, it is also possible to create valid proofs for certain 64-byte values *not* in elements: + const noSuchLeaf = keccak256( + Buffer.concat([keccak256(elements[0]), keccak256(elements[1])].sort(Buffer.compare)), + ); + expect(await this.merkleProof.verify(proof.slice(1), root, noSuchLeaf)).to.equal(true); }); it('returns false for an invalid Merkle proof', async function () { From 21c5d623d6d27aa33357cbfe3f39a0b3b31cbbfe Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 31 Jan 2022 17:33:42 -0300 Subject: [PATCH 20/26] Update Copyright notice and include contributors --- LICENSE | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/LICENSE b/LICENSE index ade2b707e2f..4f51be0f010 100644 --- a/LICENSE +++ b/LICENSE @@ -1,6 +1,6 @@ The MIT License (MIT) -Copyright (c) 2016-2020 zOS Global Limited +Copyright (c) 2016-2022 zOS Global Limited and contributors Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the From fc01c51c130b9f9a8638cb3e86a43175bdf21c36 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 31 Jan 2022 22:05:20 -0300 Subject: [PATCH 21/26] Simplify inheritance to avoid overrides --- docs/modules/ROOT/pages/erc721.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/erc721.adoc b/docs/modules/ROOT/pages/erc721.adoc index 7a068e5c7ad..25dde116208 100644 --- a/docs/modules/ROOT/pages/erc721.adoc +++ b/docs/modules/ROOT/pages/erc721.adoc @@ -20,7 +20,7 @@ import "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol"; import "@openzeppelin/contracts/utils/Counters.sol"; -contract GameItem is ERC721, ERC721URIStorage { +contract GameItem is ERC721URIStorage { using Counters for Counters.Counter; Counters.Counter private _tokenIds; From ca755ce7999720484574718c52b129843faa868e Mon Sep 17 00:00:00 2001 From: Gaspar Dip Date: Tue, 1 Feb 2022 13:37:32 -0300 Subject: [PATCH 22/26] Add AddressToUintMap (#3150) * add AddressToUintMap * Update contracts/utils/structs/EnumerableMap.sol Co-authored-by: Francisco Giordano * address comments * lint code * merge mocks into a single file * add PR link to changelog entry Co-authored-by: Francisco Giordano Co-authored-by: Hadrien Croubois --- .prettierrc | 3 + CHANGELOG.md | 1 + contracts/mocks/EnumerableMapMock.sol | 42 ++++- contracts/utils/structs/EnumerableMap.sol | 86 +++++++++- .../EnumerableMap/AddressToUintMap.test.js | 157 ++++++++++++++++++ .../UintToAddressMap.test.js} | 36 +--- test/utils/structs/EnumerableMap/helpers.js | 31 ++++ 7 files changed, 323 insertions(+), 33 deletions(-) create mode 100644 test/utils/structs/EnumerableMap/AddressToUintMap.test.js rename test/utils/structs/{EnumerableMap.test.js => EnumerableMap/UintToAddressMap.test.js} (78%) create mode 100644 test/utils/structs/EnumerableMap/helpers.js diff --git a/.prettierrc b/.prettierrc index 5c11cc2237d..f91ad7ee614 100644 --- a/.prettierrc +++ b/.prettierrc @@ -1,8 +1,11 @@ { + "singleQuote": true, + "trailingComma": "all", "overrides": [ { "files": "*.sol", "options": { + "singleQuote": false, "printWidth": 120, "explicitTypes": "always" } diff --git a/CHANGELOG.md b/CHANGELOG.md index e878c2c43b3..0bb0981f52f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased * `AccessControl`: add a virtual `_checkRole(bytes32)` function that can be overriden 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)) ## Unreleased diff --git a/contracts/mocks/EnumerableMapMock.sol b/contracts/mocks/EnumerableMapMock.sol index 510647b5839..21c1fe362d9 100644 --- a/contracts/mocks/EnumerableMapMock.sol +++ b/contracts/mocks/EnumerableMapMock.sol @@ -4,7 +4,8 @@ pragma solidity ^0.8.0; import "../utils/structs/EnumerableMap.sol"; -contract EnumerableMapMock { +// UintToAddressMap +contract UintToAddressMapMock { using EnumerableMap for EnumerableMap.UintToAddressMap; event OperationResult(bool result); @@ -45,3 +46,42 @@ contract EnumerableMapMock { return _map.get(key, errorMessage); } } + +// AddressToUintMap +contract AddressToUintMapMock { + using EnumerableMap for EnumerableMap.AddressToUintMap; + + event OperationResult(bool result); + + EnumerableMap.AddressToUintMap private _map; + + function contains(address key) public view returns (bool) { + return _map.contains(key); + } + + function set(address key, uint256 value) public { + bool result = _map.set(key, value); + emit OperationResult(result); + } + + function remove(address key) public { + bool result = _map.remove(key); + emit OperationResult(result); + } + + function length() public view returns (uint256) { + return _map.length(); + } + + function at(uint256 index) public view returns (address key, uint256 value) { + return _map.at(index); + } + + function tryGet(address key) public view returns (bool, uint256) { + return _map.tryGet(key); + } + + function get(address key) public view returns (uint256) { + return _map.get(key); + } +} diff --git a/contracts/utils/structs/EnumerableMap.sol b/contracts/utils/structs/EnumerableMap.sol index 1bc571594cb..1c3f4357ecf 100644 --- a/contracts/utils/structs/EnumerableMap.sol +++ b/contracts/utils/structs/EnumerableMap.sol @@ -26,8 +26,10 @@ import "./EnumerableSet.sol"; * } * ``` * - * As of v3.0.0, only maps of type `uint256 -> address` (`UintToAddressMap`) are - * supported. + * The following map types are supported: + * + * - `uint256 -> address` (`UintToAddressMap`) since v3.0.0 + * - `address -> uint256` (`AddressToUintMap`) since v4.6.0 */ library EnumerableMap { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -237,4 +239,84 @@ library EnumerableMap { ) internal view returns (address) { return address(uint160(uint256(_get(map._inner, bytes32(key), errorMessage)))); } + + // AddressToUintMap + + struct AddressToUintMap { + Map _inner; + } + + /** + * @dev Adds a key-value pair to a map, or updates the value for an existing + * key. O(1). + * + * Returns true if the key was added to the map, that is if it was not + * already present. + */ + function set( + AddressToUintMap storage map, + address key, + uint256 value + ) internal returns (bool) { + return _set(map._inner, bytes32(uint256(uint160(key))), bytes32(value)); + } + + /** + * @dev Removes a value from a set. O(1). + * + * Returns true if the key was removed from the map, that is if it was present. + */ + function remove(AddressToUintMap storage map, address key) internal returns (bool) { + return _remove(map._inner, bytes32(uint256(uint160(key)))); + } + + /** + * @dev Returns true if the key is in the map. O(1). + */ + function contains(AddressToUintMap storage map, address key) internal view returns (bool) { + return _contains(map._inner, bytes32(uint256(uint160(key)))); + } + + /** + * @dev Returns the number of elements in the map. O(1). + */ + function length(AddressToUintMap storage map) internal view returns (uint256) { + return _length(map._inner); + } + + /** + * @dev Returns the element stored at position `index` in the set. O(1). + * Note that there are no guarantees on the ordering of values inside the + * array, and it may change when more values are added or removed. + * + * Requirements: + * + * - `index` must be strictly less than {length}. + */ + function at(AddressToUintMap storage map, uint256 index) internal view returns (address, uint256) { + (bytes32 key, bytes32 value) = _at(map._inner, index); + return (address(uint160(uint256(key))), uint256(value)); + } + + /** + * @dev Tries to returns the value associated with `key`. O(1). + * Does not revert if `key` is not in the map. + * + * _Available since v3.4._ + */ + function tryGet(AddressToUintMap storage map, address key) internal view returns (bool, uint256) { + (bool success, bytes32 value) = _tryGet(map._inner, bytes32(uint256(uint160(key)))); + return (success, uint256(value)); + } + + /** + * @dev Returns the value associated with `key`. O(1). + * + * Requirements: + * + * - `key` must be in the map. + */ + function get(AddressToUintMap storage map, address key) internal view returns (uint256) { + return uint256(_get(map._inner, bytes32(uint256(uint160(key))))); + } } diff --git a/test/utils/structs/EnumerableMap/AddressToUintMap.test.js b/test/utils/structs/EnumerableMap/AddressToUintMap.test.js new file mode 100644 index 00000000000..0a79479323e --- /dev/null +++ b/test/utils/structs/EnumerableMap/AddressToUintMap.test.js @@ -0,0 +1,157 @@ +const { BN, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); +const { expectMembersMatch } = require('./helpers'); + +const AddressToUintMapMock = artifacts.require('AddressToUintMapMock'); + +contract('AddressToUintMap', function (accounts) { + const [accountA, accountB, accountC] = accounts; + + const valueA = new BN('7891'); + const valueB = new BN('451'); + const valueC = new BN('9592328'); + + beforeEach(async function () { + this.map = await AddressToUintMapMock.new(); + }); + + it('starts empty', async function () { + expect(await this.map.contains(accountA)).to.equal(false); + + await expectMembersMatch(this.map, [], []); + }); + + describe('set', function () { + it('adds a key', async function () { + const receipt = await this.map.set(accountA, valueA); + + expectEvent(receipt, 'OperationResult', { result: true }); + + await expectMembersMatch(this.map, [accountA], [valueA]); + }); + + it('adds several keys', async function () { + await this.map.set(accountA, valueA); + await this.map.set(accountB, valueB); + + await expectMembersMatch(this.map, [accountA, accountB], [valueA, valueB]); + + expect(await this.map.contains(accountC)).to.equal(false); + }); + + it('returns false when adding keys already in the set', async function () { + await this.map.set(accountA, valueA); + + const receipt = await this.map.set(accountA, valueA); + + expectEvent(receipt, 'OperationResult', { result: false }); + + await expectMembersMatch(this.map, [accountA], [valueA]); + }); + + it('updates values for keys already in the set', async function () { + await this.map.set(accountA, valueA); + await this.map.set(accountA, valueB); + + await expectMembersMatch(this.map, [accountA], [valueB]); + }); + }); + + describe('remove', function () { + it('removes added keys', async function () { + await this.map.set(accountA, valueA); + + const receipt = await this.map.remove(accountA); + + expectEvent(receipt, 'OperationResult', { result: true }); + + expect(await this.map.contains(accountA)).to.equal(false); + + await expectMembersMatch(this.map, [], []); + }); + + it('returns false when removing keys not in the set', async function () { + const receipt = await this.map.remove(accountA); + + expectEvent(receipt, 'OperationResult', { result: false }); + + expect(await this.map.contains(accountA)).to.equal(false); + }); + + it('adds and removes multiple keys', async function () { + // [] + + await this.map.set(accountA, valueA); + await this.map.set(accountC, valueC); + + // [A, C] + + await this.map.remove(accountA); + await this.map.remove(accountB); + + // [C] + + await this.map.set(accountB, valueB); + + // [C, B] + + await this.map.set(accountA, valueA); + await this.map.remove(accountC); + + // [A, B] + + await this.map.set(accountA, valueA); + await this.map.set(accountB, valueB); + + // [A, B] + + await this.map.set(accountC, valueC); + await this.map.remove(accountA); + + // [B, C] + + await this.map.set(accountA, valueA); + await this.map.remove(accountB); + + // [A, C] + + await expectMembersMatch(this.map, [accountA, accountC], [valueA, valueC]); + + expect(await this.map.contains(accountB)).to.equal(false); + }); + }); + + describe('read', function () { + beforeEach(async function () { + await this.map.set(accountA, valueA); + }); + + describe('get', function () { + it('existing value', async function () { + expect(await this.map.get(accountA)).to.bignumber.equal(valueA); + }); + + it('missing value', async function () { + await expectRevert(this.map.get(accountB), 'EnumerableMap: nonexistent key'); + }); + }); + + describe('tryGet', function () { + const stringifyTryGetValue = ({ 0: result, 1: value }) => ({ 0: result, 1: value.toString() }); + + it('existing value', async function () { + const actual = stringifyTryGetValue(await this.map.tryGet(accountA)); + const expected = stringifyTryGetValue({ 0: true, 1: valueA }); + + expect(actual).to.deep.equal(expected); + }); + + it('missing value', async function () { + const actual = stringifyTryGetValue(await this.map.tryGet(accountB)); + const expected = stringifyTryGetValue({ 0: false, 1: new BN('0') }); + + expect(actual).to.deep.equal(expected); + }); + }); + }); +}); diff --git a/test/utils/structs/EnumerableMap.test.js b/test/utils/structs/EnumerableMap/UintToAddressMap.test.js similarity index 78% rename from test/utils/structs/EnumerableMap.test.js rename to test/utils/structs/EnumerableMap/UintToAddressMap.test.js index 9dc700b510b..560ed507701 100644 --- a/test/utils/structs/EnumerableMap.test.js +++ b/test/utils/structs/EnumerableMap/UintToAddressMap.test.js @@ -1,44 +1,20 @@ const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); +const { expectMembersMatch } = require('./helpers'); -const zip = require('lodash.zip'); +const UintToAddressMapMock = artifacts.require('UintToAddressMapMock'); -const EnumerableMapMock = artifacts.require('EnumerableMapMock'); - -contract('EnumerableMap', function (accounts) { - const [ accountA, accountB, accountC ] = accounts; +contract('UintToAddressMap', function (accounts) { + const [accountA, accountB, accountC] = accounts; const keyA = new BN('7891'); const keyB = new BN('451'); const keyC = new BN('9592328'); beforeEach(async function () { - this.map = await EnumerableMapMock.new(); + this.map = await UintToAddressMapMock.new(); }); - async function expectMembersMatch (map, keys, values) { - expect(keys.length).to.equal(values.length); - - await Promise.all(keys.map(async key => - expect(await map.contains(key)).to.equal(true), - )); - - expect(await map.length()).to.bignumber.equal(keys.length.toString()); - - expect(await Promise.all(keys.map(key => - map.get(key), - ))).to.have.same.members(values); - - // To compare key-value pairs, we zip keys and values, and convert BNs to - // strings to workaround Chai limitations when dealing with nested arrays - expect(await Promise.all([...Array(keys.length).keys()].map(async (index) => { - const entry = await map.at(index); - return [entry.key.toString(), entry.value]; - }))).to.have.same.deep.members( - zip(keys.map(k => k.toString()), values), - ); - } - it('starts empty', async function () { expect(await this.map.contains(keyA)).to.equal(false); @@ -64,7 +40,7 @@ contract('EnumerableMap', function (accounts) { it('returns false when adding keys already in the set', async function () { await this.map.set(keyA, accountA); - const receipt = (await this.map.set(keyA, accountA)); + const receipt = await this.map.set(keyA, accountA); expectEvent(receipt, 'OperationResult', { result: false }); await expectMembersMatch(this.map, [keyA], [accountA]); diff --git a/test/utils/structs/EnumerableMap/helpers.js b/test/utils/structs/EnumerableMap/helpers.js new file mode 100644 index 00000000000..456cdf156f3 --- /dev/null +++ b/test/utils/structs/EnumerableMap/helpers.js @@ -0,0 +1,31 @@ +const zip = require('lodash.zip'); + +const toStringArray = (array) => array.map((i) => i.toString()); + +async function expectMembersMatch (map, keys, values) { + const stringKeys = toStringArray(keys); + const stringValues = toStringArray(values); + + expect(keys.length).to.equal(values.length); + + await Promise.all(keys.map(async (key) => expect(await map.contains(key)).to.equal(true))); + + expect(await map.length()).to.bignumber.equal(keys.length.toString()); + + expect(toStringArray(await Promise.all(keys.map((key) => map.get(key))))).to.have.same.members(stringValues); + + // to compare key-value pairs, we zip keys and values + // we also stringify pairs because this helper is used for multiple types of maps + expect( + await Promise.all( + [...Array(keys.length).keys()].map(async (index) => { + const { key, value } = await map.at(index); + return [key.toString(), value.toString()]; + }), + ), + ).to.have.same.deep.members(zip(stringKeys, stringValues)); +} + +module.exports = { + expectMembersMatch, +}; From 574f3b89e1b94bdf932957a65d40fa252508e9af Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 1 Feb 2022 23:10:11 +0100 Subject: [PATCH 23/26] Add proper revert message on overflow of totalSupply during burn (#3144) --- contracts/token/ERC1155/extensions/ERC1155Supply.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/token/ERC1155/extensions/ERC1155Supply.sol b/contracts/token/ERC1155/extensions/ERC1155Supply.sol index 822c3bb0efd..5caa52726bf 100644 --- a/contracts/token/ERC1155/extensions/ERC1155Supply.sol +++ b/contracts/token/ERC1155/extensions/ERC1155Supply.sol @@ -51,7 +51,13 @@ abstract contract ERC1155Supply is ERC1155 { if (to == address(0)) { for (uint256 i = 0; i < ids.length; ++i) { - _totalSupply[ids[i]] -= amounts[i]; + uint256 id = ids[i]; + uint256 amount = amounts[i]; + uint256 supply = _totalSupply[id]; + require(supply >= amount, "ERC1155: burn amount exceeds totalSupply"); + unchecked { + _totalSupply[id] = supply - amount; + } } } } From bfd05d964604849b4282bb6a8337c8114da30826 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 1 Feb 2022 20:04:02 -0300 Subject: [PATCH 24/26] Add "available since" on Base64.sol --- contracts/utils/Base64.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/utils/Base64.sol b/contracts/utils/Base64.sol index f5e0b17220c..b6df0460f4f 100644 --- a/contracts/utils/Base64.sol +++ b/contracts/utils/Base64.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.0; /** * @dev Provides a set of functions to operate with Base64 strings. + * + * _Available since v4.5._ */ library Base64 { /** From 85566faeb279c9252d1fd885b9adbec038a7d5b3 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 2 Feb 2022 13:36:45 -0300 Subject: [PATCH 25/26] Improve documentation of various governance aspects (#3161) Co-authored-by: Hadrien Croubois --- contracts/governance/Governor.sol | 5 +-- .../extensions/GovernorTimelockCompound.sol | 6 ++-- .../extensions/GovernorTimelockControl.sol | 11 +++--- .../GovernorVotesQuorumFraction.sol | 35 +++++++++++++++++++ docs/modules/ROOT/pages/governance.adoc | 6 ++-- 5 files changed, 53 insertions(+), 10 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 19a930ddb5d..d195d8c4a9e 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -41,8 +41,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { mapping(uint256 => ProposalCore) private _proposals; /** - * @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. + * @dev Restrict access of functions to the governance executor, which may be the Governor itself or a timelock + * contract, as specified by {_executor}. This generally means that function with this modifier must be voted on and + * executed through the governance protocol. */ modifier onlyGovernance() { require(_msgSender() == _executor(), "Governor: onlyGovernance"); diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index b8d5d9a37bc..9e138197541 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -224,14 +224,16 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { /** * @dev Public endpoint to update the underlying timelock instance. Restricted to the timelock itself, so updates - * must be proposed, scheduled and executed using the {Governor} workflow. + * must be proposed, scheduled, and executed through governance proposals. * - * For security reason, the timelock must be handed over to another admin before setting up a new one. The two + * For security reasons, the timelock must be handed over to another admin before setting up a new one. The two * operations (hand over the timelock) and do the update can be batched in a single proposal. * * Note that if the timelock admin has been handed over in a previous operation, we refuse updates made through the * timelock if admin of the timelock has already been accepted and the operation is executed outside the scope of * governance. + + * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals. */ function updateTimelock(ICompoundTimelock newTimelock) external virtual onlyGovernance { _updateTimelock(newTimelock); diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 21b481781a2..541b8ce0b46 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -16,9 +16,10 @@ import "../TimelockController.sol"; * the assets and permissions must be attached to the {TimelockController}. Any asset sent to the {Governor} will be * inaccessible. * - * WARNING: Setting up the TimelockController to have additional proposers besides the governor introduces the risk that - * approved governance proposals could be blocked by the other proposers, effectively executing a Denial of Service attack, - * and therefore blocking access to governance-controlled assets. + * WARNING: Setting up the TimelockController to have additional proposers besides the governor is very risky, as it + * grants them powers that they must be trusted or known not to use: 1) {onlyGovernance} functions like {relay} are + * available to them through the timelock, and 2) approved governance proposals can be blocked by them, effectively + * executing a Denial of Service attack. This risk will be mitigated in a future release. * * _Available since v4.3._ */ @@ -150,7 +151,9 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { /** * @dev Public endpoint to update the underlying timelock instance. Restricted to the timelock itself, so updates - * must be proposed, scheduled and executed using the {Governor} workflow. + * must be proposed, scheduled, and executed through governance proposals. + * + * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals. */ function updateTimelock(TimelockController newTimelock) external virtual onlyGovernance { _updateTimelock(newTimelock); diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index d9f0352f4c3..970a9c6a0de 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -16,26 +16,61 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator); + /** + * @dev Initialize quorum as a fraction of the token's total supply. + * + * The fraction is specified as `numerator / denominator`. By default the denominator is 100, so quorum is + * specified as a percent: a numerator of 10 corresponds to quorum being 10% of total supply. The denominator can be + * customized by overriding {quorumDenominator}. + */ constructor(uint256 quorumNumeratorValue) { _updateQuorumNumerator(quorumNumeratorValue); } + /** + * @dev Returns the current quorum numerator. See {quorumDenominator}. + */ function quorumNumerator() public view virtual returns (uint256) { return _quorumNumerator; } + /** + * @dev Returns the quorum denominator. Defaults to 100, but may be overridden. + */ function quorumDenominator() public view virtual returns (uint256) { return 100; } + /** + * @dev Returns the quorum for a block number, in terms of number of votes: `supply * numerator / denominator`. + */ function quorum(uint256 blockNumber) public view virtual override returns (uint256) { return (token.getPastTotalSupply(blockNumber) * quorumNumerator()) / quorumDenominator(); } + /** + * @dev Changes the quorum numerator. + * + * Emits a {QuorumNumeratorUpdated} event. + * + * Requirements: + * + * - Must be called through a governance proposal. + * - New numerator must be smaller or equal to the denominator. + */ function updateQuorumNumerator(uint256 newQuorumNumerator) external virtual onlyGovernance { _updateQuorumNumerator(newQuorumNumerator); } + /** + * @dev Changes the quorum numerator. + * + * Emits a {QuorumNumeratorUpdated} event. + * + * Requirements: + * + * - New numerator must be smaller or equal to the denominator. + */ function _updateQuorumNumerator(uint256 newQuorumNumerator) internal virtual { require( newQuorumNumerator <= quorumDenominator(), diff --git a/docs/modules/ROOT/pages/governance.adoc b/docs/modules/ROOT/pages/governance.adoc index 9f04ab5974e..b87f9169246 100644 --- a/docs/modules/ROOT/pages/governance.adoc +++ b/docs/modules/ROOT/pages/governance.adoc @@ -248,7 +248,7 @@ contract MyGovernor is Governor, GovernorCompatibilityBravo, GovernorVotes, Gove It is good practice to add a timelock to governance decisions. This allows users to exit the system if they disagree with a decision before it is executed. We will use OpenZeppelin’s TimelockController in combination with the GovernorTimelockControl module. -IMPORTANT: When using a timelock, it is the timelock that will execute proposals and thus the timelock that should hold any funds, ownership, and access control roles. Funds in the Governor contract are not currently retrievable when using a timelock! (As of version 4.3 there is a caveat when using the Compound Timelock: ETH in the timelock is not easily usable, so it is recommended to manage ERC20 funds only in this combination until a future version resolves the issue.) +IMPORTANT: When using a timelock, it is the timelock that will execute proposals and thus the timelock that should hold any funds, ownership, and access control roles. Before version 4.5 there was no way to recover funds in the Governor contract when using a timelock! Before version 4.3, when using the Compound Timelock, ETH in the timelock was not easily accessible. TimelockController uses an AccessControl setup that we need to understand in order to set up roles. @@ -294,7 +294,9 @@ This will create a new proposal, with a proposal id that is obtained by hashing === Cast a Vote -Once a proposal is active, stakeholders can cast their vote. This is done through a function in the Governor contract that users can invoke directly from a governance UI such as Tally. +Once a proposal is active, delegates can cast their vote. Note that it is delegates who carry voting power: if a token holder wants to participate, they can set a trusted representative as their delegate, or they can become a delegate themselves by self-delegating their voting power. + +Votes are cast by interacting with the Governor contract through the `castVote` family of functions. Voters would generally invoke this from a governance UI such as Tally. image::tally-vote.png[Voting in Tally] From 28986d2f2eeb173f2be9889898ff1b148e1433ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20Daubensch=C3=BCtz?= Date: Thu, 3 Feb 2022 15:45:06 +0100 Subject: [PATCH 26/26] Start tokenId at zero in docs (#3162) - Fixes #3123 --- docs/modules/ROOT/pages/erc721.adoc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/modules/ROOT/pages/erc721.adoc b/docs/modules/ROOT/pages/erc721.adoc index 25dde116208..5149c1746c1 100644 --- a/docs/modules/ROOT/pages/erc721.adoc +++ b/docs/modules/ROOT/pages/erc721.adoc @@ -30,12 +30,11 @@ contract GameItem is ERC721URIStorage { public returns (uint256) { - _tokenIds.increment(); - uint256 newItemId = _tokenIds.current(); _mint(player, newItemId); _setTokenURI(newItemId, tokenURI); + _tokenIds.increment(); return newItemId; } }