Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Address.isContract #3945

Merged
merged 20 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* `ProxyAdmin`: Removed `getProxyAdmin` and `getProxyImplementation` getters. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820))
* `ERC20`, `ERC1155`: Deleted `_beforeTokenTransfer` and `_afterTokenTransfer` hooks, added a new internal `_update` function for customizations, and refactored all extensions using those hooks to use `_update` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838), [#3876](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876))
* `ERC165Storage`: Removed this contract in favor of inheritance based approach. ([#3880](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3880))
* `Address`: Removed `isContract` because of its ambiguous nature and potential for misuse. ([#3945](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3945))

### How to upgrade from 4.x

Expand Down
4 changes: 0 additions & 4 deletions contracts/mocks/AddressImpl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ contract AddressImpl {

event CallReturnValue(string data);

function isContract(address account) external view returns (bool) {
return Address.isContract(account);
}

function sendValue(address payable receiver, uint256 amount) external {
Address.sendValue(receiver, amount);
}
Expand Down
4 changes: 2 additions & 2 deletions contracts/mocks/UUPS/UUPSLegacy.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;
pragma solidity ^0.8.1;

import "./UUPSUpgradeableMock.sol";

Expand All @@ -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;
}

Expand Down
6 changes: 3 additions & 3 deletions contracts/proxy/ERC1967/ERC1967Upgrade.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down
5 changes: 2 additions & 3 deletions contracts/proxy/beacon/UpgradeableBeacon.sol
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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;
}
}
2 changes: 1 addition & 1 deletion contracts/proxy/utils/Initializable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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),
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
"Initializable: contract is already initialized"
);
_initialized = 1;
Expand Down
9 changes: 3 additions & 6 deletions contracts/token/ERC1155/ERC1155.sol
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -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;

Expand Down Expand Up @@ -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");
Expand All @@ -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
) {
Expand Down
6 changes: 2 additions & 4 deletions contracts/token/ERC721/ERC721.sol
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions contracts/token/ERC721/extensions/ERC721Consecutive.sol
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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");
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
require(to != address(0), "ERC721Consecutive: mint to the zero address");
require(batchSize <= _maxBatchSize(), "ERC721Consecutive: batch too large");

Expand All @@ -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);
}

Expand Down
7 changes: 5 additions & 2 deletions contracts/token/ERC777/ERC777.sol
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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),
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
"ERC777: token recipient contract has no implementer for ERC777TokensRecipient"
);
}
}

Expand Down
38 changes: 2 additions & 36 deletions contracts/utils/Address.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,40 +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
* ====
*
* [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.
* ====
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a new file at docs/modules/ROOT/pages/knowledge.adoc, add it in docs/modules/ROOT/nav.adoc, and include this content there so we don't lose it. I think it should be a section called "Can I restrict a function to EOAs only?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the new doc page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of "FAQ" as the title for the section? Or you can check how I did it and if it can remain like that, no problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes FAQ works.

*/
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.
Expand Down Expand Up @@ -196,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 {
Expand Down
2 changes: 1 addition & 1 deletion contracts/utils/StorageSlot.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
* }
* }
Expand Down
2 changes: 0 additions & 2 deletions docs/modules/ROOT/pages/utilities.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions test/utils/Address.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,6 @@ contract('Address', function (accounts) {
this.mock = await AddressImpl.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 () {
const contract = await AddressImpl.new();
expect(await this.mock.isContract(contract.address)).to.equal(true);
});
});

describe('sendValue', function () {
beforeEach(async function () {
this.recipientTracker = await balance.tracker(recipient);
Expand Down