From 7ccea54dc15856d0e6c3b61b829b85d9e52195cd Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 6 Jul 2023 18:33:38 -0300 Subject: [PATCH 01/31] Add back IGovernor to docs (#4421) --- contracts/governance/README.adoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 29c3887c789..35f324b7ede 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -52,6 +52,8 @@ NOTE: Functions of the `Governor` contract do not include access control. If you === Core +{{IGovernor}} + {{Governor}} === Modules From 996168f1f114645b01a1c2e6909c9d98ec451203 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 7 Jul 2023 08:29:21 -0600 Subject: [PATCH 02/31] Remove slither hardcoded version (#4431) --- .github/workflows/checks.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 15cc88e9600..122d3956413 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -100,7 +100,6 @@ jobs: - uses: crytic/slither-action@v0.3.0 with: node-version: 18.15 - slither-version: 0.9.3 codespell: runs-on: ubuntu-latest From 0053ee040a7ff1dbc39691c9e67a69f564930a88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 7 Jul 2023 14:01:35 -0600 Subject: [PATCH 03/31] Move `ECDSA` message hash methods to its own `MessageHashUtils` library (#4430) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco --- .changeset/hot-dingos-kiss.md | 5 ++ contracts/utils/README.adoc | 2 + contracts/utils/cryptography/ECDSA.sol | 84 +++--------------- contracts/utils/cryptography/EIP712.sol | 13 +-- .../utils/cryptography/MessageHashUtils.sol | 87 +++++++++++++++++++ docs/modules/ROOT/pages/utilities.adoc | 5 +- scripts/upgradeable/upgradeable.patch | 14 +-- test/utils/cryptography/ECDSA.test.js | 25 +----- .../cryptography/MessageHashUtils.test.js | 55 ++++++++++++ 9 files changed, 179 insertions(+), 111 deletions(-) create mode 100644 .changeset/hot-dingos-kiss.md create mode 100644 contracts/utils/cryptography/MessageHashUtils.sol create mode 100644 test/utils/cryptography/MessageHashUtils.test.js diff --git a/.changeset/hot-dingos-kiss.md b/.changeset/hot-dingos-kiss.md new file mode 100644 index 00000000000..fb213cd6454 --- /dev/null +++ b/.changeset/hot-dingos-kiss.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`MessageHashUtils`: Add a new library for creating message digest to be used along with signing or recovery such as ECDSA or ERC-1271. These functions are moved from the `ECDSA` library. diff --git a/contracts/utils/README.adoc b/contracts/utils/README.adoc index aa5182bd1af..d95f4dad4f0 100644 --- a/contracts/utils/README.adoc +++ b/contracts/utils/README.adoc @@ -34,6 +34,8 @@ Finally, {Create2} contains all necessary utilities to safely use the https://bl {{ECDSA}} +{{MessageHashUtils}} + {{SignatureChecker}} {{MerkleProof}} diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index aabfb5ca281..74064c88939 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -3,8 +3,6 @@ pragma solidity ^0.8.19; -import {Strings} from "../Strings.sol"; - /** * @dev Elliptic Curve Digital Signature Algorithm (ECDSA) operations. * @@ -34,18 +32,6 @@ library ECDSA { */ error ECDSAInvalidSignatureS(bytes32 s); - function _throwError(RecoverError error, bytes32 errorArg) private pure { - if (error == RecoverError.NoError) { - return; // no error: do nothing - } else if (error == RecoverError.InvalidSignature) { - revert ECDSAInvalidSignature(); - } else if (error == RecoverError.InvalidSignatureLength) { - revert ECDSAInvalidSignatureLength(uint256(errorArg)); - } else if (error == RecoverError.InvalidSignatureS) { - revert ECDSAInvalidSignatureS(errorArg); - } - } - /** * @dev Returns the address that signed a hashed message (`hash`) with * `signature` or error string. This address can then be used for verification purposes. @@ -58,7 +44,7 @@ library ECDSA { * verification to be secure: it is possible to craft signatures that * recover to arbitrary addresses for non-hashed data. A safe way to ensure * this is by receiving a hash of the original message (which may otherwise - * be too long), and then calling {toEthSignedMessageHash} on it. + * be too long), and then calling {MessageHashUtils-toEthSignedMessageHash} on it. * * Documentation for signature generation: * - with https://web3js.readthedocs.io/en/v1.3.4/web3-eth-accounts.html#sign[Web3.js] @@ -95,7 +81,7 @@ library ECDSA { * verification to be secure: it is possible to craft signatures that * recover to arbitrary addresses for non-hashed data. A safe way to ensure * this is by receiving a hash of the original message (which may otherwise - * be too long), and then calling {toEthSignedMessageHash} on it. + * be too long), and then calling {MessageHashUtils-toEthSignedMessageHash} on it. */ function recover(bytes32 hash, bytes memory signature) internal pure returns (address) { (address recovered, RecoverError error, bytes32 errorArg) = tryRecover(hash, signature); @@ -169,63 +155,17 @@ library ECDSA { } /** - * @dev Returns an Ethereum Signed Message, created from a `hash`. This - * produces hash corresponding to the one signed with the - * https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] - * JSON-RPC method as part of EIP-191. - * - * See {recover}. - */ - function toEthSignedMessageHash(bytes32 hash) internal pure returns (bytes32 message) { - // 32 is the length in bytes of hash, - // enforced by the type signature above - /// @solidity memory-safe-assembly - assembly { - mstore(0x00, "\x19Ethereum Signed Message:\n32") - mstore(0x1c, hash) - message := keccak256(0x00, 0x3c) - } - } - - /** - * @dev Returns an Ethereum Signed Message, created from `s`. This - * produces hash corresponding to the one signed with the - * https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] - * JSON-RPC method as part of EIP-191. - * - * See {recover}. + * @dev Optionally reverts with the corresponding custom error according to the `error` argument provided. */ - function toEthSignedMessageHash(bytes memory s) internal pure returns (bytes32) { - return keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", Strings.toString(s.length), s)); - } - - /** - * @dev Returns an Ethereum Signed Typed Data, created from a - * `domainSeparator` and a `structHash`. This produces hash corresponding - * to the one signed with the - * https://eips.ethereum.org/EIPS/eip-712[`eth_signTypedData`] - * JSON-RPC method as part of EIP-712. - * - * See {recover}. - */ - function toTypedDataHash(bytes32 domainSeparator, bytes32 structHash) internal pure returns (bytes32 data) { - /// @solidity memory-safe-assembly - assembly { - let ptr := mload(0x40) - mstore(ptr, hex"19_01") - mstore(add(ptr, 0x02), domainSeparator) - mstore(add(ptr, 0x22), structHash) - data := keccak256(ptr, 0x42) + function _throwError(RecoverError error, bytes32 errorArg) private pure { + if (error == RecoverError.NoError) { + return; // no error: do nothing + } else if (error == RecoverError.InvalidSignature) { + revert ECDSAInvalidSignature(); + } else if (error == RecoverError.InvalidSignatureLength) { + revert ECDSAInvalidSignatureLength(uint256(errorArg)); + } else if (error == RecoverError.InvalidSignatureS) { + revert ECDSAInvalidSignatureS(errorArg); } } - - /** - * @dev Returns an Ethereum Signed Data with intended validator, created from a - * `validator` and `data` according to the version 0 of EIP-191. - * - * See {recover}. - */ - function toDataWithIntendedValidatorHash(address validator, bytes memory data) internal pure returns (bytes32) { - return keccak256(abi.encodePacked(hex"19_00", validator, data)); - } } diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index ff34e814698..36f076e55d8 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -3,16 +3,17 @@ pragma solidity ^0.8.19; -import {ECDSA} from "./ECDSA.sol"; +import {MessageHashUtils} from "./MessageHashUtils.sol"; import {ShortStrings, ShortString} from "../ShortStrings.sol"; import {IERC5267} from "../../interfaces/IERC5267.sol"; /** * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. * - * The encoding specified in the EIP is very generic, and such a generic implementation in Solidity is not feasible, - * thus this contract does not implement the encoding itself. Protocols need to implement the type-specific encoding - * they need in their contracts using a combination of `abi.encode` and `keccak256`. + * The encoding scheme specified in the EIP requires a domain separator and a hash of the typed structured data, whose + * encoding is very generic and therefore its implementation in Solidity is not feasible, thus this contract + * does not implement the encoding itself. Protocols need to implement the type-specific encoding they need in order to + * produce the hash of their typed data using a combination of `abi.encode` and `keccak256`. * * This contract implements the EIP 712 domain separator ({_domainSeparatorV4}) that is used as part of the encoding * scheme, and the final step of the encoding to obtain the message digest that is then signed via ECDSA @@ -25,7 +26,7 @@ import {IERC5267} from "../../interfaces/IERC5267.sol"; * https://docs.metamask.io/guide/signing-data.html[`eth_signTypedDataV4` in MetaMask]. * * NOTE: In the upgradeable version of this contract, the cached values will correspond to the address, and the domain - * separator of the implementation contract. This will cause the `_domainSeparatorV4` function to always rebuild the + * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the * separator from the immutable values, which is cheaper than accessing a cached version in cold storage. * * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment @@ -104,7 +105,7 @@ abstract contract EIP712 is IERC5267 { * ``` */ function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) { - return ECDSA.toTypedDataHash(_domainSeparatorV4(), structHash); + return MessageHashUtils.toTypedDataHash(_domainSeparatorV4(), structHash); } /** diff --git a/contracts/utils/cryptography/MessageHashUtils.sol b/contracts/utils/cryptography/MessageHashUtils.sol new file mode 100644 index 00000000000..05ce35af810 --- /dev/null +++ b/contracts/utils/cryptography/MessageHashUtils.sol @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.19; + +import {Strings} from "../Strings.sol"; + +/** + * @dev Signature message hash utilities for producing digests to be consumed by {ECDSA} recovery or signing. + * + * The library provides methods for generating a hash of a message that conforms to the + * https://eips.ethereum.org/EIPS/eip-191[EIP 191] and https://eips.ethereum.org/EIPS/eip-712[EIP 712] + * specifications. + */ +library MessageHashUtils { + /** + * @dev Returns the keccak256 digest of an EIP-191 signed data with version + * `0x45` (`personal_sign` messages). + * + * The digest is calculated by prefixing a bytes32 `messageHash` with + * `"\x19Ethereum Signed Message:\n32"` and hashing the result. It corresponds with the + * hash signed when using the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] JSON-RPC method. + * + * NOTE: The `hash` parameter is intended to be the result of hashing a raw message with + * keccak256, althoguh any bytes32 value can be safely used because the final digest will + * be re-hashed. + * + * See {ECDSA-recover}. + */ + function toEthSignedMessageHash(bytes32 messageHash) internal pure returns (bytes32 digest) { + /// @solidity memory-safe-assembly + assembly { + mstore(0x00, "\x19Ethereum Signed Message:\n32") // 32 is the bytes-length of messageHash + mstore(0x1c, messageHash) // 0x1c (28) is the length of the prefix + digest := keccak256(0x00, 0x3c) // 0x3c is the length of the prefix (0x1c) + messageHash (0x20) + } + } + + /** + * @dev Returns the keccak256 digest of an EIP-191 signed data with version + * `0x45` (`personal_sign` messages). + * + * The digest is calculated by prefixing an arbitrary `message` with + * `"\x19Ethereum Signed Message:\n" + len(message)` and hashing the result. It corresponds with the + * hash signed when using the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] JSON-RPC method. + * + * See {ECDSA-recover}. + */ + function toEthSignedMessageHash(bytes memory message) internal pure returns (bytes32 digest) { + return keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", Strings.toString(message.length), message)); + } + + /** + * @dev Returns the keccak256 digest of an EIP-191 signed data with version + * `0x00` (data with intended validator). + * + * The digest is calculated by prefixing an arbitrary `data` with `"\x19\x00"` and the intended + * `validator` address. Then hashing the result. + * + * See {ECDSA-recover}. + */ + function toDataWithIntendedValidatorHash( + address validator, + bytes memory data + ) internal pure returns (bytes32 digest) { + return keccak256(abi.encodePacked(hex"19_00", validator, data)); + } + + /** + * @dev Returns the keccak256 digest of an EIP-712 typed data (EIP-191 version `0x01`). + * + * The digest is calculated from a `domainSeparator` and a `structHash`, by prefixing them with + * `\x19\x01` and hashing the result. It corresponds to the hash signed by the + * https://eips.ethereum.org/EIPS/eip-712[`eth_signTypedData`] JSON-RPC method as part of EIP-712. + * + * See {ECDSA-recover}. + */ + function toTypedDataHash(bytes32 domainSeparator, bytes32 structHash) internal pure returns (bytes32 digest) { + /// @solidity memory-safe-assembly + assembly { + let ptr := mload(0x40) + mstore(ptr, hex"19_01") + mstore(add(ptr, 0x02), domainSeparator) + mstore(add(ptr, 0x22), structHash) + digest := keccak256(ptr, 0x42) + } + } +} diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index 3a1ba5420d6..2ac7b63c9f3 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -9,11 +9,12 @@ The OpenZeppelin Contracts provide a ton of useful utilities that you can use in xref:api:utils.adoc#ECDSA[`ECDSA`] provides functions for recovering and managing Ethereum account ECDSA signatures. These are often generated via https://web3js.readthedocs.io/en/v1.7.3/web3-eth.html#sign[`web3.eth.sign`], and are a 65 byte array (of type `bytes` in Solidity) arranged the following way: `[[v (1)], [r (32)], [s (32)]]`. -The data signer can be recovered with xref:api:utils.adoc#ECDSA-recover-bytes32-bytes-[`ECDSA.recover`], and its address compared to verify the signature. Most wallets will hash the data to sign and add the prefix '\x19Ethereum Signed Message:\n', so when attempting to recover the signer of an Ethereum signed message hash, you'll want to use xref:api:utils.adoc#ECDSA-toEthSignedMessageHash-bytes32-[`toEthSignedMessageHash`]. +The data signer can be recovered with xref:api:utils.adoc#ECDSA-recover-bytes32-bytes-[`ECDSA.recover`], and its address compared to verify the signature. Most wallets will hash the data to sign and add the prefix '\x19Ethereum Signed Message:\n', so when attempting to recover the signer of an Ethereum signed message hash, you'll want to use xref:api:utils.adoc#MessageHashUtils-toEthSignedMessageHash-bytes32-[`toEthSignedMessageHash`]. [source,solidity] ---- using ECDSA for bytes32; +using MessageHashUtils for bytes32; function _verify(bytes32 data, bytes memory signature, address account) internal pure returns (bool) { return data @@ -22,7 +23,7 @@ function _verify(bytes32 data, bytes memory signature, address account) internal } ---- -WARNING: Getting signature verification right is not trivial: make sure you fully read and understand xref:api:utils.adoc#ECDSA[`ECDSA`]'s documentation. +WARNING: Getting signature verification right is not trivial: make sure you fully read and understand xref:api:utils.adoc#MessageHashUtils[`MessageHashUtils`]'s and xref:api:utils.adoc#ECDSA[`ECDSA`]'s documentation. === Verifying Merkle Proofs diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch index ebf3aa4e2e7..71a76d78c3b 100644 --- a/scripts/upgradeable/upgradeable.patch +++ b/scripts/upgradeable/upgradeable.patch @@ -126,20 +126,20 @@ index df141192..1cf90ad1 100644 "keywords": [ "solidity", diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol -index ff34e814..a9d08d5c 100644 +index 36f076e5..90c1db78 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.19; - import {ECDSA} from "./ECDSA.sol"; + import {MessageHashUtils} from "./MessageHashUtils.sol"; -import {ShortStrings, ShortString} from "../ShortStrings.sol"; import {IERC5267} from "../../interfaces/IERC5267.sol"; /** -@@ -27,28 +26,18 @@ import {IERC5267} from "../../interfaces/IERC5267.sol"; +@@ -28,28 +27,18 @@ import {IERC5267} from "../../interfaces/IERC5267.sol"; * NOTE: In the upgradeable version of this contract, the cached values will correspond to the address, and the domain - * separator of the implementation contract. This will cause the `_domainSeparatorV4` function to always rebuild the + * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the * separator from the immutable values, which is cheaper than accessing a cached version in cold storage. - * - * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment @@ -170,7 +170,7 @@ index ff34e814..a9d08d5c 100644 /** * @dev Initializes the domain separator and parameter caches. -@@ -63,29 +52,23 @@ abstract contract EIP712 is IERC5267 { +@@ -64,29 +53,23 @@ abstract contract EIP712 is IERC5267 { * contract upgrade]. */ constructor(string memory name, string memory version) { @@ -208,7 +208,7 @@ index ff34e814..a9d08d5c 100644 } /** -@@ -124,6 +107,10 @@ abstract contract EIP712 is IERC5267 { +@@ -125,6 +108,10 @@ abstract contract EIP712 is IERC5267 { uint256[] memory extensions ) { @@ -219,7 +219,7 @@ index ff34e814..a9d08d5c 100644 return ( hex"0f", // 01111 _EIP712Name(), -@@ -138,22 +125,62 @@ abstract contract EIP712 is IERC5267 { +@@ -139,22 +126,62 @@ abstract contract EIP712 is IERC5267 { /** * @dev The name parameter for the EIP712 domain. * diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index 3fd112a1844..f164ef196c7 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -1,6 +1,6 @@ require('@openzeppelin/test-helpers'); const { expectRevertCustomError } = require('../../helpers/customError'); -const { toEthSignedMessageHash, toDataWithIntendedValidatorHash } = require('../../helpers/sign'); +const { toEthSignedMessageHash } = require('../../helpers/sign'); const { expect } = require('chai'); @@ -9,7 +9,6 @@ const ECDSA = artifacts.require('$ECDSA'); const TEST_MESSAGE = web3.utils.sha3('OpenZeppelin'); const WRONG_MESSAGE = web3.utils.sha3('Nope'); const NON_HASH_MESSAGE = '0x' + Buffer.from('abcd').toString('hex'); -const RANDOM_ADDRESS = web3.utils.toChecksumAddress(web3.utils.randomHex(20)); function to2098Format(signature) { const long = web3.utils.hexToBytes(signature); @@ -243,26 +242,4 @@ contract('ECDSA', function (accounts) { expect(() => to2098Format(highSSignature)).to.throw("invalid signature 's' value"); }); }); - - context('toEthSignedMessageHash', function () { - it('prefixes bytes32 data correctly', async function () { - expect(await this.ecdsa.methods['$toEthSignedMessageHash(bytes32)'](TEST_MESSAGE)).to.equal( - toEthSignedMessageHash(TEST_MESSAGE), - ); - }); - - it('prefixes dynamic length data correctly', async function () { - expect(await this.ecdsa.methods['$toEthSignedMessageHash(bytes)'](NON_HASH_MESSAGE)).to.equal( - toEthSignedMessageHash(NON_HASH_MESSAGE), - ); - }); - }); - - context('toDataWithIntendedValidatorHash', function () { - it('returns the hash correctly', async function () { - expect( - await this.ecdsa.methods['$toDataWithIntendedValidatorHash(address,bytes)'](RANDOM_ADDRESS, NON_HASH_MESSAGE), - ).to.equal(toDataWithIntendedValidatorHash(RANDOM_ADDRESS, NON_HASH_MESSAGE)); - }); - }); }); diff --git a/test/utils/cryptography/MessageHashUtils.test.js b/test/utils/cryptography/MessageHashUtils.test.js new file mode 100644 index 00000000000..b38e945daaa --- /dev/null +++ b/test/utils/cryptography/MessageHashUtils.test.js @@ -0,0 +1,55 @@ +require('@openzeppelin/test-helpers'); +const { toEthSignedMessageHash, toDataWithIntendedValidatorHash } = require('../../helpers/sign'); +const { domainSeparator, hashTypedData } = require('../../helpers/eip712'); + +const { expect } = require('chai'); + +const MessageHashUtils = artifacts.require('$MessageHashUtils'); + +contract('MessageHashUtils', function () { + beforeEach(async function () { + this.messageHashUtils = await MessageHashUtils.new(); + + this.message = '0x' + Buffer.from('abcd').toString('hex'); + this.messageHash = web3.utils.sha3(this.message); + this.verifyingAddress = web3.utils.toChecksumAddress(web3.utils.randomHex(20)); + }); + + context('toEthSignedMessageHash', function () { + it('prefixes bytes32 data correctly', async function () { + expect(await this.messageHashUtils.methods['$toEthSignedMessageHash(bytes32)'](this.messageHash)).to.equal( + toEthSignedMessageHash(this.messageHash), + ); + }); + + it('prefixes dynamic length data correctly', async function () { + expect(await this.messageHashUtils.methods['$toEthSignedMessageHash(bytes)'](this.message)).to.equal( + toEthSignedMessageHash(this.message), + ); + }); + }); + + context('toDataWithIntendedValidatorHash', function () { + it('returns the digest correctly', async function () { + expect( + await this.messageHashUtils.$toDataWithIntendedValidatorHash(this.verifyingAddress, this.message), + ).to.equal(toDataWithIntendedValidatorHash(this.verifyingAddress, this.message)); + }); + }); + + context('toTypedDataHash', function () { + it('returns the digest correctly', async function () { + const domain = { + name: 'Test', + version: 1, + chainId: 1, + verifyingContract: this.verifyingAddress, + }; + const structhash = web3.utils.randomHex(32); + const expectedDomainSeparator = await domainSeparator(domain); + expect(await this.messageHashUtils.$toTypedDataHash(expectedDomainSeparator, structhash)).to.equal( + hashTypedData(domain, structhash), + ); + }); + }); +}); From f5bf7233cb32b28d29a0ff5f1911d4c5118836a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 7 Jul 2023 18:56:49 -0600 Subject: [PATCH 04/31] Add `ERC2771Forwarder` fuzz tests for avoiding loss of unused ETH (#4396) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco --- contracts/metatx/ERC2771Forwarder.sol | 12 +- test/metatx/ERC2771Forwarder.t.sol | 165 ++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 test/metatx/ERC2771Forwarder.t.sol diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index f271d5d3b4d..84d736c7416 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -53,7 +53,7 @@ contract ERC2771Forwarder is EIP712, Nonces { bytes signature; } - bytes32 private constant _FORWARD_REQUEST_TYPEHASH = + bytes32 internal constant _FORWARD_REQUEST_TYPEHASH = keccak256( "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" ); @@ -255,7 +255,7 @@ contract ERC2771Forwarder is EIP712, Nonces { abi.encodePacked(request.data, request.from) ); - _checkForwardedGas(request); + _checkForwardedGas(gasleft(), request); emit ExecutedForwardRequest(signer, currentNonce, success); } @@ -270,10 +270,10 @@ contract ERC2771Forwarder is EIP712, Nonces { * * It reverts consuming all the available gas if the forwarded gas is not the requested gas. * - * IMPORTANT: This function should be called exactly the end of the forwarded call. Any gas consumed - * in between will make room for bypassing this check. + * IMPORTANT: The `gasLeft` parameter should be measured exactly at the end of the forwarded call. + * Any gas consumed in between will make room for bypassing this check. */ - function _checkForwardedGas(ForwardRequestData calldata request) private view { + function _checkForwardedGas(uint256 gasLeft, ForwardRequestData calldata request) private pure { // To avoid insufficient gas griefing attacks, as referenced in https://ronan.eth.limo/blog/ethereum-gas-dangers/ // // A malicious relayer can attempt to shrink the gas forwarded so that the underlying call reverts out-of-gas @@ -295,7 +295,7 @@ contract ERC2771Forwarder is EIP712, Nonces { // - req.gas >= X * 63 / 64 // In other words if req.gas < X * 63 / 64 then req.gas / 63 <= gasleft(), thus if the relayer behaves honestly // the forwarding does not revert. - if (gasleft() < request.gas / 63) { + if (gasLeft < request.gas / 63) { // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since // neither revert or assert consume all gas since Solidity 0.8.0 // https://docs.soliditylang.org/en/v0.8.0/control-structures.html#panic-via-assert-and-error-via-require diff --git a/test/metatx/ERC2771Forwarder.t.sol b/test/metatx/ERC2771Forwarder.t.sol new file mode 100644 index 00000000000..946599cc9cd --- /dev/null +++ b/test/metatx/ERC2771Forwarder.t.sol @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.19; + +import {Test} from "forge-std/Test.sol"; +import {ERC2771Forwarder} from "contracts/metatx/ERC2771Forwarder.sol"; +import {CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol"; + +struct ForwardRequest { + address from; + address to; + uint256 value; + uint256 gas; + uint256 nonce; + uint48 deadline; + bytes data; +} + +contract ERC2771ForwarderMock is ERC2771Forwarder { + constructor(string memory name) ERC2771Forwarder(name) {} + + function structHash(ForwardRequest calldata request) external view returns (bytes32) { + return + _hashTypedDataV4( + keccak256( + abi.encode( + _FORWARD_REQUEST_TYPEHASH, + request.from, + request.to, + request.value, + request.gas, + request.nonce, + request.deadline, + keccak256(request.data) + ) + ) + ); + } +} + +contract ERC2771ForwarderTest is Test { + ERC2771ForwarderMock internal _erc2771Forwarder; + CallReceiverMock internal _receiver; + + uint256 internal _signerPrivateKey; + uint256 internal _relayerPrivateKey; + + address internal _signer; + address internal _relayer; + + uint256 internal constant _MAX_ETHER = 10_000_000; // To avoid overflow + + function setUp() public { + _erc2771Forwarder = new ERC2771ForwarderMock("ERC2771Forwarder"); + _receiver = new CallReceiverMock(); + + _signerPrivateKey = 0xA11CE; + _relayerPrivateKey = 0xB0B; + + _signer = vm.addr(_signerPrivateKey); + _relayer = vm.addr(_relayerPrivateKey); + } + + function _forgeRequestData( + uint256 value, + uint256 nonce, + uint48 deadline, + bytes memory data + ) private view returns (ERC2771Forwarder.ForwardRequestData memory) { + ForwardRequest memory request = ForwardRequest({ + from: _signer, + to: address(_receiver), + value: value, + gas: 30000, + nonce: nonce, + deadline: deadline, + data: data + }); + + bytes32 digest = _erc2771Forwarder.structHash(request); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(_signerPrivateKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + return + ERC2771Forwarder.ForwardRequestData({ + from: request.from, + to: request.to, + value: request.value, + gas: request.gas, + deadline: request.deadline, + data: request.data, + signature: signature + }); + } + + function testExecuteAvoidsETHStuck(uint256 initialBalance, uint256 value, bool targetReverts) public { + initialBalance = bound(initialBalance, 0, _MAX_ETHER); + value = bound(value, 0, _MAX_ETHER); + + vm.deal(address(_erc2771Forwarder), initialBalance); + + uint256 nonce = _erc2771Forwarder.nonces(_signer); + + vm.deal(address(this), value); + + ERC2771Forwarder.ForwardRequestData memory requestData = _forgeRequestData({ + value: value, + nonce: nonce, + deadline: uint48(block.timestamp + 1), + data: targetReverts + ? abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ()) + : abi.encodeCall(CallReceiverMock.mockFunction, ()) + }); + + if (targetReverts) { + vm.expectRevert(); + } + + _erc2771Forwarder.execute{value: value}(requestData); + assertEq(address(_erc2771Forwarder).balance, initialBalance); + } + + function testExecuteBatchAvoidsETHStuck(uint256 initialBalance, uint256 batchSize, uint256 value) public { + batchSize = bound(batchSize, 1, 10); + initialBalance = bound(initialBalance, 0, _MAX_ETHER); + value = bound(value, 0, _MAX_ETHER); + + vm.deal(address(_erc2771Forwarder), initialBalance); + uint256 nonce = _erc2771Forwarder.nonces(_signer); + + ERC2771Forwarder.ForwardRequestData[] memory batchRequestDatas = new ERC2771Forwarder.ForwardRequestData[]( + batchSize + ); + + uint256 expectedRefund; + + for (uint256 i = 0; i < batchSize; ++i) { + bytes memory data; + bool succeed = uint256(keccak256(abi.encodePacked(initialBalance, i))) % 2 == 0; + + if (succeed) { + data = abi.encodeCall(CallReceiverMock.mockFunction, ()); + } else { + expectedRefund += value; + data = abi.encodeCall(CallReceiverMock.mockFunctionRevertsNoReason, ()); + } + + batchRequestDatas[i] = _forgeRequestData({ + value: value, + nonce: nonce + i, + deadline: uint48(block.timestamp + 1), + data: data + }); + } + + address payable refundReceiver = payable(address(0xebe)); + uint256 totalValue = value * batchSize; + + vm.deal(address(this), totalValue); + _erc2771Forwarder.executeBatch{value: totalValue}(batchRequestDatas, refundReceiver); + + assertEq(address(_erc2771Forwarder).balance, initialBalance); + assertEq(refundReceiver.balance, expectedRefund); + } +} From 6d74b913885d729b5c72209aa03c4b68a33c794c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 8 Jul 2023 03:23:28 +0200 Subject: [PATCH 05/31] Remove superfluous receive() function from Proxy.sol (#4434) Co-authored-by: Francisco Giordano --- .changeset/eight-peaches-guess.md | 5 +++++ contracts/proxy/Proxy.sol | 8 -------- 2 files changed, 5 insertions(+), 8 deletions(-) create mode 100644 .changeset/eight-peaches-guess.md diff --git a/.changeset/eight-peaches-guess.md b/.changeset/eight-peaches-guess.md new file mode 100644 index 00000000000..ba4e87c179a --- /dev/null +++ b/.changeset/eight-peaches-guess.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Proxy`: Removed redundant `receive` function. diff --git a/contracts/proxy/Proxy.sol b/contracts/proxy/Proxy.sol index c2875aeac66..8c8925b9b45 100644 --- a/contracts/proxy/Proxy.sol +++ b/contracts/proxy/Proxy.sol @@ -68,14 +68,6 @@ abstract contract Proxy { _fallback(); } - /** - * @dev Fallback function that delegates calls to the address returned by `_implementation()`. Will run if call data - * is empty. - */ - receive() external payable virtual { - _fallback(); - } - /** * @dev Hook that is called before falling back to the implementation. Can happen as part of a manual `_fallback` * call, or as part of the Solidity `fallback` or `receive` functions. From 5229b75785213541e93fb0a466a3d102a3bf5dbe Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Sat, 8 Jul 2023 18:24:12 -0400 Subject: [PATCH 06/31] Use immutable beacon address in BeaconProxy (#4435) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- .changeset/proud-seals-complain.md | 5 +++++ contracts/proxy/ERC1967/ERC1967Utils.sol | 7 +++++-- contracts/proxy/beacon/BeaconProxy.sol | 16 +++++++++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 .changeset/proud-seals-complain.md diff --git a/.changeset/proud-seals-complain.md b/.changeset/proud-seals-complain.md new file mode 100644 index 00000000000..35df4777e27 --- /dev/null +++ b/.changeset/proud-seals-complain.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`BeaconProxy`: Use an immutable variable to store the address of the beacon. It is no longer possible for a `BeaconProxy` to upgrade by changing to another beacon. diff --git a/contracts/proxy/ERC1967/ERC1967Utils.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol index f7caaa68486..3ad93ff3c01 100644 --- a/contracts/proxy/ERC1967/ERC1967Utils.sol +++ b/contracts/proxy/ERC1967/ERC1967Utils.sol @@ -161,10 +161,13 @@ library ERC1967Utils { } /** - * @dev Perform beacon upgrade with additional setup call. Note: This upgrades the address of the beacon, it does - * not upgrade the implementation contained in the beacon (see {UpgradeableBeacon-_setImplementation} for that). + * @dev Change the beacon and trigger a setup call. * * Emits an {IERC1967-BeaconUpgraded} event. + * + * CAUTION: Invoking this function has no effect on an instance of {BeaconProxy} since v5, since + * it uses an immutable beacon without looking at the value of the ERC-1967 beacon slot for + * efficiency. */ function upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal { _setBeacon(newBeacon); diff --git a/contracts/proxy/beacon/BeaconProxy.sol b/contracts/proxy/beacon/BeaconProxy.sol index 94e697ca4d9..93e30688465 100644 --- a/contracts/proxy/beacon/BeaconProxy.sol +++ b/contracts/proxy/beacon/BeaconProxy.sol @@ -12,8 +12,14 @@ import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; * * The beacon address is stored in storage slot `uint256(keccak256('eip1967.proxy.beacon')) - 1`, so that it doesn't * conflict with the storage layout of the implementation behind the proxy. + * + * CAUTION: The beacon address can only be set once during construction, and cannot be changed afterwards. + * You must ensure that you either control the beacon, or trust the beacon to not upgrade the implementation maliciously. */ contract BeaconProxy is Proxy { + // An immutable address for the beacon to avoid unnecessary SLOADs before each delegate call. + address private immutable _beacon; + /** * @dev Initializes the proxy with `beacon`. * @@ -27,12 +33,20 @@ contract BeaconProxy is Proxy { */ constructor(address beacon, bytes memory data) payable { ERC1967Utils.upgradeBeaconToAndCall(beacon, data, false); + _beacon = beacon; } /** * @dev Returns the current implementation address of the associated beacon. */ function _implementation() internal view virtual override returns (address) { - return IBeacon(ERC1967Utils.getBeacon()).implementation(); + return IBeacon(_getBeacon()).implementation(); + } + + /** + * @dev Returns the beacon. + */ + function _getBeacon() internal view virtual returns (address) { + return _beacon; } } From e47b53bce4991a3eb9ecdca59a82b9956a3f0699 Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Sun, 9 Jul 2023 11:33:23 -0400 Subject: [PATCH 07/31] Improve BeaconProxy documentation for storage slot (#4438) --- contracts/proxy/beacon/BeaconProxy.sol | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/proxy/beacon/BeaconProxy.sol b/contracts/proxy/beacon/BeaconProxy.sol index 93e30688465..14a8d1b661c 100644 --- a/contracts/proxy/beacon/BeaconProxy.sol +++ b/contracts/proxy/beacon/BeaconProxy.sol @@ -10,11 +10,15 @@ import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; /** * @dev This contract implements a proxy that gets the implementation address for each call from an {UpgradeableBeacon}. * - * The beacon address is stored in storage slot `uint256(keccak256('eip1967.proxy.beacon')) - 1`, so that it doesn't - * conflict with the storage layout of the implementation behind the proxy. + * The beacon address can only be set once during construction, and cannot be changed afterwards. It is stored in an immutable + * variable to avoid unnecessary storage reads, and also in the beacon storage slot specified by + * https://eips.ethereum.org/EIPS/eip-1967[EIP1967] so that it can be accessed externally. * - * CAUTION: The beacon address can only be set once during construction, and cannot be changed afterwards. - * You must ensure that you either control the beacon, or trust the beacon to not upgrade the implementation maliciously. + * CAUTION: Since the beacon address can never be changed, you must ensure that you either control the beacon, or trust the + * beacon to not upgrade the implementation maliciously. + * + * IMPORTANT: Do not use the implementation logic to modify the beacon storage slot. Doing so would leave the proxy in an + * inconsistent state where the beacon storage slot does not match the beacon address. */ contract BeaconProxy is Proxy { // An immutable address for the beacon to avoid unnecessary SLOADs before each delegate call. From 4bac6fa310f85a439b8e269e17265e7f5cf85540 Mon Sep 17 00:00:00 2001 From: Francisco Date: Sun, 9 Jul 2023 18:36:23 -0300 Subject: [PATCH 08/31] Improve custom error helper when there is no match (#4437) --- test/helpers/customError.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/helpers/customError.js b/test/helpers/customError.js index e38170b78af..ea5c36820c5 100644 --- a/test/helpers/customError.js +++ b/test/helpers/customError.js @@ -13,13 +13,10 @@ async function expectRevertCustomError(promise, expectedErrorName, args) { // VM Exception while processing transaction: // reverted with custom error 'InvalidAccountNonce("0x70997970C51812dc3A010C7d01b50e0d17dc79C8", 0)' - // We trim out anything inside the single quotes as comma-separated values - const [, error] = message.match(/'(.*)'/); - - // Attempt to parse as an error - const match = error.match(/(?\w+)\((?.*)\)/); + // Attempt to parse as a custom error + const match = message.match(/custom error '(?\w+)\((?.*)\)'/); if (!match) { - expect.fail(`Couldn't parse "${error}" as a custom error`); + expect.fail(`Could not parse as custom error. ${message}`); } // Extract the error name and parameters const errorName = match.groups.name; From 2a4396c9dd281222e38fa4b1136d291ff77a0b16 Mon Sep 17 00:00:00 2001 From: Prince Allwin <127643894+worksofallwin@users.noreply.github.com> Date: Mon, 10 Jul 2023 03:12:23 +0530 Subject: [PATCH 09/31] Add suggested remappings in readme (#4440) Co-authored-by: Francisco --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 27627f43953..1befa024a56 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,8 @@ OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/con $ forge install OpenZeppelin/openzeppelin-contracts ``` +Add `@openzeppelin/=lib/openzeppelin-contracts/` in `remappings.txt.` + ### Usage Once installed, you can use the contracts in the library by importing them: From cd981f6521e22f2b74ee9d1cea27c6b16d318528 Mon Sep 17 00:00:00 2001 From: Luiz Hemerly Date: Mon, 10 Jul 2023 17:26:02 -0300 Subject: [PATCH 10/31] Add custom linting rules (#4132) Co-authored-by: Francisco Giordano Co-authored-by: Hadrien Croubois --- .solhint.json | 15 ------ package-lock.json | 18 +++++++ package.json | 1 + scripts/solhint-custom/index.js | 84 +++++++++++++++++++++++++++++ scripts/solhint-custom/package.json | 4 ++ solhint.config.js | 20 +++++++ 6 files changed, 127 insertions(+), 15 deletions(-) delete mode 100644 .solhint.json create mode 100644 scripts/solhint-custom/index.js create mode 100644 scripts/solhint-custom/package.json create mode 100644 solhint.config.js diff --git a/.solhint.json b/.solhint.json deleted file mode 100644 index cb8a8af6d78..00000000000 --- a/.solhint.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "rules": { - "no-unused-vars": "error", - "const-name-snakecase": "error", - "contract-name-camelcase": "error", - "event-name-camelcase": "error", - "func-name-mixedcase": "error", - "func-param-name-mixedcase": "error", - "modifier-name-mixedcase": "error", - "private-vars-leading-underscore": "error", - "var-name-mixedcase": "error", - "imports-on-top": "error", - "no-global-import": "error" - } -} diff --git a/package-lock.json b/package-lock.json index 3f9a4f5dab7..d02ae3baf4f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -43,6 +43,7 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "solhint": "^3.3.6", + "solhint-plugin-openzeppelin": "file:scripts/solhint-custom", "solidity-ast": "^0.4.25", "solidity-coverage": "^0.8.0", "solidity-docgen": "^0.6.0-beta.29", @@ -12436,6 +12437,10 @@ "prettier": "^2.8.3" } }, + "node_modules/solhint-plugin-openzeppelin": { + "resolved": "scripts/solhint-custom", + "link": true + }, "node_modules/solhint/node_modules/@solidity-parser/parser": { "version": "0.16.0", "resolved": "https://registry.npmjs.org/@solidity-parser/parser/-/parser-0.16.0.tgz", @@ -15405,6 +15410,16 @@ "funding": { "url": "https://github.com/sponsors/sindresorhus" } + }, + "scripts/lints": { + "version": "1.0.2", + "extraneous": true, + "license": "MIT" + }, + "scripts/solhint-custom": { + "version": "1.0.2", + "dev": true, + "license": "MIT" } }, "dependencies": { @@ -25126,6 +25141,9 @@ } } }, + "solhint-plugin-openzeppelin": { + "version": "file:scripts/solhint-custom" + }, "solidity-ast": { "version": "0.4.49", "resolved": "https://registry.npmjs.org/solidity-ast/-/solidity-ast-0.4.49.tgz", diff --git a/package.json b/package.json index 9eae67320c0..ffa868ac7bc 100644 --- a/package.json +++ b/package.json @@ -84,6 +84,7 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "solhint": "^3.3.6", + "solhint-plugin-openzeppelin": "file:scripts/solhint-custom", "solidity-ast": "^0.4.25", "solidity-coverage": "^0.8.0", "solidity-docgen": "^0.6.0-beta.29", diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js new file mode 100644 index 00000000000..a4cba1a93cf --- /dev/null +++ b/scripts/solhint-custom/index.js @@ -0,0 +1,84 @@ +const path = require('path'); +const minimatch = require('minimatch'); + +// Files matching these patterns will be ignored unless a rule has `static global = true` +const ignore = ['contracts/mocks/**/*', 'test/**/*']; + +class Base { + constructor(reporter, config, source, fileName) { + this.reporter = reporter; + this.ignored = this.constructor.global || ignore.some(p => minimatch(path.normalize(fileName), p)); + this.ruleId = this.constructor.ruleId; + if (this.ruleId === undefined) { + throw Error('missing ruleId static property'); + } + } + + error(node, message) { + if (!this.ignored) { + this.reporter.error(node, this.ruleId, message); + } + } +} + +module.exports = [ + class extends Base { + static ruleId = 'interface-names'; + + ContractDefinition(node) { + if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) { + this.error(node, 'Interface names should have a capital I prefix'); + } + } + }, + + class extends Base { + static ruleId = 'private-variables'; + + VariableDeclaration(node) { + const constantOrImmutable = node.isDeclaredConst || node.isImmutable; + if (node.isStateVar && !constantOrImmutable && node.visibility !== 'private') { + this.error(node, 'State variables must be private'); + } + } + }, + + class extends Base { + static ruleId = 'leading-underscore'; + + VariableDeclaration(node) { + if (node.isDeclaredConst) { + if (/^_/.test(node.name)) { + // TODO: re-enable and fix + // this.error(node, 'Constant variables should not have leading underscore'); + } + } else if (node.visibility === 'private' && !/^_/.test(node.name)) { + this.error(node, 'Non-constant private variables must have leading underscore'); + } + } + + FunctionDefinition(node) { + if (node.visibility === 'private' || (node.visibility === 'internal' && node.parent.kind !== 'library')) { + if (!/^_/.test(node.name)) { + this.error(node, 'Private and internal functions must have leading underscore'); + } + } + if (node.visibility === 'internal' && node.parent.kind === 'library') { + if (/^_/.test(node.name)) { + this.error(node, 'Library internal functions should not have leading underscore'); + } + } + } + }, + + // TODO: re-enable and fix + // class extends Base { + // static ruleId = 'no-external-virtual'; + // + // FunctionDefinition(node) { + // if (node.visibility == 'external' && node.isVirtual) { + // this.error(node, 'Functions should not be external and virtual'); + // } + // } + // }, +]; diff --git a/scripts/solhint-custom/package.json b/scripts/solhint-custom/package.json new file mode 100644 index 00000000000..d91e327a4cc --- /dev/null +++ b/scripts/solhint-custom/package.json @@ -0,0 +1,4 @@ +{ + "name": "solhint-plugin-openzeppelin", + "private": true +} diff --git a/solhint.config.js b/solhint.config.js new file mode 100644 index 00000000000..123ff91fa2d --- /dev/null +++ b/solhint.config.js @@ -0,0 +1,20 @@ +const customRules = require('./scripts/solhint-custom'); + +const rules = [ + 'no-unused-vars', + 'const-name-snakecase', + 'contract-name-camelcase', + 'event-name-camelcase', + 'func-name-mixedcase', + 'func-param-name-mixedcase', + 'modifier-name-mixedcase', + 'var-name-mixedcase', + 'imports-on-top', + 'no-global-import', + ...customRules.map(r => `openzeppelin/${r.ruleId}`), +]; + +module.exports = { + plugins: ['openzeppelin'], + rules: Object.fromEntries(rules.map(r => [r, 'error'])), +}; From 3d0edbecf1ce98d0f0960094c9ec7ee2b3947cc2 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 11 Jul 2023 14:49:58 -0300 Subject: [PATCH 11/31] Remove ERC1155Receiver in favor of ERC1155Holder (#4450) --- .changeset/afraid-walls-smell.md | 5 +++++ contracts/governance/TimelockController.sol | 3 +-- contracts/token/ERC1155/README.adoc | 2 -- .../token/ERC1155/utils/ERC1155Holder.sol | 14 ++++++++++--- .../token/ERC1155/utils/ERC1155Receiver.sol | 21 ------------------- 5 files changed, 17 insertions(+), 28 deletions(-) create mode 100644 .changeset/afraid-walls-smell.md delete mode 100644 contracts/token/ERC1155/utils/ERC1155Receiver.sol diff --git a/.changeset/afraid-walls-smell.md b/.changeset/afraid-walls-smell.md new file mode 100644 index 00000000000..682fdde5eaa --- /dev/null +++ b/.changeset/afraid-walls-smell.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC1155Receiver`: Removed in favor of `ERC1155Holder`. diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 2cf954d1215..be86f7b8827 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -6,7 +6,6 @@ pragma solidity ^0.8.19; import {AccessControl} from "../access/AccessControl.sol"; import {ERC721Holder} from "../token/ERC721/utils/ERC721Holder.sol"; import {ERC1155Holder} from "../token/ERC1155/utils/ERC1155Holder.sol"; -import {ERC1155Receiver} from "../token/ERC1155/utils/ERC1155Receiver.sol"; import {Address} from "../utils/Address.sol"; /** @@ -160,7 +159,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function supportsInterface( bytes4 interfaceId - ) public view virtual override(AccessControl, ERC1155Receiver) returns (bool) { + ) public view virtual override(AccessControl, ERC1155Holder) returns (bool) { return super.supportsInterface(interfaceId); } diff --git a/contracts/token/ERC1155/README.adoc b/contracts/token/ERC1155/README.adoc index 48c7faa0a6c..1a56358ef42 100644 --- a/contracts/token/ERC1155/README.adoc +++ b/contracts/token/ERC1155/README.adoc @@ -26,8 +26,6 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel {{IERC1155Receiver}} -{{ERC1155Receiver}} - == Extensions {{ERC1155Pausable}} diff --git a/contracts/token/ERC1155/utils/ERC1155Holder.sol b/contracts/token/ERC1155/utils/ERC1155Holder.sol index e288f86926b..908ad82c5d5 100644 --- a/contracts/token/ERC1155/utils/ERC1155Holder.sol +++ b/contracts/token/ERC1155/utils/ERC1155Holder.sol @@ -3,15 +3,23 @@ pragma solidity ^0.8.19; -import {ERC1155Receiver} from "./ERC1155Receiver.sol"; +import {IERC165, ERC165} from "../../../utils/introspection/ERC165.sol"; +import {IERC1155Receiver} from "../IERC1155Receiver.sol"; /** - * @dev Simple implementation of `ERC1155Receiver` that will allow a contract to hold ERC1155 tokens. + * @dev Simple implementation of `IERC1155Receiver` that will allow a contract to hold ERC1155 tokens. * * IMPORTANT: When inheriting this contract, you must include a way to use the received tokens, otherwise they will be * stuck. */ -abstract contract ERC1155Holder is ERC1155Receiver { +abstract contract ERC1155Holder is ERC165, IERC1155Receiver { + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) { + return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); + } + function onERC1155Received( address, address, diff --git a/contracts/token/ERC1155/utils/ERC1155Receiver.sol b/contracts/token/ERC1155/utils/ERC1155Receiver.sol deleted file mode 100644 index 02f922cf849..00000000000 --- a/contracts/token/ERC1155/utils/ERC1155Receiver.sol +++ /dev/null @@ -1,21 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.4.1 (token/ERC1155/utils/ERC1155Receiver.sol) - -pragma solidity ^0.8.19; - -import {IERC1155Receiver} from "../IERC1155Receiver.sol"; -import {IERC165, ERC165} from "../../../utils/introspection/ERC165.sol"; - -/** - * @dev Basic contract implementing the ERC-165 interface for {IERC1155Receiver}. - * - * NOTE: This contract does not suffice to receive tokens. See {ERC1155Holder}. - */ -abstract contract ERC1155Receiver is ERC165, IERC1155Receiver { - /** - * @dev See {IERC165-supportsInterface}. - */ - function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) { - return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); - } -} From 24ebff5ae947dca60101b1ba0cfe4a5e727b926d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 11 Jul 2023 11:51:40 -0600 Subject: [PATCH 12/31] Remove unused imports (#4436) Co-authored-by: Francisco --- contracts/access/AccessControl.sol | 1 - contracts/governance/extensions/GovernorTimelockCompound.sol | 1 - contracts/mocks/token/ERC20PermitNoRevertMock.sol | 1 - contracts/proxy/transparent/ProxyAdmin.sol | 2 +- contracts/proxy/utils/Initializable.sol | 2 -- contracts/token/ERC20/extensions/ERC20Votes.sol | 1 - contracts/token/ERC721/extensions/ERC721Royalty.sol | 1 - scripts/upgradeable/transpile.sh | 3 +++ test/token/ERC20/extensions/ERC4626.t.sol | 1 - 9 files changed, 4 insertions(+), 9 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index ec30e81e7a1..d934eddafe9 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.19; import {IAccessControl} from "./IAccessControl.sol"; import {Context} from "../utils/Context.sol"; -import {Strings} from "../utils/Strings.sol"; import {ERC165} from "../utils/introspection/ERC165.sol"; /** diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 4b609723d0d..c50c82c5784 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.19; import {IGovernorTimelock} from "./IGovernorTimelock.sol"; import {IGovernor, Governor} from "../Governor.sol"; -import {SafeCast} from "../../utils/math/SafeCast.sol"; import {ICompoundTimelock} from "../../vendor/compound/ICompoundTimelock.sol"; import {IERC165} from "../../interfaces/IERC165.sol"; import {Address} from "../../utils/Address.sol"; diff --git a/contracts/mocks/token/ERC20PermitNoRevertMock.sol b/contracts/mocks/token/ERC20PermitNoRevertMock.sol index 23395d75d46..63ae363fa4f 100644 --- a/contracts/mocks/token/ERC20PermitNoRevertMock.sol +++ b/contracts/mocks/token/ERC20PermitNoRevertMock.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.19; -import {ERC20} from "../../token/ERC20/ERC20.sol"; import {ERC20Permit} from "../../token/ERC20/extensions/ERC20Permit.sol"; abstract contract ERC20PermitNoRevertMock is ERC20Permit { diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 344c942f242..12d80a9c4ee 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; -import {ITransparentUpgradeableProxy, TransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol"; +import {ITransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol"; import {Ownable} from "../../access/Ownable.sol"; /** diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index a2e3569c48b..08171015d3c 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -3,8 +3,6 @@ pragma solidity ^0.8.19; -import {Address} from "../../utils/Address.sol"; - /** * @dev This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed * behind a proxy. Since proxied contracts do not make use of a constructor, it's common to move constructor logic to an diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index e0d675b2a7e..74953d3d6ca 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.19; import {ERC20} from "../ERC20.sol"; import {Votes} from "../../../governance/utils/Votes.sol"; -import {SafeCast} from "../../../utils/math/SafeCast.sol"; import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; /** diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index eb128ac5860..c4b8d371ea7 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.19; import {ERC721} from "../ERC721.sol"; import {ERC2981} from "../../common/ERC2981.sol"; -import {ERC165} from "../../../utils/introspection/ERC165.sol"; /** * @dev Extension of ERC721 with the ERC2981 NFT Royalty Standard, a standardized way to retrieve royalty payment diff --git a/scripts/upgradeable/transpile.sh b/scripts/upgradeable/transpile.sh index fbffe844e34..c0cb9ff5e56 100644 --- a/scripts/upgradeable/transpile.sh +++ b/scripts/upgradeable/transpile.sh @@ -33,3 +33,6 @@ npx @openzeppelin/upgrade-safe-transpiler@latest -D \ -x '!contracts/proxy/utils/UUPSUpgradeable.sol' \ -x '!contracts/proxy/beacon/IBeacon.sol' \ -p 'contracts/**/presets/**/*' + +# delete compilation artifacts of vanilla code +npm run clean diff --git a/test/token/ERC20/extensions/ERC4626.t.sol b/test/token/ERC20/extensions/ERC4626.t.sol index da01c7d8163..b5f94fcb148 100644 --- a/test/token/ERC20/extensions/ERC4626.t.sol +++ b/test/token/ERC20/extensions/ERC4626.t.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.19; import {ERC4626Test} from "erc4626-tests/ERC4626.test.sol"; -import {SafeCast} from "openzeppelin/utils/math/SafeCast.sol"; import {ERC20} from "openzeppelin/token/ERC20/ERC20.sol"; import {ERC4626} from "openzeppelin/token/ERC20/extensions/ERC4626.sol"; From 8b72e20e326078029b92d526ff5a44add2671df1 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 11 Jul 2023 16:35:56 -0300 Subject: [PATCH 13/31] Remove unnecessary explicit assignment override (#4443) --- contracts/proxy/utils/UUPSUpgradeable.sol | 2 +- contracts/utils/cryptography/EIP712.sol | 2 +- scripts/upgradeable/upgradeable.patch | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index f4045aa6f8b..286eafe8e96 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -17,7 +17,7 @@ import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; * The {_authorizeUpgrade} function must be overridden to include access restriction to the upgrade mechanism. */ abstract contract UUPSUpgradeable is IERC1822Proxiable { - /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment + /// @custom:oz-upgrades-unsafe-allow state-variable-immutable address private immutable __self = address(this); /** diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 36f076e55d8..3800804abdc 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -29,7 +29,7 @@ import {IERC5267} from "../../interfaces/IERC5267.sol"; * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the * separator from the immutable values, which is cheaper than accessing a cached version in cold storage. * - * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment + * @custom:oz-upgrades-unsafe-allow state-variable-immutable */ abstract contract EIP712 is IERC5267 { using ShortStrings for *; diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch index 71a76d78c3b..e50d3a70e8e 100644 --- a/scripts/upgradeable/upgradeable.patch +++ b/scripts/upgradeable/upgradeable.patch @@ -142,7 +142,7 @@ index 36f076e5..90c1db78 100644 * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the * separator from the immutable values, which is cheaper than accessing a cached version in cold storage. - * -- * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment +- * @custom:oz-upgrades-unsafe-allow state-variable-immutable */ abstract contract EIP712 is IERC5267 { - using ShortStrings for *; From 921ac49ccb898fb0bd7c57dc5c8156b96f5624a6 Mon Sep 17 00:00:00 2001 From: Pierre Grimaud Date: Wed, 12 Jul 2023 22:05:21 +0200 Subject: [PATCH 14/31] Fix typos in the tests (#4452) --- test/governance/extensions/GovernorWithParams.test.js | 2 +- test/helpers/governance.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 896c2e094b8..35da3a0f3e6 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -147,7 +147,7 @@ contract('GovernorWithParams', function (accounts) { ethSigUtil.signTypedMessage(privateKey, { data: await this.data(contract, message) }); }); - it('suports EOA signatures', async function () { + it('supports EOA signatures', async function () { await this.token.delegate(this.voterBySig.address, { from: voter2 }); const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 3ae0695ac0d..9b3349cf81c 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -81,7 +81,7 @@ class GovernorHelper { ...concatOpts(proposal.shortProposal, opts), ); default: - throw new Error(`unsuported visibility "${visibility}"`); + throw new Error(`unsupported visibility "${visibility}"`); } } From 0abf18f305e0066279231e930889cf8ae30a673f Mon Sep 17 00:00:00 2001 From: Molly Date: Wed, 12 Jul 2023 16:07:35 -0400 Subject: [PATCH 15/31] Remove duplicated SLOAD in Arrays.findUpperBound (#4442) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- .changeset/fluffy-countries-buy.md | 5 +++++ contracts/utils/Arrays.sol | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) create mode 100644 .changeset/fluffy-countries-buy.md diff --git a/.changeset/fluffy-countries-buy.md b/.changeset/fluffy-countries-buy.md new file mode 100644 index 00000000000..0cc7de37039 --- /dev/null +++ b/.changeset/fluffy-countries-buy.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Arrays`: Optimize `findUpperBound` by removing redundant SLOAD. diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 7dbc86fdd52..0c16b07cf57 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -22,13 +22,13 @@ library Arrays { * repeated elements. */ function findUpperBound(uint256[] storage array, uint256 element) internal view returns (uint256) { - if (array.length == 0) { - return 0; - } - uint256 low = 0; uint256 high = array.length; + if (high == 0) { + return 0; + } + while (low < high) { uint256 mid = Math.average(low, high); From b6c5abbde5b9ad2b5f12b26a862061d883aa744e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 12 Jul 2023 16:36:10 -0600 Subject: [PATCH 16/31] Add `GovernorTimelockControl` address to `TimelockController` salt (#4432) Co-authored-by: Francisco Giordano --- .changeset/purple-cats-cheer.md | 5 +++++ .../extensions/GovernorTimelockControl.sol | 17 ++++++++++++++--- .../extensions/GovernorTimelockControl.test.js | 8 ++++++-- 3 files changed, 25 insertions(+), 5 deletions(-) create mode 100644 .changeset/purple-cats-cheer.md diff --git a/.changeset/purple-cats-cheer.md b/.changeset/purple-cats-cheer.md new file mode 100644 index 00000000000..7e9dc1c4ddd --- /dev/null +++ b/.changeset/purple-cats-cheer.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`GovernorTimelockControl`: Add the Governor instance address as part of the TimelockController operation `salt` to avoid operation id collisions between governors using the same TimelockController. diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 11156940843..e6dea036f4f 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -106,8 +106,9 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { } uint256 delay = _timelock.getMinDelay(); - _timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, descriptionHash); - _timelock.scheduleBatch(targets, values, calldatas, 0, descriptionHash, delay); + bytes32 salt = _timelockSalt(descriptionHash); + _timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, salt); + _timelock.scheduleBatch(targets, values, calldatas, 0, salt, delay); emit ProposalQueued(proposalId, block.timestamp + delay); @@ -125,7 +126,7 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes32 descriptionHash ) internal virtual override { // execute - _timelock.executeBatch{value: msg.value}(targets, values, calldatas, 0, descriptionHash); + _timelock.executeBatch{value: msg.value}(targets, values, calldatas, 0, _timelockSalt(descriptionHash)); // cleanup for refund delete _timelockIds[proposalId]; } @@ -177,4 +178,14 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { emit TimelockChange(address(_timelock), address(newTimelock)); _timelock = newTimelock; } + + /** + * @dev Computes the {TimelockController} operation salt. + * + * It is computed with the governor address itself to avoid collisions across governor instances using the + * same timelock. + */ + function _timelockSalt(bytes32 descriptionHash) private view returns (bytes32) { + return bytes20(address(this)) ^ descriptionHash; + } } diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 5954d4117f7..8dde8acb97b 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -37,6 +37,9 @@ contract('GovernorTimelockControl', function (accounts) { for (const { mode, Token } of TOKENS) { describe(`using ${Token._json.contractName}`, function () { + const timelockSalt = (address, descriptionHash) => + '0x' + web3.utils.toBN(address).shln(96).xor(web3.utils.toBN(descriptionHash)).toString(16, 64); + beforeEach(async function () { const [deployer] = await web3.eth.getAccounts(); @@ -86,10 +89,11 @@ contract('GovernorTimelockControl', function (accounts) { ], '', ); + this.proposal.timelockid = await this.timelock.hashOperationBatch( ...this.proposal.shortProposal.slice(0, 3), '0x0', - this.proposal.shortProposal[3], + timelockSalt(this.mock.address, this.proposal.shortProposal[3]), ); }); @@ -204,7 +208,7 @@ contract('GovernorTimelockControl', function (accounts) { await this.timelock.executeBatch( ...this.proposal.shortProposal.slice(0, 3), '0x0', - this.proposal.shortProposal[3], + timelockSalt(this.mock.address, this.proposal.shortProposal[3]), ); await expectRevertCustomError(this.helper.execute(), 'GovernorUnexpectedProposalState', [ From 4d4a509b1f8e521583d82c042ef4bce919fe8e12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 12 Jul 2023 17:30:19 -0600 Subject: [PATCH 17/31] Add `GovernorTimelockControl` address to `TimelockController` salt (#4432) Co-authored-by: Francisco Giordano From a55af77c75e8e5ce5205d2787856ee52055adf11 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Thu, 13 Jul 2023 05:11:12 +0200 Subject: [PATCH 18/31] Natspec update for TimelockController (#4454) --- contracts/governance/TimelockController.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index be86f7b8827..aaaeb255b99 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -102,7 +102,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @dev Initializes the contract with the following parameters: * - * - `minDelay`: initial minimum delay for operations + * - `minDelay`: initial minimum delay in seconds for operations * - `proposers`: accounts to be granted proposer and canceller roles * - `executors`: accounts to be granted executor role * - `admin`: optional account to be granted admin role; disable with zero address @@ -218,7 +218,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { } /** - * @dev Returns the minimum delay for an operation to become valid. + * @dev Returns the minimum delay in seconds for an operation to become valid. * * This value can be changed by executing an operation that calls `updateDelay`. */ From 84db204a4155b6a058a6e51ea2e053c4d29d29c8 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 13 Jul 2023 17:52:03 -0300 Subject: [PATCH 19/31] Rename rounding modes and complete with fourth (#4455) Co-authored-by: ernestognw --- .changeset/wild-rockets-rush.md | 5 + contracts/mocks/docs/ERC4626Fees.sol | 4 +- contracts/token/ERC20/extensions/ERC4626.sol | 14 +- contracts/utils/Arrays.sol | 2 +- contracts/utils/math/Math.sol | 37 ++- docs/modules/ROOT/pages/erc4626.adoc | 2 +- test/helpers/enums.js | 2 +- test/token/ERC20/extensions/ERC4626.test.js | 6 +- test/utils/math/Math.t.sol | 16 +- test/utils/math/Math.test.js | 311 ++++++++++--------- 10 files changed, 218 insertions(+), 181 deletions(-) create mode 100644 .changeset/wild-rockets-rush.md diff --git a/.changeset/wild-rockets-rush.md b/.changeset/wild-rockets-rush.md new file mode 100644 index 00000000000..7fc6f598d29 --- /dev/null +++ b/.changeset/wild-rockets-rush.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Math`: Renamed members of `Rounding` enum, and added a new rounding mode for "away from zero". diff --git a/contracts/mocks/docs/ERC4626Fees.sol b/contracts/mocks/docs/ERC4626Fees.sol index c82279a605c..49c1095ab47 100644 --- a/contracts/mocks/docs/ERC4626Fees.sol +++ b/contracts/mocks/docs/ERC4626Fees.sol @@ -81,10 +81,10 @@ abstract contract ERC4626Fees is ERC4626 { } function _feeOnRaw(uint256 assets, uint256 feeBasePoint) private pure returns (uint256) { - return assets.mulDiv(feeBasePoint, 1e5, Math.Rounding.Up); + return assets.mulDiv(feeBasePoint, 1e5, Math.Rounding.Ceil); } function _feeOnTotal(uint256 assets, uint256 feeBasePoint) private pure returns (uint256) { - return assets.mulDiv(feeBasePoint, feeBasePoint + 1e5, Math.Rounding.Up); + return assets.mulDiv(feeBasePoint, feeBasePoint + 1e5, Math.Rounding.Ceil); } } diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 11f1cd59306..ad3b5a170d2 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -119,12 +119,12 @@ abstract contract ERC4626 is ERC20, IERC4626 { /** @dev See {IERC4626-convertToShares}. */ function convertToShares(uint256 assets) public view virtual returns (uint256) { - return _convertToShares(assets, Math.Rounding.Down); + return _convertToShares(assets, Math.Rounding.Floor); } /** @dev See {IERC4626-convertToAssets}. */ function convertToAssets(uint256 shares) public view virtual returns (uint256) { - return _convertToAssets(shares, Math.Rounding.Down); + return _convertToAssets(shares, Math.Rounding.Floor); } /** @dev See {IERC4626-maxDeposit}. */ @@ -139,7 +139,7 @@ abstract contract ERC4626 is ERC20, IERC4626 { /** @dev See {IERC4626-maxWithdraw}. */ function maxWithdraw(address owner) public view virtual returns (uint256) { - return _convertToAssets(balanceOf(owner), Math.Rounding.Down); + return _convertToAssets(balanceOf(owner), Math.Rounding.Floor); } /** @dev See {IERC4626-maxRedeem}. */ @@ -149,22 +149,22 @@ abstract contract ERC4626 is ERC20, IERC4626 { /** @dev See {IERC4626-previewDeposit}. */ function previewDeposit(uint256 assets) public view virtual returns (uint256) { - return _convertToShares(assets, Math.Rounding.Down); + return _convertToShares(assets, Math.Rounding.Floor); } /** @dev See {IERC4626-previewMint}. */ function previewMint(uint256 shares) public view virtual returns (uint256) { - return _convertToAssets(shares, Math.Rounding.Up); + return _convertToAssets(shares, Math.Rounding.Ceil); } /** @dev See {IERC4626-previewWithdraw}. */ function previewWithdraw(uint256 assets) public view virtual returns (uint256) { - return _convertToShares(assets, Math.Rounding.Up); + return _convertToShares(assets, Math.Rounding.Ceil); } /** @dev See {IERC4626-previewRedeem}. */ function previewRedeem(uint256 shares) public view virtual returns (uint256) { - return _convertToAssets(shares, Math.Rounding.Down); + return _convertToAssets(shares, Math.Rounding.Floor); } /** @dev See {IERC4626-deposit}. */ diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index 0c16b07cf57..b2eeaac5063 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -33,7 +33,7 @@ library Arrays { uint256 mid = Math.average(low, high); // Note that mid will always be strictly less than high (i.e. it will be a valid array index) - // because Math.average rounds down (it does integer division with truncation). + // because Math.average rounds towards zero (it does integer division with truncation). if (unsafeAccess(array, mid).value > element) { high = mid; } else { diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index f55b69afc12..e3d7f799d41 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -13,9 +13,10 @@ library Math { error MathOverflowedMulDiv(); enum Rounding { - Down, // Toward negative infinity - Up, // Toward infinity - Zero // Toward zero + Floor, // Toward negative infinity + Ceil, // Toward positive infinity + Trunc, // Toward zero + Expand // Away from zero } /** @@ -100,8 +101,8 @@ library Math { /** * @dev Returns the ceiling of the division of two numbers. * - * This differs from standard division with `/` in that it rounds up instead - * of rounding down. + * This differs from standard division with `/` in that it rounds towards infinity instead + * of rounding towards zero. */ function ceilDiv(uint256 a, uint256 b) internal pure returns (uint256) { if (b == 0) { @@ -206,14 +207,15 @@ library Math { */ function mulDiv(uint256 x, uint256 y, uint256 denominator, Rounding rounding) internal pure returns (uint256) { uint256 result = mulDiv(x, y, denominator); - if (rounding == Rounding.Up && mulmod(x, y, denominator) > 0) { + if (unsignedRoundsUp(rounding) && mulmod(x, y, denominator) > 0) { result += 1; } return result; } /** - * @dev Returns the square root of a number. If the number is not a perfect square, the value is rounded down. + * @dev Returns the square root of a number. If the number is not a perfect square, the value is rounded + * towards zero. * * Inspired by Henry S. Warren, Jr.'s "Hacker's Delight" (Chapter 11). */ @@ -256,12 +258,12 @@ library Math { function sqrt(uint256 a, Rounding rounding) internal pure returns (uint256) { unchecked { uint256 result = sqrt(a); - return result + (rounding == Rounding.Up && result * result < a ? 1 : 0); + return result + (unsignedRoundsUp(rounding) && result * result < a ? 1 : 0); } } /** - * @dev Return the log in base 2, rounded down, of a positive value. + * @dev Return the log in base 2 of a positive value rounded towards zero. * Returns 0 if given 0. */ function log2(uint256 value) internal pure returns (uint256) { @@ -309,12 +311,12 @@ library Math { function log2(uint256 value, Rounding rounding) internal pure returns (uint256) { unchecked { uint256 result = log2(value); - return result + (rounding == Rounding.Up && 1 << result < value ? 1 : 0); + return result + (unsignedRoundsUp(rounding) && 1 << result < value ? 1 : 0); } } /** - * @dev Return the log in base 10, rounded down, of a positive value. + * @dev Return the log in base 10 of a positive value rounded towards zero. * Returns 0 if given 0. */ function log10(uint256 value) internal pure returns (uint256) { @@ -358,12 +360,12 @@ library Math { function log10(uint256 value, Rounding rounding) internal pure returns (uint256) { unchecked { uint256 result = log10(value); - return result + (rounding == Rounding.Up && 10 ** result < value ? 1 : 0); + return result + (unsignedRoundsUp(rounding) && 10 ** result < value ? 1 : 0); } } /** - * @dev Return the log in base 256, rounded down, of a positive value. + * @dev Return the log in base 256 of a positive value rounded towards zero. * Returns 0 if given 0. * * Adding one to the result gives the number of pairs of hex symbols needed to represent `value` as a hex string. @@ -401,7 +403,14 @@ library Math { function log256(uint256 value, Rounding rounding) internal pure returns (uint256) { unchecked { uint256 result = log256(value); - return result + (rounding == Rounding.Up && 1 << (result << 3) < value ? 1 : 0); + return result + (unsignedRoundsUp(rounding) && 1 << (result << 3) < value ? 1 : 0); } } + + /** + * @dev Returns whether a provided rounding mode is considered rounding up for unsigned integers. + */ + function unsignedRoundsUp(Rounding rounding) internal pure returns (bool) { + return uint8(rounding) % 2 == 1; + } } diff --git a/docs/modules/ROOT/pages/erc4626.adoc b/docs/modules/ROOT/pages/erc4626.adoc index c8adce73672..43ec3ebb939 100644 --- a/docs/modules/ROOT/pages/erc4626.adoc +++ b/docs/modules/ROOT/pages/erc4626.adoc @@ -29,7 +29,7 @@ image::erc4626-rate-loglogext.png[More exchange rates in logarithmic scale] === The attack -When depositing tokens, the number of shares a user gets is rounded down. This rounding takes away value from the user in favor or the vault (i.e. in favor of all the current share holders). This rounding is often negligible because of the amount at stake. If you deposit 1e9 shares worth of tokens, the rounding will have you lose at most 0.0000001% of your deposit. However if you deposit 10 shares worth of tokens, you could lose 10% of your deposit. Even worse, if you deposit <1 share worth of tokens, then you get 0 shares, and you basically made a donation. +When depositing tokens, the number of shares a user gets is rounded towards zero. This rounding takes away value from the user in favor or the vault (i.e. in favor of all the current share holders). This rounding is often negligible because of the amount at stake. If you deposit 1e9 shares worth of tokens, the rounding will have you lose at most 0.0000001% of your deposit. However if you deposit 10 shares worth of tokens, you could lose 10% of your deposit. Even worse, if you deposit <1 share worth of tokens, then you get 0 shares, and you basically made a donation. For a given amount of assets, the more shares you receive the safer you are. If you want to limit your losses to at most 1%, you need to receive at least 100 shares. diff --git a/test/helpers/enums.js b/test/helpers/enums.js index 75746e08744..6280e0f319b 100644 --- a/test/helpers/enums.js +++ b/test/helpers/enums.js @@ -6,6 +6,6 @@ module.exports = { Enum, ProposalState: Enum('Pending', 'Active', 'Canceled', 'Defeated', 'Succeeded', 'Queued', 'Expired', 'Executed'), VoteType: Enum('Against', 'For', 'Abstain'), - Rounding: Enum('Down', 'Up', 'Zero'), + Rounding: Enum('Floor', 'Ceil', 'Trunc', 'Expand'), OperationState: Enum('Unset', 'Waiting', 'Ready', 'Done'), }; diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index 9dfc4c3b818..aead1d50a4c 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -965,8 +965,8 @@ contract('ERC4626', function (accounts) { } // 5. Bob mints 2000 shares (costs 3001 assets) - // NOTE: Bob's assets spent got rounded up - // NOTE: Alices's vault assets got rounded up + // NOTE: Bob's assets spent got rounded towards infinity + // NOTE: Alices's vault assets got rounded towards infinity { const { tx } = await this.vault.mint(2000, user2, { from: user2 }); await expectEvent.inTransaction(tx, this.token, 'Transfer', { @@ -1056,7 +1056,7 @@ contract('ERC4626', function (accounts) { } // 9. Alice withdraws 3643 assets (2000 shares) - // NOTE: Bob's assets have been rounded back up + // NOTE: Bob's assets have been rounded back towards infinity { const { tx } = await this.vault.withdraw(3643, user1, user1, { from: user1 }); await expectEvent.inTransaction(tx, this.vault, 'Transfer', { diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index 5a6be476f3e..d6b0c5d0349 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -31,12 +31,12 @@ contract MathTest is Test { // square of result is bigger than input if (_squareBigger(result, input)) { - assertTrue(rounding == Math.Rounding.Up); + assertTrue(Math.unsignedRoundsUp(rounding)); assertTrue(_squareSmaller(result - 1, input)); } // square of result is smaller than input else if (_squareSmaller(result, input)) { - assertFalse(rounding == Math.Rounding.Up); + assertFalse(Math.unsignedRoundsUp(rounding)); assertTrue(_squareBigger(result + 1, input)); } // input is perfect square @@ -63,10 +63,10 @@ contract MathTest is Test { if (input == 0) { assertEq(result, 0); } else if (_powerOf2Bigger(result, input)) { - assertTrue(rounding == Math.Rounding.Up); + assertTrue(Math.unsignedRoundsUp(rounding)); assertTrue(_powerOf2Smaller(result - 1, input)); } else if (_powerOf2Smaller(result, input)) { - assertFalse(rounding == Math.Rounding.Up); + assertFalse(Math.unsignedRoundsUp(rounding)); assertTrue(_powerOf2Bigger(result + 1, input)); } else { assertEq(2 ** result, input); @@ -90,10 +90,10 @@ contract MathTest is Test { if (input == 0) { assertEq(result, 0); } else if (_powerOf10Bigger(result, input)) { - assertTrue(rounding == Math.Rounding.Up); + assertTrue(Math.unsignedRoundsUp(rounding)); assertTrue(_powerOf10Smaller(result - 1, input)); } else if (_powerOf10Smaller(result, input)) { - assertFalse(rounding == Math.Rounding.Up); + assertFalse(Math.unsignedRoundsUp(rounding)); assertTrue(_powerOf10Bigger(result + 1, input)); } else { assertEq(10 ** result, input); @@ -117,10 +117,10 @@ contract MathTest is Test { if (input == 0) { assertEq(result, 0); } else if (_powerOf256Bigger(result, input)) { - assertTrue(rounding == Math.Rounding.Up); + assertTrue(Math.unsignedRoundsUp(rounding)); assertTrue(_powerOf256Smaller(result - 1, input)); } else if (_powerOf256Smaller(result, input)) { - assertFalse(rounding == Math.Rounding.Up); + assertFalse(Math.unsignedRoundsUp(rounding)); assertTrue(_powerOf256Bigger(result + 1, input)); } else { assertEq(256 ** result, input); diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index afd822b1788..7d4a58c81b1 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -6,6 +6,9 @@ const { expectRevertCustomError } = require('../../helpers/customError.js'); const Math = artifacts.require('$Math'); +const RoundingDown = [Rounding.Floor, Rounding.Trunc]; +const RoundingUp = [Rounding.Ceil, Rounding.Expand]; + function expectStruct(value, expected) { for (const key in expected) { if (BN.isBN(value[key])) { @@ -244,202 +247,222 @@ contract('Math', function () { describe('muldiv', function () { it('divide by 0', async function () { - await expectRevert.unspecified(this.math.$mulDiv(1, 1, 0, Rounding.Down)); + await expectRevert.unspecified(this.math.$mulDiv(1, 1, 0, Rounding.Floor)); }); it('reverts with result higher than 2 ^ 256', async function () { - await expectRevertCustomError(this.math.$mulDiv(5, MAX_UINT256, 2, Rounding.Down), 'MathOverflowedMulDiv', []); + await expectRevertCustomError(this.math.$mulDiv(5, MAX_UINT256, 2, Rounding.Floor), 'MathOverflowedMulDiv', []); }); describe('does round down', async function () { it('small values', async function () { - expect(await this.math.$mulDiv('3', '4', '5', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$mulDiv('3', '5', '5', Rounding.Down)).to.be.bignumber.equal('3'); + for (const rounding of RoundingDown) { + expect(await this.math.$mulDiv('3', '4', '5', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$mulDiv('3', '5', '5', rounding)).to.be.bignumber.equal('3'); + } }); it('large values', async function () { - expect( - await this.math.$mulDiv(new BN('42'), MAX_UINT256_SUB1, MAX_UINT256, Rounding.Down), - ).to.be.bignumber.equal(new BN('41')); - - expect(await this.math.$mulDiv(new BN('17'), MAX_UINT256, MAX_UINT256, Rounding.Down)).to.be.bignumber.equal( - new BN('17'), - ); - - expect( - await this.math.$mulDiv(MAX_UINT256_SUB1, MAX_UINT256_SUB1, MAX_UINT256, Rounding.Down), - ).to.be.bignumber.equal(MAX_UINT256_SUB2); - - expect( - await this.math.$mulDiv(MAX_UINT256, MAX_UINT256_SUB1, MAX_UINT256, Rounding.Down), - ).to.be.bignumber.equal(MAX_UINT256_SUB1); - - expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256, MAX_UINT256, Rounding.Down)).to.be.bignumber.equal( - MAX_UINT256, - ); + for (const rounding of RoundingDown) { + expect(await this.math.$mulDiv(new BN('42'), MAX_UINT256_SUB1, MAX_UINT256, rounding)).to.be.bignumber.equal( + new BN('41'), + ); + + expect(await this.math.$mulDiv(new BN('17'), MAX_UINT256, MAX_UINT256, rounding)).to.be.bignumber.equal( + new BN('17'), + ); + + expect( + await this.math.$mulDiv(MAX_UINT256_SUB1, MAX_UINT256_SUB1, MAX_UINT256, rounding), + ).to.be.bignumber.equal(MAX_UINT256_SUB2); + + expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256_SUB1, MAX_UINT256, rounding)).to.be.bignumber.equal( + MAX_UINT256_SUB1, + ); + + expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256, MAX_UINT256, rounding)).to.be.bignumber.equal( + MAX_UINT256, + ); + } }); }); describe('does round up', async function () { it('small values', async function () { - expect(await this.math.$mulDiv('3', '4', '5', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.$mulDiv('3', '5', '5', Rounding.Up)).to.be.bignumber.equal('3'); + for (const rounding of RoundingUp) { + expect(await this.math.$mulDiv('3', '4', '5', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$mulDiv('3', '5', '5', rounding)).to.be.bignumber.equal('3'); + } }); it('large values', async function () { - expect(await this.math.$mulDiv(new BN('42'), MAX_UINT256_SUB1, MAX_UINT256, Rounding.Up)).to.be.bignumber.equal( - new BN('42'), - ); - - expect(await this.math.$mulDiv(new BN('17'), MAX_UINT256, MAX_UINT256, Rounding.Up)).to.be.bignumber.equal( - new BN('17'), - ); - - expect( - await this.math.$mulDiv(MAX_UINT256_SUB1, MAX_UINT256_SUB1, MAX_UINT256, Rounding.Up), - ).to.be.bignumber.equal(MAX_UINT256_SUB1); - - expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256_SUB1, MAX_UINT256, Rounding.Up)).to.be.bignumber.equal( - MAX_UINT256_SUB1, - ); - - expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256, MAX_UINT256, Rounding.Up)).to.be.bignumber.equal( - MAX_UINT256, - ); + for (const rounding of RoundingUp) { + expect(await this.math.$mulDiv(new BN('42'), MAX_UINT256_SUB1, MAX_UINT256, rounding)).to.be.bignumber.equal( + new BN('42'), + ); + + expect(await this.math.$mulDiv(new BN('17'), MAX_UINT256, MAX_UINT256, rounding)).to.be.bignumber.equal( + new BN('17'), + ); + + expect( + await this.math.$mulDiv(MAX_UINT256_SUB1, MAX_UINT256_SUB1, MAX_UINT256, rounding), + ).to.be.bignumber.equal(MAX_UINT256_SUB1); + + expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256_SUB1, MAX_UINT256, rounding)).to.be.bignumber.equal( + MAX_UINT256_SUB1, + ); + + expect(await this.math.$mulDiv(MAX_UINT256, MAX_UINT256, MAX_UINT256, rounding)).to.be.bignumber.equal( + MAX_UINT256, + ); + } }); }); }); describe('sqrt', function () { it('rounds down', async function () { - expect(await this.math.$sqrt('0', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$sqrt('1', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$sqrt('2', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$sqrt('3', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$sqrt('4', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$sqrt('144', Rounding.Down)).to.be.bignumber.equal('12'); - expect(await this.math.$sqrt('999999', Rounding.Down)).to.be.bignumber.equal('999'); - expect(await this.math.$sqrt('1000000', Rounding.Down)).to.be.bignumber.equal('1000'); - expect(await this.math.$sqrt('1000001', Rounding.Down)).to.be.bignumber.equal('1000'); - expect(await this.math.$sqrt('1002000', Rounding.Down)).to.be.bignumber.equal('1000'); - expect(await this.math.$sqrt('1002001', Rounding.Down)).to.be.bignumber.equal('1001'); - expect(await this.math.$sqrt(MAX_UINT256, Rounding.Down)).to.be.bignumber.equal( - '340282366920938463463374607431768211455', - ); + for (const rounding of RoundingDown) { + expect(await this.math.$sqrt('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$sqrt('1', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$sqrt('2', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$sqrt('3', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$sqrt('4', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$sqrt('144', rounding)).to.be.bignumber.equal('12'); + expect(await this.math.$sqrt('999999', rounding)).to.be.bignumber.equal('999'); + expect(await this.math.$sqrt('1000000', rounding)).to.be.bignumber.equal('1000'); + expect(await this.math.$sqrt('1000001', rounding)).to.be.bignumber.equal('1000'); + expect(await this.math.$sqrt('1002000', rounding)).to.be.bignumber.equal('1000'); + expect(await this.math.$sqrt('1002001', rounding)).to.be.bignumber.equal('1001'); + expect(await this.math.$sqrt(MAX_UINT256, rounding)).to.be.bignumber.equal( + '340282366920938463463374607431768211455', + ); + } }); it('rounds up', async function () { - expect(await this.math.$sqrt('0', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.$sqrt('1', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$sqrt('2', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$sqrt('3', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$sqrt('4', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$sqrt('144', Rounding.Up)).to.be.bignumber.equal('12'); - expect(await this.math.$sqrt('999999', Rounding.Up)).to.be.bignumber.equal('1000'); - expect(await this.math.$sqrt('1000000', Rounding.Up)).to.be.bignumber.equal('1000'); - expect(await this.math.$sqrt('1000001', Rounding.Up)).to.be.bignumber.equal('1001'); - expect(await this.math.$sqrt('1002000', Rounding.Up)).to.be.bignumber.equal('1001'); - expect(await this.math.$sqrt('1002001', Rounding.Up)).to.be.bignumber.equal('1001'); - expect(await this.math.$sqrt(MAX_UINT256, Rounding.Up)).to.be.bignumber.equal( - '340282366920938463463374607431768211456', - ); + for (const rounding of RoundingUp) { + expect(await this.math.$sqrt('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$sqrt('1', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$sqrt('2', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$sqrt('3', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$sqrt('4', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$sqrt('144', rounding)).to.be.bignumber.equal('12'); + expect(await this.math.$sqrt('999999', rounding)).to.be.bignumber.equal('1000'); + expect(await this.math.$sqrt('1000000', rounding)).to.be.bignumber.equal('1000'); + expect(await this.math.$sqrt('1000001', rounding)).to.be.bignumber.equal('1001'); + expect(await this.math.$sqrt('1002000', rounding)).to.be.bignumber.equal('1001'); + expect(await this.math.$sqrt('1002001', rounding)).to.be.bignumber.equal('1001'); + expect(await this.math.$sqrt(MAX_UINT256, rounding)).to.be.bignumber.equal( + '340282366920938463463374607431768211456', + ); + } }); }); describe('log', function () { describe('log2', function () { it('rounds down', async function () { - // For some reason calling .$log2() directly fails - expect(await this.math.methods['$log2(uint256,uint8)']('0', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.methods['$log2(uint256,uint8)']('1', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.methods['$log2(uint256,uint8)']('2', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.methods['$log2(uint256,uint8)']('3', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.methods['$log2(uint256,uint8)']('4', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.methods['$log2(uint256,uint8)']('5', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.methods['$log2(uint256,uint8)']('6', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.methods['$log2(uint256,uint8)']('7', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.methods['$log2(uint256,uint8)']('8', Rounding.Down)).to.be.bignumber.equal('3'); - expect(await this.math.methods['$log2(uint256,uint8)']('9', Rounding.Down)).to.be.bignumber.equal('3'); - expect(await this.math.methods['$log2(uint256,uint8)'](MAX_UINT256, Rounding.Down)).to.be.bignumber.equal( - '255', - ); + for (const rounding of RoundingDown) { + expect(await this.math.methods['$log2(uint256,uint8)']('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.methods['$log2(uint256,uint8)']('1', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.methods['$log2(uint256,uint8)']('2', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.methods['$log2(uint256,uint8)']('3', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.methods['$log2(uint256,uint8)']('4', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.methods['$log2(uint256,uint8)']('5', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.methods['$log2(uint256,uint8)']('6', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.methods['$log2(uint256,uint8)']('7', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.methods['$log2(uint256,uint8)']('8', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.methods['$log2(uint256,uint8)']('9', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.methods['$log2(uint256,uint8)'](MAX_UINT256, rounding)).to.be.bignumber.equal('255'); + } }); it('rounds up', async function () { - // For some reason calling .$log2() directly fails - expect(await this.math.methods['$log2(uint256,uint8)']('0', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.methods['$log2(uint256,uint8)']('1', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.methods['$log2(uint256,uint8)']('2', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.methods['$log2(uint256,uint8)']('3', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.methods['$log2(uint256,uint8)']('4', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.methods['$log2(uint256,uint8)']('5', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.methods['$log2(uint256,uint8)']('6', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.methods['$log2(uint256,uint8)']('7', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.methods['$log2(uint256,uint8)']('8', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.methods['$log2(uint256,uint8)']('9', Rounding.Up)).to.be.bignumber.equal('4'); - expect(await this.math.methods['$log2(uint256,uint8)'](MAX_UINT256, Rounding.Up)).to.be.bignumber.equal('256'); + for (const rounding of RoundingUp) { + expect(await this.math.methods['$log2(uint256,uint8)']('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.methods['$log2(uint256,uint8)']('1', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.methods['$log2(uint256,uint8)']('2', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.methods['$log2(uint256,uint8)']('3', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.methods['$log2(uint256,uint8)']('4', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.methods['$log2(uint256,uint8)']('5', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.methods['$log2(uint256,uint8)']('6', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.methods['$log2(uint256,uint8)']('7', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.methods['$log2(uint256,uint8)']('8', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.methods['$log2(uint256,uint8)']('9', rounding)).to.be.bignumber.equal('4'); + expect(await this.math.methods['$log2(uint256,uint8)'](MAX_UINT256, rounding)).to.be.bignumber.equal('256'); + } }); }); describe('log10', function () { it('rounds down', async function () { - expect(await this.math.$log10('0', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log10('1', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log10('2', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log10('9', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log10('10', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$log10('11', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$log10('99', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$log10('100', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$log10('101', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$log10('999', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$log10('1000', Rounding.Down)).to.be.bignumber.equal('3'); - expect(await this.math.$log10('1001', Rounding.Down)).to.be.bignumber.equal('3'); - expect(await this.math.$log10(MAX_UINT256, Rounding.Down)).to.be.bignumber.equal('77'); + for (const rounding of RoundingDown) { + expect(await this.math.$log10('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log10('1', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log10('2', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log10('9', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log10('10', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log10('11', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log10('99', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log10('100', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log10('101', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log10('999', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log10('1000', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$log10('1001', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$log10(MAX_UINT256, rounding)).to.be.bignumber.equal('77'); + } }); it('rounds up', async function () { - expect(await this.math.$log10('0', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.$log10('1', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.$log10('2', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$log10('9', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$log10('10', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$log10('11', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$log10('99', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$log10('100', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$log10('101', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.$log10('999', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.$log10('1000', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.$log10('1001', Rounding.Up)).to.be.bignumber.equal('4'); - expect(await this.math.$log10(MAX_UINT256, Rounding.Up)).to.be.bignumber.equal('78'); + for (const rounding of RoundingUp) { + expect(await this.math.$log10('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log10('1', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log10('2', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log10('9', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log10('10', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log10('11', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log10('99', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log10('100', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log10('101', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$log10('999', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$log10('1000', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$log10('1001', rounding)).to.be.bignumber.equal('4'); + expect(await this.math.$log10(MAX_UINT256, rounding)).to.be.bignumber.equal('78'); + } }); }); describe('log256', function () { it('rounds down', async function () { - expect(await this.math.$log256('0', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log256('1', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log256('2', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log256('255', Rounding.Down)).to.be.bignumber.equal('0'); - expect(await this.math.$log256('256', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$log256('257', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$log256('65535', Rounding.Down)).to.be.bignumber.equal('1'); - expect(await this.math.$log256('65536', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$log256('65537', Rounding.Down)).to.be.bignumber.equal('2'); - expect(await this.math.$log256(MAX_UINT256, Rounding.Down)).to.be.bignumber.equal('31'); + for (const rounding of RoundingDown) { + expect(await this.math.$log256('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log256('1', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log256('2', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log256('255', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log256('256', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log256('257', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log256('65535', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log256('65536', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log256('65537', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log256(MAX_UINT256, rounding)).to.be.bignumber.equal('31'); + } }); it('rounds up', async function () { - expect(await this.math.$log256('0', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.$log256('1', Rounding.Up)).to.be.bignumber.equal('0'); - expect(await this.math.$log256('2', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$log256('255', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$log256('256', Rounding.Up)).to.be.bignumber.equal('1'); - expect(await this.math.$log256('257', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$log256('65535', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$log256('65536', Rounding.Up)).to.be.bignumber.equal('2'); - expect(await this.math.$log256('65537', Rounding.Up)).to.be.bignumber.equal('3'); - expect(await this.math.$log256(MAX_UINT256, Rounding.Up)).to.be.bignumber.equal('32'); + for (const rounding of RoundingUp) { + expect(await this.math.$log256('0', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log256('1', rounding)).to.be.bignumber.equal('0'); + expect(await this.math.$log256('2', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log256('255', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log256('256', rounding)).to.be.bignumber.equal('1'); + expect(await this.math.$log256('257', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log256('65535', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log256('65536', rounding)).to.be.bignumber.equal('2'); + expect(await this.math.$log256('65537', rounding)).to.be.bignumber.equal('3'); + expect(await this.math.$log256(MAX_UINT256, rounding)).to.be.bignumber.equal('32'); + } }); }); }); From 9cf873ea1481764c3be51e203861475d6b302450 Mon Sep 17 00:00:00 2001 From: Renan Souza Date: Thu, 13 Jul 2023 18:54:22 -0300 Subject: [PATCH 20/31] Change access folder structure (#4359) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco --- .changeset/spicy-sheep-eat.md | 5 +++++ contracts/access/README.adoc | 6 +++++- .../{ => extensions}/AccessControlDefaultAdminRules.sol | 8 ++++---- .../access/{ => extensions}/AccessControlEnumerable.sol | 4 ++-- .../{ => extensions}/IAccessControlDefaultAdminRules.sol | 2 +- .../access/{ => extensions}/IAccessControlEnumerable.sol | 2 +- .../AccessControlDefaultAdminRules.test.js | 2 +- .../{ => extensions}/AccessControlEnumerable.test.js | 2 +- 8 files changed, 20 insertions(+), 11 deletions(-) create mode 100644 .changeset/spicy-sheep-eat.md rename contracts/access/{ => extensions}/AccessControlDefaultAdminRules.sol (98%) rename contracts/access/{ => extensions}/AccessControlEnumerable.sol (95%) rename contracts/access/{ => extensions}/IAccessControlDefaultAdminRules.sol (99%) rename contracts/access/{ => extensions}/IAccessControlEnumerable.sol (95%) rename test/access/{ => extensions}/AccessControlDefaultAdminRules.test.js (95%) rename test/access/{ => extensions}/AccessControlEnumerable.test.js (92%) diff --git a/.changeset/spicy-sheep-eat.md b/.changeset/spicy-sheep-eat.md new file mode 100644 index 00000000000..17b6d558514 --- /dev/null +++ b/.changeset/spicy-sheep-eat.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`access`: Move `AccessControl` extensions to a dedicated directory. diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 117cd7c9735..b03753d9652 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -8,7 +8,7 @@ This directory provides ways to restrict who can access the functions of a contr - {AccessControl} provides a general role based access control mechanism. Multiple hierarchical roles can be created and assigned each to multiple accounts. - {Ownable} is a simpler mechanism with a single owner "role" that can be assigned to a single account. This simpler mechanism can be useful for quick tests but projects with production concerns are likely to outgrow it. -== Authorization +== Core {{Ownable}} @@ -18,8 +18,12 @@ This directory provides ways to restrict who can access the functions of a contr {{AccessControl}} +== Extensions + {{IAccessControlEnumerable}} {{AccessControlEnumerable}} +{{IAccessControlDefaultAdminRules}} + {{AccessControlDefaultAdminRules}} diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/extensions/AccessControlDefaultAdminRules.sol similarity index 98% rename from contracts/access/AccessControlDefaultAdminRules.sol rename to contracts/access/extensions/AccessControlDefaultAdminRules.sol index 4572a328d6a..e8820cb4b4f 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/extensions/AccessControlDefaultAdminRules.sol @@ -3,11 +3,11 @@ pragma solidity ^0.8.19; -import {AccessControl, IAccessControl} from "./AccessControl.sol"; import {IAccessControlDefaultAdminRules} from "./IAccessControlDefaultAdminRules.sol"; -import {SafeCast} from "../utils/math/SafeCast.sol"; -import {Math} from "../utils/math/Math.sol"; -import {IERC5313} from "../interfaces/IERC5313.sol"; +import {AccessControl, IAccessControl} from "../AccessControl.sol"; +import {SafeCast} from "../../utils/math/SafeCast.sol"; +import {Math} from "../../utils/math/Math.sol"; +import {IERC5313} from "../../interfaces/IERC5313.sol"; /** * @dev Extension of {AccessControl} that allows specifying special rules to manage diff --git a/contracts/access/AccessControlEnumerable.sol b/contracts/access/extensions/AccessControlEnumerable.sol similarity index 95% rename from contracts/access/AccessControlEnumerable.sol rename to contracts/access/extensions/AccessControlEnumerable.sol index 6f160e4e8ce..2512c43796f 100644 --- a/contracts/access/AccessControlEnumerable.sol +++ b/contracts/access/extensions/AccessControlEnumerable.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.19; import {IAccessControlEnumerable} from "./IAccessControlEnumerable.sol"; -import {AccessControl} from "./AccessControl.sol"; -import {EnumerableSet} from "../utils/structs/EnumerableSet.sol"; +import {AccessControl} from "../AccessControl.sol"; +import {EnumerableSet} from "../../utils/structs/EnumerableSet.sol"; /** * @dev Extension of {AccessControl} that allows enumerating the members of each role. diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/extensions/IAccessControlDefaultAdminRules.sol similarity index 99% rename from contracts/access/IAccessControlDefaultAdminRules.sol rename to contracts/access/extensions/IAccessControlDefaultAdminRules.sol index a1afb3d22e5..7dac1c1f276 100644 --- a/contracts/access/IAccessControlDefaultAdminRules.sol +++ b/contracts/access/extensions/IAccessControlDefaultAdminRules.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; -import {IAccessControl} from "./IAccessControl.sol"; +import {IAccessControl} from "../IAccessControl.sol"; /** * @dev External interface of AccessControlDefaultAdminRules declared to support ERC165 detection. diff --git a/contracts/access/IAccessControlEnumerable.sol b/contracts/access/extensions/IAccessControlEnumerable.sol similarity index 95% rename from contracts/access/IAccessControlEnumerable.sol rename to contracts/access/extensions/IAccessControlEnumerable.sol index 1bd88a4f4de..bc59ed58635 100644 --- a/contracts/access/IAccessControlEnumerable.sol +++ b/contracts/access/extensions/IAccessControlEnumerable.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; -import {IAccessControl} from "./IAccessControl.sol"; +import {IAccessControl} from "../IAccessControl.sol"; /** * @dev External interface of AccessControlEnumerable declared to support ERC165 detection. diff --git a/test/access/AccessControlDefaultAdminRules.test.js b/test/access/extensions/AccessControlDefaultAdminRules.test.js similarity index 95% rename from test/access/AccessControlDefaultAdminRules.test.js rename to test/access/extensions/AccessControlDefaultAdminRules.test.js index b8eae322088..af34143f198 100644 --- a/test/access/AccessControlDefaultAdminRules.test.js +++ b/test/access/extensions/AccessControlDefaultAdminRules.test.js @@ -2,7 +2,7 @@ const { time, constants, expectRevert } = require('@openzeppelin/test-helpers'); const { shouldBehaveLikeAccessControl, shouldBehaveLikeAccessControlDefaultAdminRules, -} = require('./AccessControl.behavior.js'); +} = require('../AccessControl.behavior.js'); const AccessControlDefaultAdminRules = artifacts.require('$AccessControlDefaultAdminRules'); diff --git a/test/access/AccessControlEnumerable.test.js b/test/access/extensions/AccessControlEnumerable.test.js similarity index 92% rename from test/access/AccessControlEnumerable.test.js rename to test/access/extensions/AccessControlEnumerable.test.js index 429f22f8f1a..7dfb0bce8b1 100644 --- a/test/access/AccessControlEnumerable.test.js +++ b/test/access/extensions/AccessControlEnumerable.test.js @@ -2,7 +2,7 @@ const { DEFAULT_ADMIN_ROLE, shouldBehaveLikeAccessControl, shouldBehaveLikeAccessControlEnumerable, -} = require('./AccessControl.behavior.js'); +} = require('../AccessControl.behavior.js'); const AccessControlEnumerable = artifacts.require('$AccessControlEnumerable'); From 121be5dd09caa2f7ce731f0806b0e14761023bd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 13 Jul 2023 16:25:22 -0600 Subject: [PATCH 21/31] Make `TransparentUpgradeableProxy` deploy its `ProxyAdmin` and optimize proxy interfaces (#4382) Co-authored-by: Francisco Co-authored-by: Eric Lau Co-authored-by: Hadrien Croubois --- .changeset/empty-taxis-kiss.md | 5 + .changeset/tender-shirts-turn.md | 5 + contracts/mocks/UpgreadeableBeaconMock.sol | 12 ++ .../mocks/proxy/ClashingImplementation.sol | 2 +- contracts/mocks/proxy/UUPSUpgradeableMock.sol | 6 +- contracts/proxy/ERC1967/ERC1967Proxy.sol | 6 +- contracts/proxy/ERC1967/ERC1967Utils.sol | 43 ++-- contracts/proxy/beacon/BeaconProxy.sol | 3 +- contracts/proxy/transparent/ProxyAdmin.sol | 24 +-- .../TransparentUpgradeableProxy.sol | 97 +++------ contracts/proxy/utils/UUPSUpgradeable.sol | 34 ++- .../GovernorCompatibilityBravo.test.js | 13 +- .../GovernorTimelockCompound.test.js | 13 +- test/helpers/account.js | 14 ++ test/helpers/create.js | 23 ++ test/helpers/create2.js | 11 - test/helpers/erc1967.js | 17 +- test/proxy/Clones.test.js | 2 +- test/proxy/ERC1967/ERC1967Proxy.test.js | 9 +- test/proxy/ERC1967/ERC1967Utils.test.js | 160 ++++++++++++++ test/proxy/Proxy.behaviour.js | 83 ++----- test/proxy/beacon/BeaconProxy.test.js | 16 +- test/proxy/transparent/ProxyAdmin.test.js | 29 ++- .../TransparentUpgradeableProxy.behaviour.js | 203 +++++++++--------- .../TransparentUpgradeableProxy.test.js | 19 +- test/proxy/utils/UUPSUpgradeable.test.js | 24 ++- test/utils/Create2.test.js | 2 +- 27 files changed, 520 insertions(+), 355 deletions(-) create mode 100644 .changeset/empty-taxis-kiss.md create mode 100644 .changeset/tender-shirts-turn.md create mode 100644 contracts/mocks/UpgreadeableBeaconMock.sol create mode 100644 test/helpers/account.js create mode 100644 test/helpers/create.js delete mode 100644 test/helpers/create2.js create mode 100644 test/proxy/ERC1967/ERC1967Utils.test.js diff --git a/.changeset/empty-taxis-kiss.md b/.changeset/empty-taxis-kiss.md new file mode 100644 index 00000000000..b01c92bd0e5 --- /dev/null +++ b/.changeset/empty-taxis-kiss.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`UUPSUpgradeable`, `TransparentUpgradeableProxy` and `ProxyAdmin`: Removed `upgradeTo` and `upgrade` functions, and made `upgradeToAndCall` and `upgradeAndCall` ignore the data argument if it is empty. It is no longer possible to invoke the receive function (or send value with empty data) along with an upgrade. diff --git a/.changeset/tender-shirts-turn.md b/.changeset/tender-shirts-turn.md new file mode 100644 index 00000000000..9c98e6e2b7e --- /dev/null +++ b/.changeset/tender-shirts-turn.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`BeaconProxy`: Reject value in initialization unless a payable function is explicitly invoked. diff --git a/contracts/mocks/UpgreadeableBeaconMock.sol b/contracts/mocks/UpgreadeableBeaconMock.sol new file mode 100644 index 00000000000..02c138d4de3 --- /dev/null +++ b/contracts/mocks/UpgreadeableBeaconMock.sol @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +import {IBeacon} from "../proxy/beacon/IBeacon.sol"; + +contract UpgradeableBeaconMock is IBeacon { + address public implementation; + + constructor(address impl) { + implementation = impl; + } +} diff --git a/contracts/mocks/proxy/ClashingImplementation.sol b/contracts/mocks/proxy/ClashingImplementation.sol index 89904b91f16..4cb5d232624 100644 --- a/contracts/mocks/proxy/ClashingImplementation.sol +++ b/contracts/mocks/proxy/ClashingImplementation.sol @@ -9,7 +9,7 @@ pragma solidity ^0.8.19; contract ClashingImplementation { event ClashingImplementationCall(); - function upgradeTo(address) external payable { + function upgradeToAndCall(address, bytes calldata) external payable { emit ClashingImplementationCall(); } diff --git a/contracts/mocks/proxy/UUPSUpgradeableMock.sol b/contracts/mocks/proxy/UUPSUpgradeableMock.sol index 4333e63e8fe..769c899462b 100644 --- a/contracts/mocks/proxy/UUPSUpgradeableMock.sol +++ b/contracts/mocks/proxy/UUPSUpgradeableMock.sol @@ -23,12 +23,8 @@ contract UUPSUpgradeableMock is NonUpgradeableMock, UUPSUpgradeable { } contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock { - function upgradeTo(address newImplementation) public override { - ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false); - } - function upgradeToAndCall(address newImplementation, bytes memory data) public payable override { - ERC1967Utils.upgradeToAndCall(newImplementation, data, false); + ERC1967Utils.upgradeToAndCall(newImplementation, data); } } diff --git a/contracts/proxy/ERC1967/ERC1967Proxy.sol b/contracts/proxy/ERC1967/ERC1967Proxy.sol index e7c90c70691..d2a927d58c3 100644 --- a/contracts/proxy/ERC1967/ERC1967Proxy.sol +++ b/contracts/proxy/ERC1967/ERC1967Proxy.sol @@ -18,9 +18,13 @@ contract ERC1967Proxy is Proxy { * * If `_data` is nonempty, it's used as data in a delegate call to `_logic`. This will typically be an encoded * function call, and allows initializing the storage of the proxy like a Solidity constructor. + * + * Requirements: + * + * - If `data` is empty, `msg.value` must be zero. */ constructor(address _logic, bytes memory _data) payable { - ERC1967Utils.upgradeToAndCall(_logic, _data, false); + ERC1967Utils.upgradeToAndCall(_logic, _data); } /** diff --git a/contracts/proxy/ERC1967/ERC1967Utils.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol index 3ad93ff3c01..bd2108096da 100644 --- a/contracts/proxy/ERC1967/ERC1967Utils.sol +++ b/contracts/proxy/ERC1967/ERC1967Utils.sol @@ -52,6 +52,11 @@ library ERC1967Utils { */ error ERC1967InvalidBeacon(address beacon); + /** + * @dev An upgrade function sees `msg.value > 0` that may be lost. + */ + error ERC1967NonPayable(); + /** * @dev Returns the current implementation address. */ @@ -70,24 +75,20 @@ library ERC1967Utils { } /** - * @dev Perform implementation upgrade + * @dev Performs implementation upgrade with additional setup call if data is nonempty. + * This function is payable only if the setup call is performed, otherwise `msg.value` is rejected + * to avoid stuck value in the contract. * * Emits an {IERC1967-Upgraded} event. */ - function upgradeTo(address newImplementation) internal { + function upgradeToAndCall(address newImplementation, bytes memory data) internal { _setImplementation(newImplementation); emit Upgraded(newImplementation); - } - /** - * @dev Perform implementation upgrade with additional setup call. - * - * Emits an {IERC1967-Upgraded} event. - */ - function upgradeToAndCall(address newImplementation, bytes memory data, bool forceCall) internal { - upgradeTo(newImplementation); - if (data.length > 0 || forceCall) { + if (data.length > 0) { Address.functionDelegateCall(newImplementation, data); + } else { + _checkNonPayable(); } } @@ -161,7 +162,9 @@ library ERC1967Utils { } /** - * @dev Change the beacon and trigger a setup call. + * @dev Change the beacon and trigger a setup call if data is nonempty. + * This function is payable only if the setup call is performed, otherwise `msg.value` is rejected + * to avoid stuck value in the contract. * * Emits an {IERC1967-BeaconUpgraded} event. * @@ -169,11 +172,23 @@ library ERC1967Utils { * it uses an immutable beacon without looking at the value of the ERC-1967 beacon slot for * efficiency. */ - function upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal { + function upgradeBeaconToAndCall(address newBeacon, bytes memory data) internal { _setBeacon(newBeacon); emit BeaconUpgraded(newBeacon); - if (data.length > 0 || forceCall) { + + if (data.length > 0) { Address.functionDelegateCall(IBeacon(newBeacon).implementation(), data); + } else { + _checkNonPayable(); + } + } + + /** + * @dev Reverts if `msg.value` is not zero. + */ + function _checkNonPayable() private { + if (msg.value > 0) { + revert ERC1967NonPayable(); } } } diff --git a/contracts/proxy/beacon/BeaconProxy.sol b/contracts/proxy/beacon/BeaconProxy.sol index 14a8d1b661c..b519b9cafcb 100644 --- a/contracts/proxy/beacon/BeaconProxy.sol +++ b/contracts/proxy/beacon/BeaconProxy.sol @@ -34,9 +34,10 @@ contract BeaconProxy is Proxy { * Requirements: * * - `beacon` must be a contract with the interface {IBeacon}. + * - If `data` is empty, `msg.value` must be zero. */ constructor(address beacon, bytes memory data) payable { - ERC1967Utils.upgradeBeaconToAndCall(beacon, data, false); + ERC1967Utils.upgradeBeaconToAndCall(beacon, data); _beacon = beacon; } diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 12d80a9c4ee..fd4a82d1292 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -12,28 +12,28 @@ import {Ownable} from "../../access/Ownable.sol"; */ contract ProxyAdmin is Ownable { /** - * @dev Sets the initial owner who can perform upgrades. + * @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgrade(address)` + * and `upgradeAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, + * while `upgradeAndCall` will invoke the `receive` function if the second argument is the empty byte string. + * If the getter returns `"5.0.0"`, only `upgradeAndCall(address,bytes)` is present, and the second argument must + * be the empty byte string if no function should be called, being impossible to invoke the `receive` function + * during an upgrade. */ - constructor(address initialOwner) Ownable(initialOwner) {} + string public constant UPGRADE_INTERFACE_VERSION = "5.0.0"; /** - * @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}. - * - * Requirements: - * - * - This contract must be the admin of `proxy`. + * @dev Sets the initial owner who can perform upgrades. */ - function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner { - proxy.upgradeTo(implementation); - } + constructor(address initialOwner) Ownable(initialOwner) {} /** - * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. See - * {TransparentUpgradeableProxy-upgradeToAndCall}. + * @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. + * See {TransparentUpgradeableProxy-_dispatchUpgradeToAndCall}. * * Requirements: * * - This contract must be the admin of `proxy`. + * - If `data` is empty, `msg.value` must be zero. */ function upgradeAndCall( ITransparentUpgradeableProxy proxy, diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 3d6e11f38a3..fc357464236 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -6,21 +6,20 @@ pragma solidity ^0.8.19; import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; import {ERC1967Proxy} from "../ERC1967/ERC1967Proxy.sol"; import {IERC1967} from "../../interfaces/IERC1967.sol"; +import {ProxyAdmin} from "./ProxyAdmin.sol"; /** * @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy} - * does not implement this interface directly, and some of its functions are implemented by an internal dispatch + * does not implement this interface directly, and its upgradeability mechanism is implemented by an internal dispatch * mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not * include them in the ABI so this interface must be used to interact with it. */ interface ITransparentUpgradeableProxy is IERC1967 { - function upgradeTo(address) external; - - function upgradeToAndCall(address, bytes memory) external payable; + function upgradeToAndCall(address, bytes calldata) external payable; } /** - * @dev This contract implements a proxy that is upgradeable by an immutable admin. + * @dev This contract implements a proxy that is upgradeable through an associated {ProxyAdmin} instance. * * To avoid https://medium.com/nomic-labs-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357[proxy selector * clashing], which can potentially be used in an attack, this contract uses the @@ -28,23 +27,22 @@ interface ITransparentUpgradeableProxy is IERC1967 { * things that go hand in hand: * * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if - * that call matches one of the admin functions exposed by the proxy itself. - * 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the + * that call matches the {ITransparentUpgradeableProxy-upgradeToAndCall} function exposed by the proxy itself. + * 2. If the admin calls the proxy, it can call the `upgradeToAndCall` function but any other call won't be forwarded to the * implementation. If the admin tries to call a function on the implementation it will fail with an error indicating the * proxy admin cannot fallback to the target implementation. * * These properties mean that the admin account can only be used for upgrading the proxy, so it's best if it's a dedicated * account that is not used for anything else. This will avoid headaches due to sudden errors when trying to call a function - * from the proxy implementation. - * - * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way, - * you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy, which extends from the - * {Ownable} contract to allow for changing the proxy's admin owner. + * from the proxy implementation. For this reason, the proxy deploys an instance of {ProxyAdmin} and allows upgrades + * only if they come through it. + * You should think of the `ProxyAdmin` instance as the administrative interface of the proxy, including the ability to + * change who can trigger upgrades by transferring ownership. * * NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not - * inherit from that interface, and instead the admin functions are implicitly implemented using a custom dispatch - * mechanism in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to - * fully implement transparency without decoding reverts caused by selector clashes between the proxy and the + * inherit from that interface, and instead `upgradeToAndCall` is implicitly implemented using a custom dispatch mechanism + * in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to fully + * implement transparency without decoding reverts caused by selector clashes between the proxy and the * implementation. * * IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an immutable variable, @@ -55,10 +53,10 @@ interface ITransparentUpgradeableProxy is IERC1967 { * WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the compiler * will not check that there are no selector conflicts, due to the note above. A selector clash between any new function * and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could - * render the admin operations inaccessible, which could prevent upgradeability. Transparency may also be compromised. + * render the `upgradeToAndCall` function inaccessible, preventing upgradeability and compromising transparency. */ contract TransparentUpgradeableProxy is ERC1967Proxy { - // An immutable address for the admin avoid unnecessary SLOADs before each call + // An immutable address for the admin to avoid unnecessary SLOADs before each call // at the expense of removing the ability to change the admin once it's set. // This is acceptable if the admin is always a ProxyAdmin instance or similar contract // with its own ability to transfer the permissions to another account. @@ -70,73 +68,40 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { error ProxyDeniedAdminAccess(); /** - * @dev msg.value is not 0. - */ - error ProxyNonPayableFunction(); - - /** - * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and - * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}. + * @dev Initializes an upgradeable proxy managed by an instance of a {ProxyAdmin} with an `initialOwner`, + * backed by the implementation at `_logic`, and optionally initialized with `_data` as explained in + * {ERC1967Proxy-constructor}. */ - constructor(address _logic, address admin_, bytes memory _data) payable ERC1967Proxy(_logic, _data) { - _admin = admin_; + constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) { + _admin = address(new ProxyAdmin(initialOwner)); // Set the storage value and emit an event for ERC-1967 compatibility - ERC1967Utils.changeAdmin(admin_); + ERC1967Utils.changeAdmin(_admin); } /** - * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior + * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior. */ function _fallback() internal virtual override { if (msg.sender == _admin) { - bytes memory ret; - bytes4 selector = msg.sig; - if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { - ret = _dispatchUpgradeTo(); - } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - ret = _dispatchUpgradeToAndCall(); + if (msg.sig == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { + _dispatchUpgradeToAndCall(); } else { revert ProxyDeniedAdminAccess(); } - assembly { - return(add(ret, 0x20), mload(ret)) - } } else { super._fallback(); } } /** - * @dev Upgrade the implementation of the proxy. - */ - function _dispatchUpgradeTo() private returns (bytes memory) { - _requireZeroValue(); - - address newImplementation = abi.decode(msg.data[4:], (address)); - ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false); - - return ""; - } - - /** - * @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified - * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the - * proxied contract. + * @dev Upgrade the implementation of the proxy. See {ERC1967Utils-upgradeToAndCall}. + * + * Requirements: + * + * - If `data` is empty, `msg.value` must be zero. */ - function _dispatchUpgradeToAndCall() private returns (bytes memory) { + function _dispatchUpgradeToAndCall() private { (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); - ERC1967Utils.upgradeToAndCall(newImplementation, data, true); - - return ""; - } - - /** - * @dev To keep this contract fully transparent, the fallback is payable. This helper is here to enforce - * non-payability of function implemented through dispatchers while still allowing value to pass through. - */ - function _requireZeroValue() private { - if (msg.value != 0) { - revert ProxyNonPayableFunction(); - } + ERC1967Utils.upgradeToAndCall(newImplementation, data); } } diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index 286eafe8e96..982e1e58f98 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -20,6 +20,16 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable address private immutable __self = address(this); + /** + * @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgradeTo(address)` + * and `upgradeToAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, + * while `upgradeToAndCall` will invoke the `receive` function if the second argument is the empty byte string. + * If the getter returns `"5.0.0"`, only `upgradeToAndCall(address,bytes)` is present, and the second argument must + * be the empty byte string if no function should be called, being impossible to invoke the `receive` function + * during an upgrade. + */ + string public constant UPGRADE_INTERFACE_VERSION = "5.0.0"; + /** * @dev The call is from an unauthorized context. */ @@ -71,20 +81,6 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { return ERC1967Utils.IMPLEMENTATION_SLOT; } - /** - * @dev Upgrade the implementation of the proxy to `newImplementation`. - * - * Calls {_authorizeUpgrade}. - * - * Emits an {Upgraded} event. - * - * @custom:oz-upgrades-unsafe-allow-reachable delegatecall - */ - function upgradeTo(address newImplementation) public virtual onlyProxy { - _authorizeUpgrade(newImplementation); - _upgradeToAndCallUUPS(newImplementation, new bytes(0), false); - } - /** * @dev Upgrade the implementation of the proxy to `newImplementation`, and subsequently execute the function call * encoded in `data`. @@ -97,17 +93,17 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { */ function upgradeToAndCall(address newImplementation, bytes memory data) public payable virtual onlyProxy { _authorizeUpgrade(newImplementation); - _upgradeToAndCallUUPS(newImplementation, data, true); + _upgradeToAndCallUUPS(newImplementation, data); } /** * @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract. Called by - * {upgradeTo} and {upgradeToAndCall}. + * {upgradeToAndCall}. * * Normally, this function will use an xref:access.adoc[access control] modifier such as {Ownable-onlyOwner}. * * ```solidity - * function _authorizeUpgrade(address) internal onlyOwner {} + * function _authorizeUpgrade(address) internal onlyOwner {} * ``` */ function _authorizeUpgrade(address newImplementation) internal virtual; @@ -117,12 +113,12 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { * * Emits an {IERC1967-Upgraded} event. */ - function _upgradeToAndCallUUPS(address newImplementation, bytes memory data, bool forceCall) private { + function _upgradeToAndCallUUPS(address newImplementation, bytes memory data) private { try IERC1822Proxiable(newImplementation).proxiableUUID() returns (bytes32 slot) { if (slot != ERC1967Utils.IMPLEMENTATION_SLOT) { revert UUPSUnsupportedProxiableUUID(slot); } - ERC1967Utils.upgradeToAndCall(newImplementation, data, forceCall); + ERC1967Utils.upgradeToAndCall(newImplementation, data); } catch { // The implementation is not UUPS revert ERC1967Utils.ERC1967InvalidImplementation(newImplementation); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 4182dfb4eb2..26255342f6c 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -1,6 +1,6 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const RLP = require('rlp'); +const { computeCreateAddress } = require('../../helpers/create'); const Enums = require('../../helpers/enums'); const { GovernorHelper } = require('../../helpers/governance'); const { clockFromReceipt } = require('../../helpers/time'); @@ -12,15 +12,6 @@ const CallReceiver = artifacts.require('CallReceiverMock'); const { shouldBehaveLikeEIP6372 } = require('../utils/EIP6372.behavior'); -function makeContractAddress(creator, nonce) { - return web3.utils.toChecksumAddress( - web3.utils - .sha3(RLP.encode([creator, nonce])) - .slice(12) - .substring(14), - ); -} - const TOKENS = [ { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, @@ -58,7 +49,7 @@ contract('GovernorCompatibilityBravo', function (accounts) { // Need to predict governance address to set it as timelock admin with a delayed transfer const nonce = await web3.eth.getTransactionCount(deployer); - const predictGovernor = makeContractAddress(deployer, nonce + 1); + const predictGovernor = computeCreateAddress(deployer, nonce + 1); this.timelock = await Timelock.new(predictGovernor, 2 * 86400); this.mock = await Governor.new( diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index c406baf8dde..7a2cb0abd8e 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -1,10 +1,10 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const RLP = require('rlp'); const Enums = require('../../helpers/enums'); const { GovernorHelper, proposalStatesToBitMap } = require('../../helpers/governance'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { computeCreateAddress } = require('../../helpers/create'); const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior'); @@ -14,15 +14,6 @@ const CallReceiver = artifacts.require('CallReceiverMock'); const ERC721 = artifacts.require('$ERC721'); const ERC1155 = artifacts.require('$ERC1155'); -function makeContractAddress(creator, nonce) { - return web3.utils.toChecksumAddress( - web3.utils - .sha3(RLP.encode([creator, nonce])) - .slice(12) - .substring(14), - ); -} - const TOKENS = [ { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, @@ -49,7 +40,7 @@ contract('GovernorTimelockCompound', function (accounts) { // Need to predict governance address to set it as timelock admin with a delayed transfer const nonce = await web3.eth.getTransactionCount(deployer); - const predictGovernor = makeContractAddress(deployer, nonce + 1); + const predictGovernor = computeCreateAddress(deployer, nonce + 1); this.timelock = await Timelock.new(predictGovernor, 2 * 86400); this.mock = await Governor.new( diff --git a/test/helpers/account.js b/test/helpers/account.js new file mode 100644 index 00000000000..1b01a721455 --- /dev/null +++ b/test/helpers/account.js @@ -0,0 +1,14 @@ +const { web3 } = require('hardhat'); +const { impersonateAccount, setBalance } = require('@nomicfoundation/hardhat-network-helpers'); + +// Hardhat default balance +const DEFAULT_BALANCE = web3.utils.toBN('10000000000000000000000'); + +async function impersonate(account, balance = DEFAULT_BALANCE) { + await impersonateAccount(account); + await setBalance(account, balance); +} + +module.exports = { + impersonate, +}; diff --git a/test/helpers/create.js b/test/helpers/create.js new file mode 100644 index 00000000000..e0cea66e2df --- /dev/null +++ b/test/helpers/create.js @@ -0,0 +1,23 @@ +const { rlp } = require('ethereumjs-util'); + +function computeCreateAddress(deployer, nonce) { + return web3.utils.toChecksumAddress(web3.utils.sha3(rlp.encode([deployer.address ?? deployer, nonce])).slice(-40)); +} + +function computeCreate2Address(saltHex, bytecode, deployer) { + return web3.utils.toChecksumAddress( + web3.utils + .sha3( + '0x' + + ['ff', deployer.address ?? deployer, saltHex, web3.utils.soliditySha3(bytecode)] + .map(x => x.replace(/0x/, '')) + .join(''), + ) + .slice(-40), + ); +} + +module.exports = { + computeCreateAddress, + computeCreate2Address, +}; diff --git a/test/helpers/create2.js b/test/helpers/create2.js deleted file mode 100644 index afe07dae385..00000000000 --- a/test/helpers/create2.js +++ /dev/null @@ -1,11 +0,0 @@ -function computeCreate2Address(saltHex, bytecode, deployer) { - return web3.utils.toChecksumAddress( - `0x${web3.utils - .sha3(`0x${['ff', deployer, saltHex, web3.utils.soliditySha3(bytecode)].map(x => x.replace(/0x/, '')).join('')}`) - .slice(-40)}`, - ); -} - -module.exports = { - computeCreate2Address, -}; diff --git a/test/helpers/erc1967.js b/test/helpers/erc1967.js index ac263d0c4e5..4ad92c55c99 100644 --- a/test/helpers/erc1967.js +++ b/test/helpers/erc1967.js @@ -1,3 +1,5 @@ +const { getStorageAt, setStorageAt } = require('@nomicfoundation/hardhat-network-helpers'); + const ImplementationLabel = 'eip1967.proxy.implementation'; const AdminLabel = 'eip1967.proxy.admin'; const BeaconLabel = 'eip1967.proxy.beacon'; @@ -7,15 +9,25 @@ function labelToSlot(label) { } function getSlot(address, slot) { - return web3.eth.getStorageAt( + return getStorageAt( + web3.utils.isAddress(address) ? address : address.address, + web3.utils.isHex(slot) ? slot : labelToSlot(slot), + ); +} + +function setSlot(address, slot, value) { + const hexValue = web3.utils.isHex(value) ? value : web3.utils.toHex(value); + + return setStorageAt( web3.utils.isAddress(address) ? address : address.address, web3.utils.isHex(slot) ? slot : labelToSlot(slot), + web3.utils.padLeft(hexValue, 64), ); } async function getAddressInSlot(address, slot) { const slotValue = await getSlot(address, slot); - return web3.utils.toChecksumAddress(slotValue.substr(-40)); + return web3.utils.toChecksumAddress(slotValue.substring(slotValue.length - 40)); } module.exports = { @@ -25,6 +37,7 @@ module.exports = { ImplementationSlot: labelToSlot(ImplementationLabel), AdminSlot: labelToSlot(AdminLabel), BeaconSlot: labelToSlot(BeaconLabel), + setSlot, getSlot, getAddressInSlot, }; diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 2edd1999c87..14a70e20bac 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -1,5 +1,5 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); -const { computeCreate2Address } = require('../helpers/create2'); +const { computeCreate2Address } = require('../helpers/create'); const { expect } = require('chai'); const { expectRevertCustomError } = require('../helpers/customError'); diff --git a/test/proxy/ERC1967/ERC1967Proxy.test.js b/test/proxy/ERC1967/ERC1967Proxy.test.js index 22df960ffbd..81cc4350749 100644 --- a/test/proxy/ERC1967/ERC1967Proxy.test.js +++ b/test/proxy/ERC1967/ERC1967Proxy.test.js @@ -3,11 +3,10 @@ const shouldBehaveLikeProxy = require('../Proxy.behaviour'); const ERC1967Proxy = artifacts.require('ERC1967Proxy'); contract('ERC1967Proxy', function (accounts) { - const [proxyAdminOwner] = accounts; - - const createProxy = async function (implementation, _admin, initData, opts) { - return ERC1967Proxy.new(implementation, initData, opts); + // `undefined`, `null` and other false-ish opts will not be forwarded. + const createProxy = async function (implementation, initData, opts) { + return ERC1967Proxy.new(implementation, initData, ...[opts].filter(Boolean)); }; - shouldBehaveLikeProxy(createProxy, undefined, proxyAdminOwner); + shouldBehaveLikeProxy(createProxy, accounts); }); diff --git a/test/proxy/ERC1967/ERC1967Utils.test.js b/test/proxy/ERC1967/ERC1967Utils.test.js new file mode 100644 index 00000000000..cce874cd93c --- /dev/null +++ b/test/proxy/ERC1967/ERC1967Utils.test.js @@ -0,0 +1,160 @@ +const { expectEvent, constants } = require('@openzeppelin/test-helpers'); +const { expectRevertCustomError } = require('../../helpers/customError'); +const { getAddressInSlot, setSlot, ImplementationSlot, AdminSlot, BeaconSlot } = require('../../helpers/erc1967'); + +const { ZERO_ADDRESS } = constants; + +const ERC1967Utils = artifacts.require('$ERC1967Utils'); + +const V1 = artifacts.require('DummyImplementation'); +const V2 = artifacts.require('CallReceiverMock'); +const UpgradeableBeaconMock = artifacts.require('UpgradeableBeaconMock'); + +contract('ERC1967Utils', function (accounts) { + const [, admin, anotherAccount] = accounts; + const EMPTY_DATA = '0x'; + + beforeEach('setup', async function () { + this.utils = await ERC1967Utils.new(); + this.v1 = await V1.new(); + this.v2 = await V2.new(); + }); + + describe('IMPLEMENTATION_SLOT', function () { + beforeEach('set v1 implementation', async function () { + await setSlot(this.utils, ImplementationSlot, this.v1.address); + }); + + describe('getImplementation', function () { + it('returns current implementation and matches implementation slot value', async function () { + expect(await this.utils.$getImplementation()).to.equal(this.v1.address); + expect(await getAddressInSlot(this.utils.address, ImplementationSlot)).to.equal(this.v1.address); + }); + }); + + describe('upgradeToAndCall', function () { + it('sets implementation in storage and emits event', async function () { + const newImplementation = this.v2.address; + const receipt = await this.utils.$upgradeToAndCall(newImplementation, EMPTY_DATA); + + expect(await getAddressInSlot(this.utils.address, ImplementationSlot)).to.equal(newImplementation); + expectEvent(receipt, 'Upgraded', { implementation: newImplementation }); + }); + + it('reverts when implementation does not contain code', async function () { + await expectRevertCustomError( + this.utils.$upgradeToAndCall(anotherAccount, EMPTY_DATA), + 'ERC1967InvalidImplementation', + [anotherAccount], + ); + }); + + describe('when data is empty', function () { + it('reverts when value is sent', async function () { + await expectRevertCustomError( + this.utils.$upgradeToAndCall(this.v2.address, EMPTY_DATA, { value: 1 }), + 'ERC1967NonPayable', + [], + ); + }); + }); + + describe('when data is not empty', function () { + it('delegates a call to the new implementation', async function () { + const initializeData = this.v2.contract.methods.mockFunction().encodeABI(); + const receipt = await this.utils.$upgradeToAndCall(this.v2.address, initializeData); + await expectEvent.inTransaction(receipt.tx, await V2.at(this.utils.address), 'MockFunctionCalled'); + }); + }); + }); + }); + + describe('ADMIN_SLOT', function () { + beforeEach('set admin', async function () { + await setSlot(this.utils, AdminSlot, admin); + }); + + describe('getAdmin', function () { + it('returns current admin and matches admin slot value', async function () { + expect(await this.utils.$getAdmin()).to.equal(admin); + expect(await getAddressInSlot(this.utils.address, AdminSlot)).to.equal(admin); + }); + }); + + describe('changeAdmin', function () { + it('sets admin in storage and emits event', async function () { + const newAdmin = anotherAccount; + const receipt = await this.utils.$changeAdmin(newAdmin); + + expect(await getAddressInSlot(this.utils.address, AdminSlot)).to.equal(newAdmin); + expectEvent(receipt, 'AdminChanged', { previousAdmin: admin, newAdmin: newAdmin }); + }); + + it('reverts when setting the address zero as admin', async function () { + await expectRevertCustomError(this.utils.$changeAdmin(ZERO_ADDRESS), 'ERC1967InvalidAdmin', [ZERO_ADDRESS]); + }); + }); + }); + + describe('BEACON_SLOT', function () { + beforeEach('set beacon', async function () { + this.beacon = await UpgradeableBeaconMock.new(this.v1.address); + await setSlot(this.utils, BeaconSlot, this.beacon.address); + }); + + describe('getBeacon', function () { + it('returns current beacon and matches beacon slot value', async function () { + expect(await this.utils.$getBeacon()).to.equal(this.beacon.address); + expect(await getAddressInSlot(this.utils.address, BeaconSlot)).to.equal(this.beacon.address); + }); + }); + + describe('upgradeBeaconToAndCall', function () { + it('sets beacon in storage and emits event', async function () { + const newBeacon = await UpgradeableBeaconMock.new(this.v2.address); + const receipt = await this.utils.$upgradeBeaconToAndCall(newBeacon.address, EMPTY_DATA); + + expect(await getAddressInSlot(this.utils.address, BeaconSlot)).to.equal(newBeacon.address); + expectEvent(receipt, 'BeaconUpgraded', { beacon: newBeacon.address }); + }); + + it('reverts when beacon does not contain code', async function () { + await expectRevertCustomError( + this.utils.$upgradeBeaconToAndCall(anotherAccount, EMPTY_DATA), + 'ERC1967InvalidBeacon', + [anotherAccount], + ); + }); + + it("reverts when beacon's implementation does not contain code", async function () { + const newBeacon = await UpgradeableBeaconMock.new(anotherAccount); + + await expectRevertCustomError( + this.utils.$upgradeBeaconToAndCall(newBeacon.address, EMPTY_DATA), + 'ERC1967InvalidImplementation', + [anotherAccount], + ); + }); + + describe('when data is empty', function () { + it('reverts when value is sent', async function () { + const newBeacon = await UpgradeableBeaconMock.new(this.v2.address); + await expectRevertCustomError( + this.utils.$upgradeBeaconToAndCall(newBeacon.address, EMPTY_DATA, { value: 1 }), + 'ERC1967NonPayable', + [], + ); + }); + }); + + describe('when data is not empty', function () { + it('delegates a call to the new implementation', async function () { + const initializeData = this.v2.contract.methods.mockFunction().encodeABI(); + const newBeacon = await UpgradeableBeaconMock.new(this.v2.address); + const receipt = await this.utils.$upgradeBeaconToAndCall(newBeacon.address, initializeData); + await expectEvent.inTransaction(receipt.tx, await V2.at(this.utils.address), 'MockFunctionCalled'); + }); + }); + }); + }); +}); diff --git a/test/proxy/Proxy.behaviour.js b/test/proxy/Proxy.behaviour.js index 0867b93c9c9..acce6d188d9 100644 --- a/test/proxy/Proxy.behaviour.js +++ b/test/proxy/Proxy.behaviour.js @@ -2,18 +2,15 @@ const { expectRevert } = require('@openzeppelin/test-helpers'); const { getSlot, ImplementationSlot } = require('../helpers/erc1967'); const { expect } = require('chai'); +const { expectRevertCustomError } = require('../helpers/customError'); const DummyImplementation = artifacts.require('DummyImplementation'); -module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyCreator) { +module.exports = function shouldBehaveLikeProxy(createProxy, accounts) { it('cannot be initialized with a non-contract address', async function () { - const nonContractAddress = proxyCreator; + const nonContractAddress = accounts[0]; const initializeData = Buffer.from(''); - await expectRevert.unspecified( - createProxy(nonContractAddress, proxyAdminAddress, initializeData, { - from: proxyCreator, - }), - ); + await expectRevert.unspecified(createProxy(nonContractAddress, initializeData)); }); before('deploy implementation', async function () { @@ -42,11 +39,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData)).address; }); assertProxyInitialization({ value: 0, balance: 0 }); @@ -55,16 +48,13 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when sending some balance', function () { const value = 10e5; - beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - value, - }) - ).address; + it('reverts', async function () { + await expectRevertCustomError( + createProxy(this.implementation, initializeData, { value }), + 'ERC1967NonPayable', + [], + ); }); - - assertProxyInitialization({ value: 0, balance: value }); }); }); @@ -75,11 +65,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData)).address; }); assertProxyInitialization({ @@ -92,9 +78,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, const value = 10e5; it('reverts', async function () { - await expectRevert.unspecified( - createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator, value }), - ); + await expectRevert.unspecified(createProxy(this.implementation, initializeData, { value })); }); }); }); @@ -105,11 +89,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData)).address; }); assertProxyInitialization({ @@ -122,12 +102,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, const value = 10e5; beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - value, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData, { value })).address; }); assertProxyInitialization({ @@ -147,11 +122,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData)).address; }); assertProxyInitialization({ @@ -164,9 +135,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, const value = 10e5; it('reverts', async function () { - await expectRevert.unspecified( - createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator, value }), - ); + await expectRevert.unspecified(createProxy(this.implementation, initializeData, { value })); }); }); }); @@ -179,11 +148,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, describe('when not sending balance', function () { beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData)).address; }); assertProxyInitialization({ @@ -196,12 +161,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, const value = 10e5; beforeEach('creating proxy', async function () { - this.proxy = ( - await createProxy(this.implementation, proxyAdminAddress, initializeData, { - from: proxyCreator, - value, - }) - ).address; + this.proxy = (await createProxy(this.implementation, initializeData, { value })).address; }); assertProxyInitialization({ @@ -215,10 +175,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, const initializeData = new DummyImplementation('').contract.methods.reverts().encodeABI(); it('reverts', async function () { - await expectRevert( - createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator }), - 'DummyImplementation reverted', - ); + await expectRevert(createProxy(this.implementation, initializeData), 'DummyImplementation reverted'); }); }); }); diff --git a/test/proxy/beacon/BeaconProxy.test.js b/test/proxy/beacon/BeaconProxy.test.js index 63d98239760..d583d0ffbf1 100644 --- a/test/proxy/beacon/BeaconProxy.test.js +++ b/test/proxy/beacon/BeaconProxy.test.js @@ -59,9 +59,8 @@ contract('BeaconProxy', function (accounts) { it('no initialization', async function () { const data = Buffer.from(''); - const balance = '10'; - this.proxy = await BeaconProxy.new(this.beacon.address, data, { value: balance }); - await this.assertInitialized({ value: '0', balance }); + this.proxy = await BeaconProxy.new(this.beacon.address, data); + await this.assertInitialized({ value: '0', balance: '0' }); }); it('non-payable initialization', async function () { @@ -79,7 +78,16 @@ contract('BeaconProxy', function (accounts) { await this.assertInitialized({ value, balance }); }); - it('reverting initialization', async function () { + it('reverting initialization due to value', async function () { + const data = Buffer.from(''); + await expectRevertCustomError( + BeaconProxy.new(this.beacon.address, data, { value: '1' }), + 'ERC1967NonPayable', + [], + ); + }); + + it('reverting initialization function', async function () { const data = this.implementationV0.contract.methods.reverts().encodeABI(); await expectRevert(BeaconProxy.new(this.beacon.address, data), 'DummyImplementation reverted'); }); diff --git a/test/proxy/transparent/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index 23f7ce9b286..4d1a54f6ab5 100644 --- a/test/proxy/transparent/ProxyAdmin.test.js +++ b/test/proxy/transparent/ProxyAdmin.test.js @@ -8,6 +8,7 @@ const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableP const { getAddressInSlot, ImplementationSlot } = require('../../helpers/erc1967'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { computeCreateAddress } = require('../../helpers/create'); contract('ProxyAdmin', function (accounts) { const [proxyAdminOwner, anotherAccount] = accounts; @@ -19,12 +20,12 @@ contract('ProxyAdmin', function (accounts) { beforeEach(async function () { const initializeData = Buffer.from(''); - this.proxyAdmin = await ProxyAdmin.new(proxyAdminOwner); - const proxy = await TransparentUpgradeableProxy.new( - this.implementationV1.address, - this.proxyAdmin.address, - initializeData, - ); + const proxy = await TransparentUpgradeableProxy.new(this.implementationV1.address, proxyAdminOwner, initializeData); + + const proxyNonce = await web3.eth.getTransactionCount(proxy.address); + const proxyAdminAddress = computeCreateAddress(proxy.address, proxyNonce - 1); // Nonce already used + this.proxyAdmin = await ProxyAdmin.at(proxyAdminAddress); + this.proxy = await ITransparentUpgradeableProxy.at(proxy.address); }); @@ -32,11 +33,17 @@ contract('ProxyAdmin', function (accounts) { expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner); }); - describe('#upgrade', function () { + it('has an interface version', async function () { + expect(await this.proxyAdmin.UPGRADE_INTERFACE_VERSION()).to.equal('5.0.0'); + }); + + describe('without data', function () { context('with unauthorized account', function () { it('fails to upgrade', async function () { await expectRevertCustomError( - this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: anotherAccount }), + this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, '0x', { + from: anotherAccount, + }), 'OwnableUnauthorizedAccount', [anotherAccount], ); @@ -45,7 +52,9 @@ contract('ProxyAdmin', function (accounts) { context('with authorized account', function () { it('upgrades implementation', async function () { - await this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: proxyAdminOwner }); + await this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, '0x', { + from: proxyAdminOwner, + }); const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); expect(implementationAddress).to.be.equal(this.implementationV2.address); @@ -53,7 +62,7 @@ contract('ProxyAdmin', function (accounts) { }); }); - describe('#upgradeAndCall', function () { + describe('with data', function () { context('with unauthorized account', function () { it('fails to upgrade', async function () { const callData = new ImplV1('').contract.methods.initializeNonPayableWithValue(1337).encodeABI(); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 1a03b84db5c..103af7fc3a9 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -5,6 +5,8 @@ const { expectRevertCustomError } = require('../../helpers/customError'); const { expect } = require('chai'); const { web3 } = require('hardhat'); +const { computeCreateAddress } = require('../../helpers/create'); +const { impersonate } = require('../../helpers/account'); const Implementation1 = artifacts.require('Implementation1'); const Implementation2 = artifacts.require('Implementation2'); @@ -16,9 +18,23 @@ const MigratableMockV3 = artifacts.require('MigratableMockV3'); const InitializableMock = artifacts.require('InitializableMock'); const DummyImplementation = artifacts.require('DummyImplementation'); const ClashingImplementation = artifacts.require('ClashingImplementation'); +const Ownable = artifacts.require('Ownable'); -module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProxy, accounts) { - const [proxyAdminAddress, proxyAdminOwner, anotherAccount] = accounts; +module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProxy, initialOwner, accounts) { + const [anotherAccount] = accounts; + + async function createProxyWithImpersonatedProxyAdmin(logic, initData, opts = undefined) { + const proxy = await createProxy(logic, initData, opts); + + // Expect proxy admin to be the first and only contract created by the proxy + const proxyAdminAddress = computeCreateAddress(proxy.address, 1); + await impersonate(proxyAdminAddress); + + return { + proxy, + proxyAdminAddress, + }; + } before(async function () { this.implementationV0 = (await DummyImplementation.new()).address; @@ -27,10 +43,12 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { const initializeData = Buffer.from(''); - this.proxy = await createProxy(this.implementationV0, proxyAdminAddress, initializeData, { - from: proxyAdminOwner, - }); - this.proxyAddress = this.proxy.address; + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + this.implementationV0, + initializeData, + ); + this.proxy = proxy; + this.proxyAdminAddress = proxyAdminAddress; }); describe('implementation', function () { @@ -40,7 +58,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('delegates to the implementation', async function () { - const dummy = new DummyImplementation(this.proxyAddress); + const dummy = new DummyImplementation(this.proxy.address); const value = await dummy.get(); expect(value).to.equal(true); @@ -49,64 +67,33 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('proxy admin', function () { it('emits AdminChanged event during construction', async function () { - expectEvent.inConstruction(this.proxy, 'AdminChanged', { + await expectEvent.inConstruction(this.proxy, 'AdminChanged', { previousAdmin: ZERO_ADDRESS, - newAdmin: proxyAdminAddress, + newAdmin: this.proxyAdminAddress, }); }); - it('sets the admin in the storage', async function () { - expect(await getAddressInSlot(this.proxy, AdminSlot)).to.be.equal(proxyAdminAddress); + it('sets the proxy admin in storage with the correct initial owner', async function () { + expect(await getAddressInSlot(this.proxy, AdminSlot)).to.be.equal(this.proxyAdminAddress); + const proxyAdmin = await Ownable.at(this.proxyAdminAddress); + expect(await proxyAdmin.owner()).to.be.equal(initialOwner); }); it('can overwrite the admin by the implementation', async function () { - const dummy = new DummyImplementation(this.proxyAddress); + const dummy = new DummyImplementation(this.proxy.address); await dummy.unsafeOverrideAdmin(anotherAccount); const ERC1967AdminSlotValue = await getAddressInSlot(this.proxy, AdminSlot); expect(ERC1967AdminSlotValue).to.be.equal(anotherAccount); // Still allows previous admin to execute admin operations - expect(ERC1967AdminSlotValue).to.not.equal(proxyAdminAddress); - expectEvent(await this.proxy.upgradeTo(this.implementationV1, { from: proxyAdminAddress }), 'Upgraded', { - implementation: this.implementationV1, - }); - }); - }); - - describe('upgradeTo', function () { - describe('when the sender is the admin', function () { - const from = proxyAdminAddress; - - describe('when the given implementation is different from the current one', function () { - it('upgrades to the requested implementation', async function () { - await this.proxy.upgradeTo(this.implementationV1, { from }); - - const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot); - expect(implementationAddress).to.be.equal(this.implementationV1); - }); - - it('emits an event', async function () { - expectEvent(await this.proxy.upgradeTo(this.implementationV1, { from }), 'Upgraded', { - implementation: this.implementationV1, - }); - }); - }); - - describe('when the given implementation is the zero address', function () { - it('reverts', async function () { - await expectRevertCustomError(this.proxy.upgradeTo(ZERO_ADDRESS, { from }), 'ERC1967InvalidImplementation', [ - ZERO_ADDRESS, - ]); - }); - }); - }); - - describe('when the sender is not the admin', function () { - const from = anotherAccount; - - it('reverts', async function () { - await expectRevert.unspecified(this.proxy.upgradeTo(this.implementationV1, { from })); - }); + expect(ERC1967AdminSlotValue).to.not.equal(this.proxyAdminAddress); + expectEvent( + await this.proxy.upgradeToAndCall(this.implementationV1, '0x', { from: this.proxyAdminAddress }), + 'Upgraded', + { + implementation: this.implementationV1, + }, + ); }); }); @@ -120,11 +107,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const initializeData = new InitializableMock('').contract.methods['initializeWithX(uint256)'](42).encodeABI(); describe('when the sender is the admin', function () { - const from = proxyAdminAddress; const value = 1e5; beforeEach(async function () { - this.receipt = await this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from, value }); + this.receipt = await this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { + from: this.proxyAdminAddress, + value, + }); }); it('upgrades to the requested implementation', async function () { @@ -137,13 +126,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('calls the initializer function', async function () { - const migratable = new InitializableMock(this.proxyAddress); + const migratable = new InitializableMock(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('42'); }); it('sends given value to the proxy', async function () { - const balance = await web3.eth.getBalance(this.proxyAddress); + const balance = await web3.eth.getBalance(this.proxy.address); expect(balance.toString()).to.be.bignumber.equal(value.toString()); }); @@ -151,7 +140,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx // storage layout should look as follows: // - 0: Initializable storage ++ initializerRan ++ onlyInitializingRan // - 1: x - const storedValue = await web3.eth.getStorageAt(this.proxyAddress, 1); + const storedValue = await web3.eth.getStorageAt(this.proxy.address, 1); expect(parseInt(storedValue)).to.eq(42); }); }); @@ -170,7 +159,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('reverts', async function () { await expectRevert.unspecified( - this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from: proxyAdminAddress }), + this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from: this.proxyAdminAddress }), ); }); }); @@ -178,7 +167,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('with migrations', function () { describe('when the sender is the admin', function () { - const from = proxyAdminAddress; const value = 1e5; describe('when upgrading to V1', function () { @@ -186,8 +174,11 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { this.behaviorV1 = await MigratableMockV1.new(); - this.balancePreviousV1 = new BN(await web3.eth.getBalance(this.proxyAddress)); - this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV1.address, v1MigrationData, { from, value }); + this.balancePreviousV1 = new BN(await web3.eth.getBalance(this.proxy.address)); + this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV1.address, v1MigrationData, { + from: this.proxyAdminAddress, + value, + }); }); it('upgrades to the requested version and emits an event', async function () { @@ -197,12 +188,12 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it("calls the 'initialize' function and sends given value to the proxy", async function () { - const migratable = new MigratableMockV1(this.proxyAddress); + const migratable = new MigratableMockV1(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('42'); - const balance = await web3.eth.getBalance(this.proxyAddress); + const balance = await web3.eth.getBalance(this.proxy.address); expect(new BN(balance)).to.be.bignumber.equal(this.balancePreviousV1.addn(value)); }); @@ -211,9 +202,9 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { this.behaviorV2 = await MigratableMockV2.new(); - this.balancePreviousV2 = new BN(await web3.eth.getBalance(this.proxyAddress)); + this.balancePreviousV2 = new BN(await web3.eth.getBalance(this.proxy.address)); this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV2.address, v2MigrationData, { - from, + from: this.proxyAdminAddress, value, }); }); @@ -225,7 +216,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it("calls the 'migrate' function and sends given value to the proxy", async function () { - const migratable = new MigratableMockV2(this.proxyAddress); + const migratable = new MigratableMockV2(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('10'); @@ -233,7 +224,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const y = await migratable.y(); expect(y).to.be.bignumber.equal('42'); - const balance = new BN(await web3.eth.getBalance(this.proxyAddress)); + const balance = new BN(await web3.eth.getBalance(this.proxy.address)); expect(balance).to.be.bignumber.equal(this.balancePreviousV2.addn(value)); }); @@ -242,9 +233,9 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx beforeEach(async function () { this.behaviorV3 = await MigratableMockV3.new(); - this.balancePreviousV3 = new BN(await web3.eth.getBalance(this.proxyAddress)); + this.balancePreviousV3 = new BN(await web3.eth.getBalance(this.proxy.address)); this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV3.address, v3MigrationData, { - from, + from: this.proxyAdminAddress, value, }); }); @@ -256,7 +247,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it("calls the 'migrate' function and sends given value to the proxy", async function () { - const migratable = new MigratableMockV3(this.proxyAddress); + const migratable = new MigratableMockV3(this.proxy.address); const x = await migratable.x(); expect(x).to.be.bignumber.equal('42'); @@ -264,7 +255,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const y = await migratable.y(); expect(y).to.be.bignumber.equal('10'); - const balance = new BN(await web3.eth.getBalance(this.proxyAddress)); + const balance = new BN(await web3.eth.getBalance(this.proxy.address)); expect(balance).to.be.bignumber.equal(this.balancePreviousV3.addn(value)); }); }); @@ -289,15 +280,18 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx const initializeData = Buffer.from(''); this.clashingImplV0 = (await ClashingImplementation.new()).address; this.clashingImplV1 = (await ClashingImplementation.new()).address; - this.proxy = await createProxy(this.clashingImplV0, proxyAdminAddress, initializeData, { - from: proxyAdminOwner, - }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + this.clashingImplV0, + initializeData, + ); + this.proxy = proxy; + this.proxyAdminAddress = proxyAdminAddress; this.clashing = new ClashingImplementation(this.proxy.address); }); it('proxy admin cannot call delegated functions', async function () { await expectRevertCustomError( - this.clashing.delegatedFunction({ from: proxyAdminAddress }), + this.clashing.delegatedFunction({ from: this.proxyAdminAddress }), 'ProxyDeniedAdminAccess', [], ); @@ -305,26 +299,16 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('when function names clash', function () { it('executes the proxy function if the sender is the admin', async function () { - const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: proxyAdminAddress, value: 0 }); + const receipt = await this.proxy.upgradeToAndCall(this.clashingImplV1, '0x', { + from: this.proxyAdminAddress, + }); expectEvent(receipt, 'Upgraded', { implementation: this.clashingImplV1 }); }); it('delegates the call to implementation when sender is not the admin', async function () { - const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: anotherAccount, value: 0 }); - expectEvent.notEmitted(receipt, 'Upgraded'); - expectEvent.inTransaction(receipt.tx, this.clashing, 'ClashingImplementationCall'); - }); - - it('requires 0 value calling upgradeTo by proxy admin', async function () { - await expectRevertCustomError( - this.proxy.upgradeTo(this.clashingImplV1, { from: proxyAdminAddress, value: 1 }), - 'ProxyNonPayableFunction', - [], - ); - }); - - it('allows calling with value if sender is not the admin', async function () { - const receipt = await this.proxy.upgradeTo(this.clashingImplV1, { from: anotherAccount, value: 1 }); + const receipt = await this.proxy.upgradeToAndCall(this.clashingImplV1, '0x', { + from: anotherAccount, + }); expectEvent.notEmitted(receipt, 'Upgraded'); expectEvent.inTransaction(receipt.tx, this.clashing, 'ClashingImplementationCall'); }); @@ -336,13 +320,16 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should add new function', async () => { const instance1 = await Implementation1.new(); - const proxy = await createProxy(instance1.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance1.address, + initializeData, + ); const proxyInstance1 = new Implementation1(proxy.address); await proxyInstance1.setValue(42); const instance2 = await Implementation2.new(); - await proxy.upgradeTo(instance2.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance2.address, '0x', { from: proxyAdminAddress }); const proxyInstance2 = new Implementation2(proxy.address); const res = await proxyInstance2.getValue(); @@ -351,7 +338,10 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should remove function', async () => { const instance2 = await Implementation2.new(); - const proxy = await createProxy(instance2.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance2.address, + initializeData, + ); const proxyInstance2 = new Implementation2(proxy.address); await proxyInstance2.setValue(42); @@ -359,7 +349,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx expect(res.toString()).to.eq('42'); const instance1 = await Implementation1.new(); - await proxy.upgradeTo(instance1.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance1.address, '0x', { from: proxyAdminAddress }); const proxyInstance1 = new Implementation2(proxy.address); await expectRevert.unspecified(proxyInstance1.getValue()); @@ -367,13 +357,16 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should change function signature', async () => { const instance1 = await Implementation1.new(); - const proxy = await createProxy(instance1.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance1.address, + initializeData, + ); const proxyInstance1 = new Implementation1(proxy.address); await proxyInstance1.setValue(42); const instance3 = await Implementation3.new(); - await proxy.upgradeTo(instance3.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance3.address, '0x', { from: proxyAdminAddress }); const proxyInstance3 = new Implementation3(proxy.address); const res = await proxyInstance3.getValue(8); @@ -383,10 +376,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should add fallback function', async () => { const initializeData = Buffer.from(''); const instance1 = await Implementation1.new(); - const proxy = await createProxy(instance1.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance1.address, + initializeData, + ); const instance4 = await Implementation4.new(); - await proxy.upgradeTo(instance4.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance4.address, '0x', { from: proxyAdminAddress }); const proxyInstance4 = new Implementation4(proxy.address); const data = '0x'; @@ -398,10 +394,13 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('should remove fallback function', async () => { const instance4 = await Implementation4.new(); - const proxy = await createProxy(instance4.address, proxyAdminAddress, initializeData, { from: proxyAdminOwner }); + const { proxy, proxyAdminAddress } = await createProxyWithImpersonatedProxyAdmin( + instance4.address, + initializeData, + ); const instance2 = await Implementation2.new(); - await proxy.upgradeTo(instance2.address, { from: proxyAdminAddress }); + await proxy.upgradeToAndCall(instance2.address, '0x', { from: proxyAdminAddress }); const data = '0x'; await expectRevert.unspecified(web3.eth.sendTransaction({ to: proxy.address, from: anotherAccount, data })); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.test.js b/test/proxy/transparent/TransparentUpgradeableProxy.test.js index d60a31a21da..f45e392f69d 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.test.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.test.js @@ -5,13 +5,20 @@ const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeablePro const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy'); contract('TransparentUpgradeableProxy', function (accounts) { - const [proxyAdminAddress, proxyAdminOwner] = accounts; + const [owner, ...otherAccounts] = accounts; - const createProxy = async function (logic, admin, initData, opts) { - const { address } = await TransparentUpgradeableProxy.new(logic, admin, initData, opts); - return ITransparentUpgradeableProxy.at(address); + // `undefined`, `null` and other false-ish opts will not be forwarded. + const createProxy = async function (logic, initData, opts = undefined) { + const { address, transactionHash } = await TransparentUpgradeableProxy.new( + logic, + owner, + initData, + ...[opts].filter(Boolean), + ); + const instance = await ITransparentUpgradeableProxy.at(address); + return { ...instance, transactionHash }; }; - shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyAdminOwner); - shouldBehaveLikeTransparentUpgradeableProxy(createProxy, accounts); + shouldBehaveLikeProxy(createProxy, otherAccounts); + shouldBehaveLikeTransparentUpgradeableProxy(createProxy, owner, otherAccounts); }); diff --git a/test/proxy/utils/UUPSUpgradeable.test.js b/test/proxy/utils/UUPSUpgradeable.test.js index 6a8104248fb..0baa9052049 100644 --- a/test/proxy/utils/UUPSUpgradeable.test.js +++ b/test/proxy/utils/UUPSUpgradeable.test.js @@ -25,8 +25,12 @@ contract('UUPSUpgradeable', function () { this.instance = await UUPSUpgradeableMock.at(address); }); + it('has an interface version', async function () { + expect(await this.instance.UPGRADE_INTERFACE_VERSION()).to.equal('5.0.0'); + }); + it('upgrade to upgradeable implementation', async function () { - const { receipt } = await this.instance.upgradeTo(this.implUpgradeOk.address); + const { receipt } = await this.instance.upgradeToAndCall(this.implUpgradeOk.address, '0x'); expect(receipt.logs.filter(({ event }) => event === 'Upgraded').length).to.be.equal(1); expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeOk.address }); expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeOk.address); @@ -48,7 +52,7 @@ contract('UUPSUpgradeable', function () { it('calling upgradeTo on the implementation reverts', async function () { await expectRevertCustomError( - this.implInitial.upgradeTo(this.implUpgradeOk.address), + this.implInitial.upgradeToAndCall(this.implUpgradeOk.address, '0x'), 'UUPSUnauthorizedCallContext', [], ); @@ -72,7 +76,7 @@ contract('UUPSUpgradeable', function () { ); await expectRevertCustomError( - instance.upgradeTo(this.implUpgradeUnsafe.address), + instance.upgradeToAndCall(this.implUpgradeUnsafe.address, '0x'), 'UUPSUnauthorizedCallContext', [], ); @@ -93,14 +97,14 @@ contract('UUPSUpgradeable', function () { it('rejects upgrading to an unsupported UUID', async function () { await expectRevertCustomError( - this.instance.upgradeTo(this.implUnsupportedUUID.address), + this.instance.upgradeToAndCall(this.implUnsupportedUUID.address, '0x'), 'UUPSUnsupportedProxiableUUID', [web3.utils.keccak256('invalid UUID')], ); }); it('upgrade to and unsafe upgradeable implementation', async function () { - const { receipt } = await this.instance.upgradeTo(this.implUpgradeUnsafe.address); + const { receipt } = await this.instance.upgradeToAndCall(this.implUpgradeUnsafe.address, '0x'); expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeUnsafe.address }); expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeUnsafe.address); }); @@ -108,7 +112,7 @@ contract('UUPSUpgradeable', function () { // delegate to a non existing upgradeTo function causes a low level revert it('reject upgrade to non uups implementation', async function () { await expectRevertCustomError( - this.instance.upgradeTo(this.implUpgradeNonUUPS.address), + this.instance.upgradeToAndCall(this.implUpgradeNonUUPS.address, '0x'), 'ERC1967InvalidImplementation', [this.implUpgradeNonUUPS.address], ); @@ -118,8 +122,10 @@ contract('UUPSUpgradeable', function () { const { address } = await ERC1967Proxy.new(this.implInitial.address, '0x'); const otherInstance = await UUPSUpgradeableMock.at(address); - await expectRevertCustomError(this.instance.upgradeTo(otherInstance.address), 'ERC1967InvalidImplementation', [ - otherInstance.address, - ]); + await expectRevertCustomError( + this.instance.upgradeToAndCall(otherInstance.address, '0x'), + 'ERC1967InvalidImplementation', + [otherInstance.address], + ); }); }); diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index f88d5504c36..afbcc3db958 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -1,5 +1,5 @@ const { balance, ether, expectEvent, expectRevert, send } = require('@openzeppelin/test-helpers'); -const { computeCreate2Address } = require('../helpers/create2'); +const { computeCreate2Address } = require('../helpers/create'); const { expect } = require('chai'); const { expectRevertCustomError } = require('../helpers/customError'); From 21bb89ef5bfc789b9333eb05e3ba2b7b284ac77c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 17 Jul 2023 21:26:31 +0200 Subject: [PATCH 22/31] Fix typo in MessageHashUtils.sol (#4462) --- contracts/utils/cryptography/MessageHashUtils.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/cryptography/MessageHashUtils.sol b/contracts/utils/cryptography/MessageHashUtils.sol index 05ce35af810..558e5e79337 100644 --- a/contracts/utils/cryptography/MessageHashUtils.sol +++ b/contracts/utils/cryptography/MessageHashUtils.sol @@ -21,7 +21,7 @@ library MessageHashUtils { * hash signed when using the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] JSON-RPC method. * * NOTE: The `hash` parameter is intended to be the result of hashing a raw message with - * keccak256, althoguh any bytes32 value can be safely used because the final digest will + * keccak256, although any bytes32 value can be safely used because the final digest will * be re-hashed. * * See {ECDSA-recover}. From f347b410cf6aeeaaf5197e1fece139c793c03b2b Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Tue, 18 Jul 2023 13:08:38 -0400 Subject: [PATCH 23/31] Update recommended Foundry remapping (#4468) Co-authored-by: ernestognw --- README.md | 2 +- scripts/upgradeable/upgradeable.patch | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 1befa024a56..38197f3af25 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/con $ forge install OpenZeppelin/openzeppelin-contracts ``` -Add `@openzeppelin/=lib/openzeppelin-contracts/` in `remappings.txt.` +Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` ### Usage diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch index e50d3a70e8e..ac9eca821aa 100644 --- a/scripts/upgradeable/upgradeable.patch +++ b/scripts/upgradeable/upgradeable.patch @@ -59,7 +59,7 @@ index ff596b0c..00000000 - - diff --git a/README.md b/README.md -index 27627f43..e42a66e8 100644 +index 38197f3a..bc934d1c 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,9 @@ @@ -81,7 +81,7 @@ index 27627f43..e42a66e8 100644 ``` OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/contracts/releases-stability#api-stability), which means that your contracts won't break unexpectedly when upgrading to a newer minor version. -@@ -38,7 +41,7 @@ OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/con +@@ -38,10 +41,10 @@ OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/con > **Warning** Foundry installs the latest version initially, but subsequent `forge update` commands will use the `master` branch. ``` @@ -89,8 +89,12 @@ index 27627f43..e42a66e8 100644 +$ forge install OpenZeppelin/openzeppelin-contracts-upgradeable ``` +-Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` ++Add `@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/` in `remappings.txt.` + ### Usage -@@ -48,10 +51,11 @@ Once installed, you can use the contracts in the library by importing them: + +@@ -50,10 +53,11 @@ Once installed, you can use the contracts in the library by importing them: ```solidity pragma solidity ^0.8.19; @@ -126,7 +130,7 @@ index df141192..1cf90ad1 100644 "keywords": [ "solidity", diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol -index 36f076e5..90c1db78 100644 +index 3800804a..90c1db78 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -4,7 +4,6 @@ @@ -293,7 +297,7 @@ index 36f076e5..90c1db78 100644 } } diff --git a/package.json b/package.json -index 9eae6732..b3a56366 100644 +index ffa868ac..e254d962 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ From 19293f3ecdb20a7f44d54279b5c1ddbb84de4a2e Mon Sep 17 00:00:00 2001 From: Prince Allwin Date: Tue, 25 Jul 2023 07:30:30 +0530 Subject: [PATCH 24/31] Remove outdated comments in AccessControl.sol (#4475) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/AccessControl.sol | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index d934eddafe9..99099dd1f43 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -58,11 +58,8 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { /** * @dev Modifier that checks that an account has a specific role. Reverts - * with a standardized message including the required role. + * with a custom error including the required role. * - * The format of the revert reason is given by the following regular expression: - * - * /^AccessControl: account (0x[0-9a-f]{40}) is missing role (0x[0-9a-f]{64})$/ */ modifier onlyRole(bytes32 role) { _checkRole(role); @@ -84,21 +81,15 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { } /** - * @dev Revert with a standard message if `_msgSender()` is missing `role`. + * @dev Revert with a custom error if `_msgSender()` is missing `role`. * Overriding this function changes the behavior of the {onlyRole} modifier. - * - * Format of the revert message is described in {_checkRole}. */ function _checkRole(bytes32 role) internal view virtual { _checkRole(role, _msgSender()); } /** - * @dev Revert with a standard message if `account` is missing `role`. - * - * The format of the revert reason is given by the following regular expression: - * - * /^AccessControl: account (0x[0-9a-f]{40}) is missing role (0x[0-9a-f]{64})$/ + * @dev Revert with a custom error if `account` is missing `role`. */ function _checkRole(bytes32 role, address account) internal view virtual { if (!hasRole(role, account)) { From 28d9ac2bdb321b24fe06b8b916ee2962889f772b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 25 Jul 2023 15:48:23 -0600 Subject: [PATCH 25/31] Make ERC2771Context return original sender address if `msg.data.length <= 20` (#4481) --- .changeset/unlucky-beans-obey.md | 5 +++++ contracts/metatx/ERC2771Context.sol | 2 +- test/metatx/ERC2771Context.test.js | 11 +++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 .changeset/unlucky-beans-obey.md diff --git a/.changeset/unlucky-beans-obey.md b/.changeset/unlucky-beans-obey.md new file mode 100644 index 00000000000..e472d3c6cb6 --- /dev/null +++ b/.changeset/unlucky-beans-obey.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC2771Context`: Return the forwarder address whenever the `msg.data` of a call originating from a trusted forwarder is not long enough to contain the request signer address (i.e. `msg.data.length` is less than 20 bytes), as specified by ERC-2771. diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 35de9f7ba73..9ade6148d39 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -22,7 +22,7 @@ abstract contract ERC2771Context is Context { } function _msgSender() internal view virtual override returns (address sender) { - if (isTrustedForwarder(msg.sender)) { + if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { // The assembly code is more direct than the Solidity version using `abi.decode`. /// @solidity memory-safe-assembly assembly { diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 3dd4b41532d..a907e502cdd 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -12,6 +12,8 @@ const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); contract('ERC2771Context', function (accounts) { + const [, anotherAccount] = accounts; + const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); beforeEach(async function () { @@ -79,6 +81,15 @@ contract('ERC2771Context', function (accounts) { const { tx } = await this.forwarder.execute(req); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender }); }); + + it('returns the original sender when calldata length is less than 20 bytes (address length)', async function () { + // The forwarder doesn't produce calls with calldata length less than 20 bytes + const recipient = await ERC2771ContextMock.new(anotherAccount); + + const { receipt } = await recipient.msgSender({ from: anotherAccount }); + + await expectEvent(receipt, 'Sender', { sender: anotherAccount }); + }); }); describe('msgData', function () { From 7222a31d548695998a475c9661fa159ef45a0e88 Mon Sep 17 00:00:00 2001 From: Prince Allwin Date: Thu, 27 Jul 2023 03:27:50 +0530 Subject: [PATCH 26/31] Add internal functions inside modifiers (#4472) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García Co-authored-by: Hadrien Croubois Co-authored-by: Francisco --- .changeset/swift-numbers-cry.md | 5 +++ contracts/governance/Governor.sol | 25 ++++++++++----- contracts/proxy/utils/Initializable.sol | 9 +++++- contracts/proxy/utils/UUPSUpgradeable.sol | 37 +++++++++++++++++------ 4 files changed, 57 insertions(+), 19 deletions(-) create mode 100644 .changeset/swift-numbers-cry.md diff --git a/.changeset/swift-numbers-cry.md b/.changeset/swift-numbers-cry.md new file mode 100644 index 00000000000..48afbd24503 --- /dev/null +++ b/.changeset/swift-numbers-cry.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Governor`, `Initializable`, and `UUPSUpgradeable`: Use internal functions in modifiers to optimize bytecode size. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 94f29e95f37..a94b4b305e6 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -66,14 +66,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * governance protocol (since v4.6). */ modifier onlyGovernance() { - if (_executor() != _msgSender()) { - revert GovernorOnlyExecutor(_msgSender()); - } - if (_executor() != address(this)) { - bytes32 msgDataHash = keccak256(_msgData()); - // loop until popping the expected operation - throw if deque is empty (operation not authorized) - while (_governanceCall.popFront() != msgDataHash) {} - } + _checkGovernance(); _; } @@ -227,6 +220,22 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 return _proposals[proposalId].proposer; } + /** + * @dev Reverts if the `msg.sender` is not the executor. In case the executor is not this contract + * itself, the function reverts if `msg.data` is not whitelisted as a result of an {execute} + * operation. See {onlyGovernance}. + */ + function _checkGovernance() internal virtual { + if (_executor() != _msgSender()) { + revert GovernorOnlyExecutor(_msgSender()); + } + if (_executor() != address(this)) { + bytes32 msgDataHash = keccak256(_msgData()); + // loop until popping the expected operation - throw if deque is empty (operation not authorized) + while (_governanceCall.popFront() != msgDataHash) {} + } + } + /** * @dev Amount of votes already cast passes the threshold limit. */ diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 08171015d3c..43f82feca3c 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -138,10 +138,17 @@ abstract contract Initializable { * {initializer} and {reinitializer} modifiers, directly or indirectly. */ modifier onlyInitializing() { + _checkInitializing(); + _; + } + + /** + * @dev Reverts if the contract is not in an initializing state. See {onlyInitializing}. + */ + function _checkInitializing() internal view virtual { if (!_initializing) { revert NotInitializing(); } - _; } /** diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index 982e1e58f98..fd41e37ec40 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -48,12 +48,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { * fail. */ modifier onlyProxy() { - if ( - address(this) == __self || // Must be called through delegatecall - ERC1967Utils.getImplementation() != __self // Must be called through an active proxy - ) { - revert UUPSUnauthorizedCallContext(); - } + _checkProxy(); _; } @@ -62,10 +57,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { * callable on the implementing contract but not through proxies. */ modifier notDelegated() { - if (address(this) != __self) { - // Must not be called through delegatecall - revert UUPSUnauthorizedCallContext(); - } + _checkNotDelegated(); _; } @@ -96,6 +88,31 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { _upgradeToAndCallUUPS(newImplementation, data); } + /** + * @dev Reverts if the execution is not performed via delegatecall or the execution + * context is not of a proxy with an ERC1967-compliant implementation pointing to self. + * See {_onlyProxy}. + */ + function _checkProxy() internal view virtual { + if ( + address(this) == __self || // Must be called through delegatecall + ERC1967Utils.getImplementation() != __self // Must be called through an active proxy + ) { + revert UUPSUnauthorizedCallContext(); + } + } + + /** + * @dev Reverts if the execution is performed via delegatecall. + * See {notDelegated}. + */ + function _checkNotDelegated() internal view virtual { + if (address(this) != __self) { + // Must not be called through delegatecall + revert UUPSUnauthorizedCallContext(); + } + } + /** * @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract. Called by * {upgradeToAndCall}. From 7c02b5cab2ccca42ccb612e56d3942c6dae382cd Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 27 Jul 2023 20:37:31 +0200 Subject: [PATCH 27/31] Refactor DoubleEndedQueue (#4150) Co-authored-by: Francisco --- .changeset/strong-poems-thank.md | 5 + certora/harnesses/DoubleEndedQueueHarness.sol | 4 +- certora/run.js | 8 + certora/specs/DoubleEndedQueue.spec | 174 ++++++------------ certora/specs/helpers/helpers.spec | 3 - contracts/utils/structs/DoubleEndedQueue.sol | 68 +++---- requirements.txt | 2 +- 7 files changed, 104 insertions(+), 160 deletions(-) create mode 100644 .changeset/strong-poems-thank.md diff --git a/.changeset/strong-poems-thank.md b/.changeset/strong-poems-thank.md new file mode 100644 index 00000000000..5f496de7f68 --- /dev/null +++ b/.changeset/strong-poems-thank.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`DoubleEndedQueue`: refactor internal structure to use `uint128` instead of `int128`. This has no effect on the library interface. diff --git a/certora/harnesses/DoubleEndedQueueHarness.sol b/certora/harnesses/DoubleEndedQueueHarness.sol index 35dd4a54aa5..c6a800726c7 100644 --- a/certora/harnesses/DoubleEndedQueueHarness.sol +++ b/certora/harnesses/DoubleEndedQueueHarness.sol @@ -29,11 +29,11 @@ contract DoubleEndedQueueHarness { _deque.clear(); } - function begin() external view returns (int128) { + function begin() external view returns (uint128) { return _deque._begin; } - function end() external view returns (int128) { + function end() external view returns (uint128) { return _deque._end; } diff --git a/certora/run.js b/certora/run.js index fdee42d2ed4..68f34aab27c 100755 --- a/certora/run.js +++ b/certora/run.js @@ -28,6 +28,11 @@ const argv = require('yargs') type: 'number', default: 4, }, + verbose: { + alias: 'v', + type: 'count', + default: 0, + }, options: { alias: 'o', type: 'array', @@ -65,6 +70,9 @@ for (const { spec, contract, files, options = [] } of specs) { // Run certora, aggregate the output and print it at the end async function runCertora(spec, contract, files, options = []) { const args = [...files, '--verify', `${contract}:certora/specs/${spec}.spec`, ...options]; + if (argv.verbose) { + console.log('Running:', args.join(' ')); + } const child = proc.spawn('certoraRun', args); const stream = new PassThrough(); diff --git a/certora/specs/DoubleEndedQueue.spec b/certora/specs/DoubleEndedQueue.spec index 2a196772d5a..3b71bb4c7a8 100644 --- a/certora/specs/DoubleEndedQueue.spec +++ b/certora/specs/DoubleEndedQueue.spec @@ -1,64 +1,25 @@ -import "helpers/helpers.spec" +import "helpers/helpers.spec"; methods { - pushFront(bytes32) envfree - pushBack(bytes32) envfree - popFront() returns (bytes32) envfree - popBack() returns (bytes32) envfree - clear() envfree + function pushFront(bytes32) external envfree; + function pushBack(bytes32) external envfree; + function popFront() external returns (bytes32) envfree; + function popBack() external returns (bytes32) envfree; + function clear() external envfree; // exposed for FV - begin() returns (int128) envfree - end() returns (int128) envfree + function begin() external returns (uint128) envfree; + function end() external returns (uint128) envfree; // view - length() returns (uint256) envfree - empty() returns (bool) envfree - front() returns (bytes32) envfree - back() returns (bytes32) envfree - at_(uint256) returns (bytes32) envfree // at is a reserved word + function length() external returns (uint256) envfree; + function empty() external returns (bool) envfree; + function front() external returns (bytes32) envfree; + function back() external returns (bytes32) envfree; + function at_(uint256) external returns (bytes32) envfree; // at is a reserved word } -/* -┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Helpers │ -└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ -*/ - -function min_int128() returns mathint { - return -(1 << 127); -} - -function max_int128() returns mathint { - return (1 << 127) - 1; -} - -// Could be broken in theory, but not in practice -function boundedQueue() returns bool { - return - max_int128() > to_mathint(end()) && - min_int128() < to_mathint(begin()); -} - -/* -┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Invariant: end is larger or equal than begin │ -└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ -*/ -invariant boundariesConsistency() - end() >= begin() - filtered { f -> !f.isView } - { preserved { require boundedQueue(); } } - -/* -┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Invariant: length is end minus begin │ -└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ -*/ -invariant lengthConsistency() - length() == to_mathint(end()) - to_mathint(begin()) - filtered { f -> !f.isView } - { preserved { require boundedQueue(); } } +definition full() returns bool = length() == max_uint128; /* ┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ @@ -68,22 +29,19 @@ invariant lengthConsistency() invariant emptiness() empty() <=> length() == 0 filtered { f -> !f.isView } - { preserved { require boundedQueue(); } } /* ┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ Invariant: front points to the first index and back points to the last one │ └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ -invariant queueEndings() - at_(length() - 1) == back() && at_(0) == front() +invariant queueFront() + at_(0) == front() + filtered { f -> !f.isView } + +invariant queueBack() + at_(require_uint256(length() - 1)) == back() filtered { f -> !f.isView } - { - preserved { - requireInvariant boundariesConsistency(); - require boundedQueue(); - } - } /* ┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ @@ -91,18 +49,18 @@ invariant queueEndings() └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule pushFront(bytes32 value) { - require boundedQueue(); - uint256 lengthBefore = length(); + bool fullBefore = full(); pushFront@withrevert(value); + bool success = !lastReverted; // liveness - assert !lastReverted, "never reverts"; + assert success <=> !fullBefore, "never revert if not previously full"; // effect - assert front() == value, "front set to value"; - assert length() == lengthBefore + 1, "queue extended"; + assert success => front() == value, "front set to value"; + assert success => to_mathint(length()) == lengthBefore + 1, "queue extended"; } /* @@ -111,15 +69,13 @@ rule pushFront(bytes32 value) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule pushFrontConsistency(uint256 index) { - require boundedQueue(); - bytes32 beforeAt = at_(index); bytes32 value; pushFront(value); // try to read value - bytes32 afterAt = at_@withrevert(index + 1); + bytes32 afterAt = at_@withrevert(require_uint256(index + 1)); assert !lastReverted, "value still there"; assert afterAt == beforeAt, "data is preserved"; @@ -131,18 +87,18 @@ rule pushFrontConsistency(uint256 index) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule pushBack(bytes32 value) { - require boundedQueue(); - uint256 lengthBefore = length(); + bool fullBefore = full(); pushBack@withrevert(value); + bool success = !lastReverted; // liveness - assert !lastReverted, "never reverts"; + assert success <=> !fullBefore, "never revert if not previously full"; // effect - assert back() == value, "back set to value"; - assert length() == lengthBefore + 1, "queue increased"; + assert success => back() == value, "back set to value"; + assert success => to_mathint(length()) == lengthBefore + 1, "queue increased"; } /* @@ -151,8 +107,6 @@ rule pushBack(bytes32 value) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule pushBackConsistency(uint256 index) { - require boundedQueue(); - bytes32 beforeAt = at_(index); bytes32 value; @@ -171,9 +125,6 @@ rule pushBackConsistency(uint256 index) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule popFront { - requireInvariant boundariesConsistency(); - require boundedQueue(); - uint256 lengthBefore = length(); bytes32 frontBefore = front@withrevert(); @@ -185,7 +136,7 @@ rule popFront { // effect assert success => frontBefore == popped, "previous front is returned"; - assert success => length() == lengthBefore - 1, "queue decreased"; + assert success => to_mathint(length()) == lengthBefore - 1, "queue decreased"; } /* @@ -194,9 +145,6 @@ rule popFront { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule popFrontConsistency(uint256 index) { - requireInvariant boundariesConsistency(); - require boundedQueue(); - // Read (any) value that is not the front (this asserts the value exists / the queue is long enough) require index > 1; bytes32 before = at_(index); @@ -204,7 +152,7 @@ rule popFrontConsistency(uint256 index) { popFront(); // try to read value - bytes32 after = at_@withrevert(index - 1); + bytes32 after = at_@withrevert(require_uint256(index - 1)); assert !lastReverted, "value still exists in the queue"; assert before == after, "values are offset and not modified"; @@ -216,9 +164,6 @@ rule popFrontConsistency(uint256 index) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule popBack { - requireInvariant boundariesConsistency(); - require boundedQueue(); - uint256 lengthBefore = length(); bytes32 backBefore = back@withrevert(); @@ -230,7 +175,7 @@ rule popBack { // effect assert success => backBefore == popped, "previous back is returned"; - assert success => length() == lengthBefore - 1, "queue decreased"; + assert success => to_mathint(length()) == lengthBefore - 1, "queue decreased"; } /* @@ -239,11 +184,8 @@ rule popBack { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule popBackConsistency(uint256 index) { - requireInvariant boundariesConsistency(); - require boundedQueue(); - // Read (any) value that is not the back (this asserts the value exists / the queue is long enough) - require index < length() - 1; + require to_mathint(index) < length() - 1; bytes32 before = at_(index); popBack(); @@ -275,24 +217,25 @@ rule clear { │ Rule: front/back access reverts only if the queue is empty or querying out of bounds │ └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ -rule onlyEmptyRevert(env e) { +rule onlyEmptyOrFullRevert(env e) { require nonpayable(e); - requireInvariant boundariesConsistency(); - require boundedQueue(); method f; calldataarg args; bool emptyBefore = empty(); + bool fullBefore = full(); f@withrevert(e, args); assert lastReverted => ( - (f.selector == front().selector && emptyBefore) || - (f.selector == back().selector && emptyBefore) || - (f.selector == popFront().selector && emptyBefore) || - (f.selector == popBack().selector && emptyBefore) || - f.selector == at_(uint256).selector // revert conditions are verified in onlyOutOfBoundsRevert + (f.selector == sig:front().selector && emptyBefore) || + (f.selector == sig:back().selector && emptyBefore) || + (f.selector == sig:popFront().selector && emptyBefore) || + (f.selector == sig:popBack().selector && emptyBefore) || + (f.selector == sig:pushFront(bytes32).selector && fullBefore ) || + (f.selector == sig:pushBack(bytes32).selector && fullBefore ) || + f.selector == sig:at_(uint256).selector // revert conditions are verified in onlyOutOfBoundsRevert ), "only revert if empty or out of bounds"; } @@ -302,9 +245,6 @@ rule onlyEmptyRevert(env e) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule onlyOutOfBoundsRevert(uint256 index) { - requireInvariant boundariesConsistency(); - require boundedQueue(); - at_@withrevert(index); assert lastReverted <=> index >= length(), "only reverts if index is out of bounds"; @@ -316,9 +256,6 @@ rule onlyOutOfBoundsRevert(uint256 index) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule noLengthChange(env e) { - requireInvariant boundariesConsistency(); - require boundedQueue(); - method f; calldataarg args; @@ -327,11 +264,11 @@ rule noLengthChange(env e) { uint256 lengthAfter = length(); assert lengthAfter != lengthBefore => ( - (f.selector == pushFront(bytes32).selector && lengthAfter == lengthBefore + 1) || - (f.selector == pushBack(bytes32).selector && lengthAfter == lengthBefore + 1) || - (f.selector == popBack().selector && lengthAfter == lengthBefore - 1) || - (f.selector == popFront().selector && lengthAfter == lengthBefore - 1) || - (f.selector == clear().selector && lengthAfter == 0) + (f.selector == sig:pushFront(bytes32).selector && to_mathint(lengthAfter) == lengthBefore + 1) || + (f.selector == sig:pushBack(bytes32).selector && to_mathint(lengthAfter) == lengthBefore + 1) || + (f.selector == sig:popBack().selector && to_mathint(lengthAfter) == lengthBefore - 1) || + (f.selector == sig:popFront().selector && to_mathint(lengthAfter) == lengthBefore - 1) || + (f.selector == sig:clear().selector && lengthAfter == 0) ), "length is only affected by clear/pop/push operations"; } @@ -341,9 +278,6 @@ rule noLengthChange(env e) { └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ */ rule noDataChange(env e) { - requireInvariant boundariesConsistency(); - require boundedQueue(); - method f; calldataarg args; @@ -354,13 +288,13 @@ rule noDataChange(env e) { bool atAfterSuccess = !lastReverted; assert !atAfterSuccess <=> ( - f.selector == clear().selector || - (f.selector == popBack().selector && index == length()) || - (f.selector == popFront().selector && index == length()) + (f.selector == sig:clear().selector ) || + (f.selector == sig:popBack().selector && index == length()) || + (f.selector == sig:popFront().selector && index == length()) ), "indexes of the queue are only removed by clear or pop"; assert atAfterSuccess && atAfter != atBefore => ( - f.selector == popFront().selector || - f.selector == pushFront(bytes32).selector + f.selector == sig:popFront().selector || + f.selector == sig:pushFront(bytes32).selector ), "values of the queue are only changed by popFront or pushFront"; } diff --git a/certora/specs/helpers/helpers.spec b/certora/specs/helpers/helpers.spec index 04e76df94ae..a6c1e230248 100644 --- a/certora/specs/helpers/helpers.spec +++ b/certora/specs/helpers/helpers.spec @@ -2,9 +2,6 @@ definition nonpayable(env e) returns bool = e.msg.value == 0; definition nonzerosender(env e) returns bool = e.msg.sender != 0; -// constants -definition max_uint48() returns mathint = (1 << 48) - 1; - // math definition min(mathint a, mathint b) returns mathint = a < b ? a : b; definition max(mathint a, mathint b) returns mathint = a > b ? a : b; diff --git a/contracts/utils/structs/DoubleEndedQueue.sol b/contracts/utils/structs/DoubleEndedQueue.sol index 30d9532393d..928665beee8 100644 --- a/contracts/utils/structs/DoubleEndedQueue.sol +++ b/contracts/utils/structs/DoubleEndedQueue.sol @@ -2,8 +2,6 @@ // OpenZeppelin Contracts (last updated v4.9.0) (utils/structs/DoubleEndedQueue.sol) pragma solidity ^0.8.19; -import {SafeCast} from "../math/SafeCast.sol"; - /** * @dev A sequence of items with the ability to efficiently push and pop items (i.e. insert and remove) on both ends of * the sequence (called front and back). Among other access patterns, it can be used to implement efficient LIFO and @@ -22,6 +20,11 @@ library DoubleEndedQueue { */ error QueueEmpty(); + /** + * @dev A push operation couldn't be completed due to the queue being full. + */ + error QueueFull(); + /** * @dev An operation (e.g. {at}) couldn't be completed due to an index being out of bounds. */ @@ -40,18 +43,19 @@ library DoubleEndedQueue { * data[end - 1]. */ struct Bytes32Deque { - int128 _begin; - int128 _end; - mapping(int128 => bytes32) _data; + uint128 _begin; + uint128 _end; + mapping(uint128 => bytes32) _data; } /** * @dev Inserts an item at the end of the queue. */ function pushBack(Bytes32Deque storage deque, bytes32 value) internal { - int128 backIndex = deque._end; - deque._data[backIndex] = value; unchecked { + uint128 backIndex = deque._end; + if (backIndex + 1 == deque._begin) revert QueueFull(); + deque._data[backIndex] = value; deque._end = backIndex + 1; } } @@ -62,26 +66,26 @@ library DoubleEndedQueue { * Reverts with `QueueEmpty` if the queue is empty. */ function popBack(Bytes32Deque storage deque) internal returns (bytes32 value) { - if (empty(deque)) revert QueueEmpty(); - int128 backIndex; unchecked { - backIndex = deque._end - 1; + uint128 backIndex = deque._end; + if (backIndex == deque._begin) revert QueueEmpty(); + --backIndex; + value = deque._data[backIndex]; + delete deque._data[backIndex]; + deque._end = backIndex; } - value = deque._data[backIndex]; - delete deque._data[backIndex]; - deque._end = backIndex; } /** * @dev Inserts an item at the beginning of the queue. */ function pushFront(Bytes32Deque storage deque, bytes32 value) internal { - int128 frontIndex; unchecked { - frontIndex = deque._begin - 1; + uint128 frontIndex = deque._begin - 1; + if (frontIndex == deque._end) revert QueueFull(); + deque._data[frontIndex] = value; + deque._begin = frontIndex; } - deque._data[frontIndex] = value; - deque._begin = frontIndex; } /** @@ -90,11 +94,11 @@ library DoubleEndedQueue { * Reverts with `QueueEmpty` if the queue is empty. */ function popFront(Bytes32Deque storage deque) internal returns (bytes32 value) { - if (empty(deque)) revert QueueEmpty(); - int128 frontIndex = deque._begin; - value = deque._data[frontIndex]; - delete deque._data[frontIndex]; unchecked { + uint128 frontIndex = deque._begin; + if (frontIndex == deque._end) revert QueueEmpty(); + value = deque._data[frontIndex]; + delete deque._data[frontIndex]; deque._begin = frontIndex + 1; } } @@ -106,8 +110,7 @@ library DoubleEndedQueue { */ function front(Bytes32Deque storage deque) internal view returns (bytes32 value) { if (empty(deque)) revert QueueEmpty(); - int128 frontIndex = deque._begin; - return deque._data[frontIndex]; + return deque._data[deque._begin]; } /** @@ -117,11 +120,9 @@ library DoubleEndedQueue { */ function back(Bytes32Deque storage deque) internal view returns (bytes32 value) { if (empty(deque)) revert QueueEmpty(); - int128 backIndex; unchecked { - backIndex = deque._end - 1; + return deque._data[deque._end - 1]; } - return deque._data[backIndex]; } /** @@ -131,10 +132,11 @@ library DoubleEndedQueue { * Reverts with `QueueOutOfBounds` if the index is out of bounds. */ function at(Bytes32Deque storage deque, uint256 index) internal view returns (bytes32 value) { - // int256(deque._begin) is a safe upcast - int128 idx = SafeCast.toInt128(int256(deque._begin) + SafeCast.toInt256(index)); - if (idx >= deque._end) revert QueueOutOfBounds(); - return deque._data[idx]; + if (index >= length(deque)) revert QueueOutOfBounds(); + // By construction, length is a uint128, so the check above ensures that index can be safely downcast to uint128. + unchecked { + return deque._data[deque._begin + uint128(index)]; + } } /** @@ -152,10 +154,8 @@ library DoubleEndedQueue { * @dev Returns the number of items in the queue. */ function length(Bytes32Deque storage deque) internal view returns (uint256) { - // The interface preserves the invariant that begin <= end so we assume this will not overflow. - // We also assume there are at most int256.max items in the queue. unchecked { - return uint256(int256(deque._end) - int256(deque._begin)); + return uint256(deque._end - deque._begin); } } @@ -163,6 +163,6 @@ library DoubleEndedQueue { * @dev Returns true if the queue is empty. */ function empty(Bytes32Deque storage deque) internal view returns (bool) { - return deque._end <= deque._begin; + return deque._end == deque._begin; } } diff --git a/requirements.txt b/requirements.txt index da3e95766cb..b92a2728d53 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1 +1 @@ -certora-cli==3.6.4 +certora-cli==4.3.1 From 9445f96223041abf2bf08daa56f8da50b674cbcd Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 27 Jul 2023 22:30:41 +0200 Subject: [PATCH 28/31] Adjust ERC2771Context._msgData for msg.data.length < 20 (#4484) --- .changeset/warm-guests-rule.md | 5 +++++ contracts/metatx/ERC2771Context.sol | 2 +- contracts/mocks/ContextMock.sol | 6 ++++++ test/metatx/ERC2771Context.test.js | 18 ++++++++++++++---- 4 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 .changeset/warm-guests-rule.md diff --git a/.changeset/warm-guests-rule.md b/.changeset/warm-guests-rule.md new file mode 100644 index 00000000000..049da4dd5d5 --- /dev/null +++ b/.changeset/warm-guests-rule.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC2771Context`: Prevent revert in `_msgData()` when a call originating from a trusted forwarder is not long enough to contain the request signer address (i.e. `msg.data.length` is less than 20 bytes). Return the full calldata in that case. diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 9ade6148d39..2c4d89f94cb 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -34,7 +34,7 @@ abstract contract ERC2771Context is Context { } function _msgData() internal view virtual override returns (bytes calldata) { - if (isTrustedForwarder(msg.sender)) { + if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { return msg.data[:msg.data.length - 20]; } else { return super._msgData(); diff --git a/contracts/mocks/ContextMock.sol b/contracts/mocks/ContextMock.sol index fd105aa9142..fb57535f706 100644 --- a/contracts/mocks/ContextMock.sol +++ b/contracts/mocks/ContextMock.sol @@ -16,6 +16,12 @@ contract ContextMock is Context { function msgData(uint256 integerValue, string memory stringValue) public { emit Data(_msgData(), integerValue, stringValue); } + + event DataShort(bytes data); + + function msgDataShort() public { + emit DataShort(_msgData()); + } } contract ContextMockCaller { diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index a907e502cdd..0ec8d98dde6 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -12,7 +12,7 @@ const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); contract('ERC2771Context', function (accounts) { - const [, anotherAccount] = accounts; + const [, trustedForwarder] = accounts; const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); @@ -84,11 +84,11 @@ contract('ERC2771Context', function (accounts) { it('returns the original sender when calldata length is less than 20 bytes (address length)', async function () { // The forwarder doesn't produce calls with calldata length less than 20 bytes - const recipient = await ERC2771ContextMock.new(anotherAccount); + const recipient = await ERC2771ContextMock.new(trustedForwarder); - const { receipt } = await recipient.msgSender({ from: anotherAccount }); + const { receipt } = await recipient.msgSender({ from: trustedForwarder }); - await expectEvent(receipt, 'Sender', { sender: anotherAccount }); + await expectEvent(receipt, 'Sender', { sender: trustedForwarder }); }); }); @@ -117,5 +117,15 @@ contract('ERC2771Context', function (accounts) { await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); }); }); + + it('returns the full original data when calldata length is less than 20 bytes (address length)', async function () { + // The forwarder doesn't produce calls with calldata length less than 20 bytes + const recipient = await ERC2771ContextMock.new(trustedForwarder); + + const { receipt } = await recipient.msgDataShort({ from: trustedForwarder }); + + const data = recipient.contract.methods.msgDataShort().encodeABI(); + await expectEvent(receipt, 'DataShort', { data }); + }); }); }); From 02ea01765a9964541dd9cdcffc4a7f8b403c2ff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 27 Jul 2023 17:18:45 -0600 Subject: [PATCH 29/31] Add custom errors to docs (#4480) --- contracts/access/AccessControl.sol | 10 +++++----- contracts/interfaces/README.adoc | 9 +++++++++ contracts/interfaces/draft-IERC6093.sol | 9 +++------ docs/templates/contract.hbs | 26 +++++++++++++++++++++++++ docs/templates/properties.js | 4 ++++ 5 files changed, 47 insertions(+), 11 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 99099dd1f43..bc81c4609c5 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -58,8 +58,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { /** * @dev Modifier that checks that an account has a specific role. Reverts - * with a custom error including the required role. - * + * with an {AccessControlUnauthorizedAccount} error including the required role. */ modifier onlyRole(bytes32 role) { _checkRole(role); @@ -81,15 +80,16 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { } /** - * @dev Revert with a custom error if `_msgSender()` is missing `role`. - * Overriding this function changes the behavior of the {onlyRole} modifier. + * @dev Reverts with an {AccessControlUnauthorizedAccount} error if `_msgSender()` + * is missing `role`. Overriding this function changes the behavior of the {onlyRole} modifier. */ function _checkRole(bytes32 role) internal view virtual { _checkRole(role, _msgSender()); } /** - * @dev Revert with a custom error if `account` is missing `role`. + * @dev Reverts with an {AccessControlUnauthorizedAccount} error if `account` + * is missing `role`. */ function _checkRole(bytes32 role, address account) internal view virtual { if (!hasRole(role, account)) { diff --git a/contracts/interfaces/README.adoc b/contracts/interfaces/README.adoc index 4525bc9a2ee..379a24a1e26 100644 --- a/contracts/interfaces/README.adoc +++ b/contracts/interfaces/README.adoc @@ -8,18 +8,21 @@ These interfaces are available as `.sol` files, and also as compiler `.json` ABI are useful to interact with third party contracts that implement them. - {IERC20} +- {IERC20Errors} - {IERC20Metadata} - {IERC165} - {IERC721} - {IERC721Receiver} - {IERC721Enumerable} - {IERC721Metadata} +- {IERC721Errors} - {IERC777} - {IERC777Recipient} - {IERC777Sender} - {IERC1155} - {IERC1155Receiver} - {IERC1155MetadataURI} +- {IERC1155Errors} - {IERC1271} - {IERC1363} - {IERC1363Receiver} @@ -40,6 +43,12 @@ are useful to interact with third party contracts that implement them. == Detailed ABI +{{IERC20Errors}} + +{{IERC721Errors}} + +{{IERC1155Errors}} + {{IERC1271}} {{IERC1363}} diff --git a/contracts/interfaces/draft-IERC6093.sol b/contracts/interfaces/draft-IERC6093.sol index 08e77553c20..3c390085221 100644 --- a/contracts/interfaces/draft-IERC6093.sol +++ b/contracts/interfaces/draft-IERC6093.sol @@ -3,8 +3,7 @@ pragma solidity ^0.8.19; /** * @dev Standard ERC20 Errors - * Interface of the ERC6093 custom errors for ERC20 tokens - * as defined in https://eips.ethereum.org/EIPS/eip-6093 + * Interface of the https://eips.ethereum.org/EIPS/eip-6093[ERC-6093] custom errors for ERC20 tokens. */ interface IERC20Errors { /** @@ -50,8 +49,7 @@ interface IERC20Errors { /** * @dev Standard ERC721 Errors - * Interface of the ERC6093 custom errors for ERC721 tokens - * as defined in https://eips.ethereum.org/EIPS/eip-6093 + * Interface of the https://eips.ethereum.org/EIPS/eip-6093[ERC-6093] custom errors for ERC721 tokens. */ interface IERC721Errors { /** @@ -109,8 +107,7 @@ interface IERC721Errors { /** * @dev Standard ERC1155 Errors - * Interface of the ERC6093 custom errors for ERC1155 tokens - * as defined in https://eips.ethereum.org/EIPS/eip-6093 + * Interface of the https://eips.ethereum.org/EIPS/eip-6093[ERC-6093] custom errors for ERC1155 tokens. */ interface IERC1155Errors { /** diff --git a/docs/templates/contract.hbs b/docs/templates/contract.hbs index d97e7fd0d1c..a4716825ab6 100644 --- a/docs/templates/contract.hbs +++ b/docs/templates/contract.hbs @@ -57,6 +57,23 @@ import "@openzeppelin/{{__item_context.file.absolutePath}}"; -- {{/if}} +{{#if has-errors}} +[.contract-index] +.Errors +-- +{{#each inheritance}} +{{#unless @first}} +[.contract-subindex-inherited] +.{{name}} +{{/unless}} +{{#each errors}} +* {xref-{{anchor~}} }[`++{{name}}({{names params}})++`] +{{/each}} + +{{/each}} +-- +{{/if}} + {{#each modifiers}} [.contract-item] [[{{anchor}}]] @@ -83,3 +100,12 @@ import "@openzeppelin/{{__item_context.file.absolutePath}}"; {{{natspec.dev}}} {{/each}} + +{{#each errors}} +[.contract-item] +[[{{anchor}}]] +==== `[.contract-item-name]#++{{name}}++#++({{typed-params params}})++` [.item-kind]#error# + +{{{natspec.dev}}} + +{{/each}} diff --git a/docs/templates/properties.js b/docs/templates/properties.js index 99bdf88b260..584c0abcac7 100644 --- a/docs/templates/properties.js +++ b/docs/templates/properties.js @@ -35,6 +35,10 @@ module.exports['has-events'] = function ({ item }) { return item.inheritance.some(c => c.events.length > 0); }; +module.exports['has-errors'] = function ({ item }) { + return item.inheritance.some(c => c.errors.length > 0); +}; + module.exports['inherited-functions'] = function ({ item }) { const { inheritance } = item; const baseFunctions = new Set(inheritance.flatMap(c => c.functions.flatMap(f => f.baseFunctions ?? []))); From aed5720a0177ddd36e2923ecedb1759205ee6014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 28 Jul 2023 18:58:30 -0600 Subject: [PATCH 30/31] Avoid `returndatacopy` in ERC2771Forwarder by calling via assembly (#4458) --- contracts/metatx/ERC2771Context.sol | 6 ++ contracts/metatx/ERC2771Forwarder.sol | 16 +++- test/metatx/ERC2771Forwarder.test.js | 112 +++++++++++++++++++++++--- 3 files changed, 117 insertions(+), 17 deletions(-) diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 2c4d89f94cb..406a62bd648 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -7,6 +7,12 @@ import {Context} from "../utils/Context.sol"; /** * @dev Context variant with ERC2771 support. + * + * WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll + * be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC2771 + * specification adding the address size in bytes (20) to the calldata size. An example of an unexpected + * behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive` + * function only accessible if `msg.data.length == 0`. */ abstract contract ERC2771Context is Context { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 84d736c7416..125f80076e0 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -251,11 +251,19 @@ contract ERC2771Forwarder is EIP712, Nonces { // Nonce should be used before the call to prevent reusing by reentrancy uint256 currentNonce = _useNonce(signer); - (success, ) = request.to.call{gas: request.gas, value: request.value}( - abi.encodePacked(request.data, request.from) - ); + uint256 reqGas = request.gas; + address to = request.to; + uint256 value = request.value; + bytes memory data = abi.encodePacked(request.data, request.from); - _checkForwardedGas(gasleft(), request); + uint256 gasLeft; + + assembly { + success := call(reqGas, to, value, add(data, 0x20), mload(data), 0, 0) + gasLeft := gas() + } + + _checkForwardedGas(gasLeft, request); emit ExecutedForwardRequest(signer, currentNonce, success); } diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index fa84ccdc301..26726e8df4c 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -41,12 +41,13 @@ contract('ERC2771Forwarder', function (accounts) { this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); this.timestamp = await time.latest(); + this.receiver = await CallReceiverMock.new(); this.request = { from: this.alice.address, - to: constants.ZERO_ADDRESS, + to: this.receiver.address, value: '0', gas: '100000', - data: '0x', + data: this.receiver.contract.methods.mockFunction().encodeABI(), deadline: this.timestamp.toNumber() + 60, // 1 minute }; this.requestData = { @@ -64,6 +65,14 @@ contract('ERC2771Forwarder', function (accounts) { ethSigUtil.signTypedMessage(privateKey, { data: this.forgeData(request), }); + this.estimateRequest = request => + web3.eth.estimateGas({ + from: this.forwarder.address, + to: request.to, + data: web3.utils.encodePacked({ value: request.data, type: 'bytes' }, { value: request.from, type: 'address' }), + value: request.value, + gas: request.gas, + }); this.requestData.signature = this.sign(this.alice.getPrivateKey()); }); @@ -125,7 +134,8 @@ contract('ERC2771Forwarder', function (accounts) { it('emits an event and consumes nonce for a successful request', async function () { const receipt = await this.forwarder.execute(this.requestData); - expectEvent(receipt, 'ExecutedForwardRequest', { + await expectEvent.inTransaction(receipt.tx, this.receiver, 'MockFunctionCalled'); + await expectEvent.inTransaction(receipt.tx, this.forwarder, 'ExecutedForwardRequest', { signer: this.requestData.from, nonce: web3.utils.toBN(this.requestData.nonce), success: true, @@ -136,11 +146,9 @@ contract('ERC2771Forwarder', function (accounts) { }); it('reverts with an unsuccessful request', async function () { - const receiver = await CallReceiverMock.new(); const req = { ...this.requestData, - to: receiver.address, - data: receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(), + data: this.receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(), }; req.signature = this.sign(this.alice.getPrivateKey(), req); await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []); @@ -208,14 +216,11 @@ contract('ERC2771Forwarder', function (accounts) { }); it('bubbles out of gas', async function () { - const receiver = await CallReceiverMock.new(); - const gasAvailable = 100000; - this.requestData.to = receiver.address; - this.requestData.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.requestData.gas = 1000000; - + this.requestData.data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestData.gas = 1_000_000; this.requestData.signature = this.sign(this.alice.getPrivateKey()); + const gasAvailable = 100_000; await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable })); const { transactions } = await web3.eth.getBlock('latest'); @@ -223,6 +228,32 @@ contract('ERC2771Forwarder', function (accounts) { expect(gasUsed).to.be.equal(gasAvailable); }); + + it('bubbles out of gas forced by the relayer', async function () { + // If there's an incentive behind executing requests, a malicious relayer could grief + // the forwarder by executing requests and providing a top-level call gas limit that + // is too low to successfully finish the request after the 63/64 rule. + + // We set the baseline to the gas limit consumed by a successful request if it was executed + // normally. Note this includes the 21000 buffer that also the relayer will be charged to + // start a request execution. + const estimate = await this.estimateRequest(this.request); + + // Because the relayer call consumes gas until the `CALL` opcode, the gas left after failing + // the subcall won't enough to finish the top level call (after testing), so we add a + // moderated buffer. + const gasAvailable = estimate + 2_000; + + // The subcall out of gas should be caught by the contract and then bubbled up consuming + // the available gas with an `invalid` opcode. + await expectRevert.outOfGas(this.forwarder.execute(this.requestData, { gas: gasAvailable })); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + // We assert that indeed the gas was totally consumed. + expect(gasUsed).to.be.equal(gasAvailable); + }); }); context('executeBatch', function () { @@ -252,6 +283,14 @@ contract('ERC2771Forwarder', function (accounts) { })); this.msgValue = batchValue(this.requestDatas); + + this.gasUntil = async reqIdx => { + const gas = 0; + const estimations = await Promise.all( + new Array(reqIdx + 1).fill().map((_, idx) => this.estimateRequest(this.requestDatas[idx])), + ); + return estimations.reduce((acc, estimation) => acc + estimation, gas); + }; }); context('with valid requests', function () { @@ -265,7 +304,8 @@ contract('ERC2771Forwarder', function (accounts) { it('emits events', async function () { for (const request of this.requestDatas) { - expectEvent(this.receipt, 'ExecutedForwardRequest', { + await expectEvent.inTransaction(this.receipt.tx, this.receiver, 'MockFunctionCalled'); + await expectEvent.inTransaction(this.receipt.tx, this.forwarder, 'ExecutedForwardRequest', { signer: request.from, nonce: web3.utils.toBN(request.nonce), success: true, @@ -428,6 +468,52 @@ contract('ERC2771Forwarder', function (accounts) { ); }); }); + + it('bubbles out of gas', async function () { + this.requestDatas[this.idx].data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestDatas[this.idx].gas = 1_000_000; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + + const gasAvailable = 300_000; + await expectRevert.assertion( + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { + gas: gasAvailable, + value: this.requestDatas.reduce((acc, { value }) => acc + Number(value), 0), + }), + ); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + expect(gasUsed).to.be.equal(gasAvailable); + }); + + it('bubbles out of gas forced by the relayer', async function () { + // Similarly to the single execute, a malicious relayer could grief requests. + + // We estimate until the selected request as if they were executed normally + const estimate = await this.gasUntil(this.requestDatas, this.idx); + + // We add a Buffer to account for all the gas that's used before the selected call. + // Note is slightly bigger because the selected request is not the index 0 and it affects + // the buffer needed. + const gasAvailable = estimate + 10_000; + + // The subcall out of gas should be caught by the contract and then bubbled up consuming + // the available gas with an `invalid` opcode. + await expectRevert.outOfGas( + this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { gas: gasAvailable }), + ); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + // We assert that indeed the gas was totally consumed. + expect(gasUsed).to.be.equal(gasAvailable); + }); }); }); }); From f631d8a5f00d06d60912bcf03181aa1e700d42f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 28 Jul 2023 19:16:14 -0600 Subject: [PATCH 31/31] Improve ERC4626 fees example (#4476) Co-authored-by: Francisco Co-authored-by: Hadrien Croubois --- contracts/mocks/docs/ERC4626Fees.sol | 59 +++++++++++++-------- contracts/mocks/token/ERC4646FeesMock.sol | 20 +++---- test/token/ERC20/extensions/ERC4626.test.js | 6 +-- 3 files changed, 49 insertions(+), 36 deletions(-) diff --git a/contracts/mocks/docs/ERC4626Fees.sol b/contracts/mocks/docs/ERC4626Fees.sol index 49c1095ab47..ab1151cd78f 100644 --- a/contracts/mocks/docs/ERC4626Fees.sol +++ b/contracts/mocks/docs/ERC4626Fees.sol @@ -7,36 +7,41 @@ import {ERC4626} from "../../token/ERC20/extensions/ERC4626.sol"; import {SafeERC20} from "../../token/ERC20/utils/SafeERC20.sol"; import {Math} from "../../utils/math/Math.sol"; +/// @dev ERC4626 vault with entry/exit fees expressed in https://en.wikipedia.org/wiki/Basis_point[basis point (bp)]. abstract contract ERC4626Fees is ERC4626 { using Math for uint256; - /** @dev See {IERC4626-previewDeposit}. */ + uint256 private constant _BASIS_POINT_SCALE = 1e4; + + // === Overrides === /// + + /// @dev Preview taking an entry fee on deposit. See {IERC4626-previewDeposit}. function previewDeposit(uint256 assets) public view virtual override returns (uint256) { - uint256 fee = _feeOnTotal(assets, _entryFeeBasePoint()); + uint256 fee = _feeOnTotal(assets, _entryFeeBasisPoints()); return super.previewDeposit(assets - fee); } - /** @dev See {IERC4626-previewMint}. */ + /// @dev Preview adding an entry fee on mint. See {IERC4626-previewMint}. function previewMint(uint256 shares) public view virtual override returns (uint256) { uint256 assets = super.previewMint(shares); - return assets + _feeOnRaw(assets, _entryFeeBasePoint()); + return assets + _feeOnRaw(assets, _entryFeeBasisPoints()); } - /** @dev See {IERC4626-previewWithdraw}. */ + /// @dev Preview adding an exit fee on withdraw. See {IERC4626-previewWithdraw}. function previewWithdraw(uint256 assets) public view virtual override returns (uint256) { - uint256 fee = _feeOnRaw(assets, _exitFeeBasePoint()); + uint256 fee = _feeOnRaw(assets, _exitFeeBasisPoints()); return super.previewWithdraw(assets + fee); } - /** @dev See {IERC4626-previewRedeem}. */ + /// @dev Preview taking an exit fee on redeem. See {IERC4626-previewRedeem}. function previewRedeem(uint256 shares) public view virtual override returns (uint256) { uint256 assets = super.previewRedeem(shares); - return assets - _feeOnTotal(assets, _exitFeeBasePoint()); + return assets - _feeOnTotal(assets, _exitFeeBasisPoints()); } - /** @dev See {IERC4626-_deposit}. */ + /// @dev Send entry fee to {_entryFeeRecipient}. See {IERC4626-_deposit}. function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal virtual override { - uint256 fee = _feeOnTotal(assets, _entryFeeBasePoint()); + uint256 fee = _feeOnTotal(assets, _entryFeeBasisPoints()); address recipient = _entryFeeRecipient(); super._deposit(caller, receiver, assets, shares); @@ -46,7 +51,7 @@ abstract contract ERC4626Fees is ERC4626 { } } - /** @dev See {IERC4626-_deposit}. */ + /// @dev Send exit fee to {_exitFeeRecipient}. See {IERC4626-_deposit}. function _withdraw( address caller, address receiver, @@ -54,7 +59,7 @@ abstract contract ERC4626Fees is ERC4626 { uint256 assets, uint256 shares ) internal virtual override { - uint256 fee = _feeOnRaw(assets, _exitFeeBasePoint()); + uint256 fee = _feeOnRaw(assets, _exitFeeBasisPoints()); address recipient = _exitFeeRecipient(); super._withdraw(caller, receiver, owner, assets, shares); @@ -64,27 +69,35 @@ abstract contract ERC4626Fees is ERC4626 { } } - function _entryFeeBasePoint() internal view virtual returns (uint256) { - return 0; + // === Fee configuration === /// + + function _entryFeeBasisPoints() internal view virtual returns (uint256) { + return 0; // replace with e.g. 1_000 for 1% } - function _entryFeeRecipient() internal view virtual returns (address) { - return address(0); + function _exitFeeBasisPoints() internal view virtual returns (uint256) { + return 0; // replace with e.g. 1_000 for 1% } - function _exitFeeBasePoint() internal view virtual returns (uint256) { - return 0; + function _entryFeeRecipient() internal view virtual returns (address) { + return address(0); // replace with e.g. a treasury address } function _exitFeeRecipient() internal view virtual returns (address) { - return address(0); + return address(0); // replace with e.g. a treasury address } - function _feeOnRaw(uint256 assets, uint256 feeBasePoint) private pure returns (uint256) { - return assets.mulDiv(feeBasePoint, 1e5, Math.Rounding.Ceil); + // === Fee operations === /// + + /// @dev Calculates the fees that should be added to an amount `assets` that does not already include fees. + /// Used in {IERC4626-mint} and {IERC4626-withdraw} operations. + function _feeOnRaw(uint256 assets, uint256 feeBasisPoints) private pure returns (uint256) { + return assets.mulDiv(feeBasisPoints, _BASIS_POINT_SCALE, Math.Rounding.Ceil); } - function _feeOnTotal(uint256 assets, uint256 feeBasePoint) private pure returns (uint256) { - return assets.mulDiv(feeBasePoint, feeBasePoint + 1e5, Math.Rounding.Ceil); + /// @dev Calculates the fee part of an amount `assets` that already includes fees. + /// Used in {IERC4626-deposit} and {IERC4626-redeem} operations. + function _feeOnTotal(uint256 assets, uint256 feeBasisPoints) private pure returns (uint256) { + return assets.mulDiv(feeBasisPoints, feeBasisPoints + _BASIS_POINT_SCALE, Math.Rounding.Ceil); } } diff --git a/contracts/mocks/token/ERC4646FeesMock.sol b/contracts/mocks/token/ERC4646FeesMock.sol index f8fde293c50..081b03a61ab 100644 --- a/contracts/mocks/token/ERC4646FeesMock.sol +++ b/contracts/mocks/token/ERC4646FeesMock.sol @@ -5,33 +5,33 @@ pragma solidity ^0.8.19; import {ERC4626Fees} from "../docs/ERC4626Fees.sol"; abstract contract ERC4626FeesMock is ERC4626Fees { - uint256 private immutable _entryFeeBasePointValue; + uint256 private immutable _entryFeeBasisPointValue; address private immutable _entryFeeRecipientValue; - uint256 private immutable _exitFeeBasePointValue; + uint256 private immutable _exitFeeBasisPointValue; address private immutable _exitFeeRecipientValue; constructor( - uint256 entryFeeBasePoint, + uint256 entryFeeBasisPoints, address entryFeeRecipient, - uint256 exitFeeBasePoint, + uint256 exitFeeBasisPoints, address exitFeeRecipient ) { - _entryFeeBasePointValue = entryFeeBasePoint; + _entryFeeBasisPointValue = entryFeeBasisPoints; _entryFeeRecipientValue = entryFeeRecipient; - _exitFeeBasePointValue = exitFeeBasePoint; + _exitFeeBasisPointValue = exitFeeBasisPoints; _exitFeeRecipientValue = exitFeeRecipient; } - function _entryFeeBasePoint() internal view virtual override returns (uint256) { - return _entryFeeBasePointValue; + function _entryFeeBasisPoints() internal view virtual override returns (uint256) { + return _entryFeeBasisPointValue; } function _entryFeeRecipient() internal view virtual override returns (address) { return _entryFeeRecipientValue; } - function _exitFeeBasePoint() internal view virtual override returns (uint256) { - return _exitFeeBasePointValue; + function _exitFeeBasisPoints() internal view virtual override returns (uint256) { + return _exitFeeBasisPointValue; } function _exitFeeRecipient() internal view virtual override returns (address) { diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index aead1d50a4c..499cf0628d9 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -737,9 +737,9 @@ contract('ERC4626', function (accounts) { } describe('ERC4626Fees', function () { - const feeBasePoint = web3.utils.toBN(5e3); + const feeBasisPoints = web3.utils.toBN(5e3); const valueWithoutFees = web3.utils.toBN(10000); - const fees = valueWithoutFees.mul(feeBasePoint).divn(1e5); + const fees = valueWithoutFees.mul(feeBasisPoints).divn(1e4); const valueWithFees = valueWithoutFees.add(fees); describe('input fees', function () { @@ -749,7 +749,7 @@ contract('ERC4626', function (accounts) { name + ' Vault', symbol + 'V', this.token.address, - feeBasePoint, + feeBasisPoints, other, 0, constants.ZERO_ADDRESS,