diff --git a/.changeset/unlucky-snakes-drive.md b/.changeset/unlucky-snakes-drive.md new file mode 100644 index 00000000000..d4c78dd50b1 --- /dev/null +++ b/.changeset/unlucky-snakes-drive.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`Address`: Removed `isContract` because of its ambiguous nature and potential for misuse. diff --git a/contracts/mocks/proxy/UUPSLegacy.sol b/contracts/mocks/proxy/UUPSLegacy.sol index 7a3002889bf..ba68e604668 100644 --- a/contracts/mocks/proxy/UUPSLegacy.sol +++ b/contracts/mocks/proxy/UUPSLegacy.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; +pragma solidity ^0.8.1; import "./UUPSUpgradeableMock.sol"; @@ -13,7 +13,7 @@ contract UUPSUpgradeableLegacyMock is UUPSUpgradeableMock { // ERC1967Upgrade._setImplementation is private so we reproduce it here. // An extra underscore prevents a name clash error. function __setImplementation(address newImplementation) private { - require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract"); + require(newImplementation.code.length > 0, "ERC1967: new implementation is not a contract"); StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; } diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index 38ea7ea1bf9..58f65fe881f 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -43,7 +43,7 @@ abstract contract ERC1967Upgrade { * @dev Stores a new address in the EIP1967 implementation slot. */ function _setImplementation(address newImplementation) private { - require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract"); + require(newImplementation.code.length > 0, "ERC1967: new implementation is not a contract"); StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; } @@ -153,9 +153,9 @@ abstract contract ERC1967Upgrade { * @dev Stores a new beacon in the EIP1967 beacon slot. */ function _setBeacon(address newBeacon) private { - require(Address.isContract(newBeacon), "ERC1967: new beacon is not a contract"); + require(newBeacon.code.length > 0, "ERC1967: new beacon is not a contract"); require( - Address.isContract(IBeacon(newBeacon).implementation()), + IBeacon(newBeacon).implementation().code.length > 0, "ERC1967: beacon implementation is not a contract" ); StorageSlot.getAddressSlot(_BEACON_SLOT).value = newBeacon; diff --git a/contracts/proxy/beacon/UpgradeableBeacon.sol b/contracts/proxy/beacon/UpgradeableBeacon.sol index 5d83ceb3b4f..9eeb149a69e 100644 --- a/contracts/proxy/beacon/UpgradeableBeacon.sol +++ b/contracts/proxy/beacon/UpgradeableBeacon.sol @@ -1,11 +1,10 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts v4.4.1 (proxy/beacon/UpgradeableBeacon.sol) -pragma solidity ^0.8.0; +pragma solidity ^0.8.1; import "./IBeacon.sol"; import "../../access/Ownable.sol"; -import "../../utils/Address.sol"; /** * @dev This contract is used in conjunction with one or more instances of {BeaconProxy} to determine their @@ -59,7 +58,7 @@ contract UpgradeableBeacon is IBeacon, Ownable { * - `newImplementation` must be a contract. */ function _setImplementation(address newImplementation) private { - require(Address.isContract(newImplementation), "UpgradeableBeacon: implementation is not a contract"); + require(newImplementation.code.length > 0, "UpgradeableBeacon: implementation is not a contract"); _implementation = newImplementation; } } diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index b454b5d95b7..4ad0b9826b8 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -83,7 +83,7 @@ abstract contract Initializable { modifier initializer() { bool isTopLevelCall = !_initializing; require( - (isTopLevelCall && _initialized < 1) || (!Address.isContract(address(this)) && _initialized == 1), + (isTopLevelCall && _initialized < 1) || (address(this).code.length == 0 && _initialized == 1), "Initializable: contract is already initialized" ); _initialized = 1; diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol index e491a23100b..c720267340c 100644 --- a/contracts/token/ERC1155/ERC1155.sol +++ b/contracts/token/ERC1155/ERC1155.sol @@ -1,12 +1,11 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.8.0) (token/ERC1155/ERC1155.sol) -pragma solidity ^0.8.0; +pragma solidity ^0.8.1; import "./IERC1155.sol"; import "./IERC1155Receiver.sol"; import "./extensions/IERC1155MetadataURI.sol"; -import "../../utils/Address.sol"; import "../../utils/Context.sol"; import "../../utils/introspection/ERC165.sol"; @@ -18,8 +17,6 @@ import "../../utils/introspection/ERC165.sol"; * _Available since v3.1._ */ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI { - using Address for address; - // Mapping from token ID to account balances mapping(uint256 => mapping(address => uint256)) private _balances; @@ -344,7 +341,7 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI { uint256 amount, bytes memory data ) private { - if (to.isContract()) { + if (to.code.length > 0) { try IERC1155Receiver(to).onERC1155Received(operator, from, id, amount, data) returns (bytes4 response) { if (response != IERC1155Receiver.onERC1155Received.selector) { revert("ERC1155: ERC1155Receiver rejected tokens"); @@ -365,7 +362,7 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI { uint256[] memory amounts, bytes memory data ) private { - if (to.isContract()) { + if (to.code.length > 0) { try IERC1155Receiver(to).onERC1155BatchReceived(operator, from, ids, amounts, data) returns ( bytes4 response ) { diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 6bf620b4f24..0d159f609f4 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -1,12 +1,11 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.8.0) (token/ERC721/ERC721.sol) -pragma solidity ^0.8.0; +pragma solidity ^0.8.1; import "./IERC721.sol"; import "./IERC721Receiver.sol"; import "./extensions/IERC721Metadata.sol"; -import "../../utils/Address.sol"; import "../../utils/Context.sol"; import "../../utils/Strings.sol"; import "../../utils/introspection/ERC165.sol"; @@ -17,7 +16,6 @@ import "../../utils/introspection/ERC165.sol"; * {ERC721Enumerable}. */ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { - using Address for address; using Strings for uint256; // Token name @@ -402,7 +400,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { uint256 tokenId, bytes memory data ) private returns (bool) { - if (to.isContract()) { + if (to.code.length > 0) { try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) { return retval == IERC721Receiver.onERC721Received.selector; } catch (bytes memory reason) { diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index e6843f1fa12..fe67032efa6 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.8.0) (token/ERC721/extensions/ERC721Consecutive.sol) -pragma solidity ^0.8.0; +pragma solidity ^0.8.1; import "../ERC721.sol"; import "../../../interfaces/IERC2309.sol"; @@ -86,7 +86,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { // minting a batch of size 0 is a no-op if (batchSize > 0) { - require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); + require(address(this).code.length == 0, "ERC721Consecutive: batch minting restricted to constructor"); require(to != address(0), "ERC721Consecutive: mint to the zero address"); require(batchSize <= _maxBatchSize(), "ERC721Consecutive: batch too large"); @@ -112,7 +112,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. */ function _mint(address to, uint256 tokenId) internal virtual override { - require(Address.isContract(address(this)), "ERC721Consecutive: can't mint during construction"); + require(address(this).code.length > 0, "ERC721Consecutive: can't mint during construction"); super._mint(to, tokenId); } diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index c1503c4dfc3..1f426c3baec 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.8.0) (token/ERC777/ERC777.sol) -pragma solidity ^0.8.0; +pragma solidity ^0.8.1; import "./IERC777.sol"; import "./IERC777Recipient.sol"; @@ -472,7 +472,10 @@ contract ERC777 is Context, IERC777, IERC20 { if (implementer != address(0)) { IERC777Recipient(implementer).tokensReceived(operator, from, to, amount, userData, operatorData); } else if (requireReceptionAck) { - require(!to.isContract(), "ERC777: token recipient contract has no implementer for ERC777TokensRecipient"); + require( + to.code.length == 0, + "ERC777: token recipient contract has no implementer for ERC777TokensRecipient" + ); } } diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index 433a866d71d..e08cdc75492 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -7,44 +7,6 @@ pragma solidity ^0.8.1; * @dev Collection of functions related to the address type */ library Address { - /** - * @dev Returns true if `account` is a contract. - * - * [IMPORTANT] - * ==== - * It is unsafe to assume that an address for which this function returns - * false is an externally-owned account (EOA) and not a contract. - * - * Among others, `isContract` will return false for the following - * types of addresses: - * - * - an externally-owned account - * - a contract in construction - * - an address where a contract will be created - * - an address where a contract lived, but was destroyed - * - * Furthermore, `isContract` will also return true if the target contract within - * the same transaction is already scheduled for destruction by `SELFDESTRUCT`, - * which only has an effect at the end of a transaction. - * ==== - * - * [IMPORTANT] - * ==== - * You shouldn't rely on `isContract` to protect against flash loan attacks! - * - * Preventing calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets - * like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract - * constructor. - * ==== - */ - function isContract(address account) internal view returns (bool) { - // This method relies on extcodesize/address.code.length, which returns 0 - // for contracts in construction, since the code is only stored at the end - // of the constructor execution. - - return account.code.length > 0; - } - /** * @dev Replacement for Solidity's `transfer`: sends `amount` wei to * `recipient`, forwarding all available gas and reverting on errors. @@ -200,9 +162,9 @@ library Address { ) internal view returns (bytes memory) { if (success) { if (returndata.length == 0) { - // only check isContract if the call was successful and the return data is empty + // only check if target is a contract if the call was successful and the return data is empty // otherwise we already know that it was a contract - require(isContract(target), "Address: call to non-contract"); + require(target.code.length > 0, "Address: call to non-contract"); } return returndata; } else { diff --git a/contracts/utils/StorageSlot.sol b/contracts/utils/StorageSlot.sol index 6ab8f5dc6bc..5941ee9d418 100644 --- a/contracts/utils/StorageSlot.sol +++ b/contracts/utils/StorageSlot.sol @@ -21,7 +21,7 @@ pragma solidity ^0.8.0; * } * * function _setImplementation(address newImplementation) internal { - * require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract"); + * require(newImplementation.code.length > 0, "ERC1967: new implementation is not a contract"); * StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation; * } * } diff --git a/docs/modules/ROOT/nav.adoc b/docs/modules/ROOT/nav.adoc index 6604c2de58f..a6249d3345b 100644 --- a/docs/modules/ROOT/nav.adoc +++ b/docs/modules/ROOT/nav.adoc @@ -19,3 +19,5 @@ * xref:crosschain.adoc[Crosschain] * xref:utilities.adoc[Utilities] + +* xref:fag.adoc[FAQ] diff --git a/docs/modules/ROOT/pages/faq.adoc b/docs/modules/ROOT/pages/faq.adoc new file mode 100644 index 00000000000..81c34bbf562 --- /dev/null +++ b/docs/modules/ROOT/pages/faq.adoc @@ -0,0 +1,13 @@ += Frequently Asked Questions + +== Can I restrict a function to EOAs only? + +When calling external addresses from your contract it is unsafe to assume that an address is an externally-owned account (EOA) and not a contract. Attempting to prevent calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract constructor. + +Although checking that the address has code, `address.code.length > 0`, may seem to differentiate contracts from EOAs, it can only say that an address is currently a contract, and its negation (that an address is not currently a contract) does not imply that the address is an EOA. Some counterexamples are: + + - address of a contract in construction + - address where a contract will be created + - address where a contract lived, but was destroyed + +Furthermore, an address will be considered a contract within the same transaction where it is scheduled for destruction by `SELFDESTRUCT`, which only has an effect at the end of the entire transaction. diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index 4231a6a746e..88207f0e109 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -99,8 +99,6 @@ If you need support for more powerful collections than Solidity's native arrays [[misc]] == Misc -Want to check if an address is a contract? Use xref:api:utils.adoc#Address[`Address`] and xref:api:utils.adoc#Address-isContract-address-[`Address.isContract()`]. - Want to keep track of some numbers that increment by 1 every time you want another one? Check out xref:api:utils.adoc#Counters[`Counters`]. This is useful for lots of things, like creating incremental identifiers, as shown on the xref:erc721.adoc[ERC721 guide]. === Base64 diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index a78ae14e641..2f8115c165c 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -12,16 +12,6 @@ contract('Address', function (accounts) { this.mock = await Address.new(); }); - describe('isContract', function () { - it('returns false for account address', async function () { - expect(await this.mock.$isContract(other)).to.equal(false); - }); - - it('returns true for contract address', async function () { - expect(await this.mock.$isContract(this.mock.address)).to.equal(true); - }); - }); - describe('sendValue', function () { beforeEach(async function () { this.recipientTracker = await balance.tracker(recipient);