isContract() Implementation is no longer valid

[tina1998612]

It was recently discovered that the account.code.length > 0 assertion for checking if an account is EOA is not valid. It can be exploited as shown in here: https://github.com/0xKitsune/Ghost-Contract

As demonstrated in this repo, a contract can use the CREATE opcode and execute an arbitrary payload, which can call another contract, transfer funds or anything else that a normal contract can do. When using this approach to call another contract, because the execution of this logic is within the constructor, the codesize of the msg.sender is 0.

One can use tx.origin == msg.sender to check whether the msg.sender is EOA. The isContract() method may need to be removed to avoid confusion.

Current implemetation:

openzeppelin-contracts/contracts/utils/Address.sol

Line 41 in afb2011

return account.code.length > 0;

[StillFantastic]

I think it is mentioned in the document to be careful with that case.

openzeppelin-contracts/contracts/utils/Address.sol

Lines 18 to 24 in c12076f

	* 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

#1212 also discussed the drawback of tx.origin approach.

[tina1998612]

hmm I see, it's still a bit misleading with the naming.

[frank890417]

Agree with @tina1998612 that isContract should consider cases like "a contract in construction", or it is not aligned with its function name and cannot help developers to avoid ghost contract calls.

[Amxx]

This as been discussed numerous times already:

• The isContract function is necessary for some operation such as ERC721 & ERC1155 safeTransfer hooks.
• Limitation of the isConstract function are clearly and extensively documented.