Openzeppelin-contracts: Reliability of the truthiness in `isContract` method

[corydickson]

When reviewing this EIP, it is unclear to me if someone were to selfdestruct a contract in the same transaction it was created that this function would still return True. Is this intended behavior?

I feel like this would lead to some obscure bugs, namely contracts that behave in test cases 5/6 described above.

[abcoathup]

Hi @corydickson! We have been able to reproduce this issue by following these steps:

isContract returns true for a contract that is created and self destructed in the same transaction. isContract returns false in subsequent transactions.

Target.sol

pragma solidity ^0.6.0;

contract Target {
    function destroy() public {
        selfdestruct(msg.sender);
    }
}

Check.sol

pragma solidity ^0.6.0;

import "@openzeppelin/contracts/utils/Address.sol";
import "./Target.sol";

contract Check {
    using Address for address;

    Target public target;

    event Status(bool newValue);

    function check() public {
        target = new Target();
        target.destroy();

        emit Status(address(target).isContract());
    }

    function check2() public {
        emit Status(address(target).isContract());
    }
}

$ npx oz deploy
✓ Compiled contracts with solc 0.6.6 (commit.6c089d02)
? Choose the kind of deployment regular
? Pick a network development
? Pick a contract to deploy Check
✓ Deployed instance of Check
0xe78A0F7E598Cc8b0Bb87894B0F60dD2a88d6a8Ab

$ npx oz send-tx
? Pick a network development
? Pick an instance Check at 0xe78A0F7E598Cc8b0Bb87894B0F60dD2a88d6a8Ab
? Select which function check()
✓ Transaction successful. Transaction hash: 0xa42a2af5798d541f87b5293d377ed012a9ba96f6c6e7138d5c0fdc8480413b54
Events emitted:
 - Status(true)

$ npx oz send-tx
? Pick a network development
? Pick an instance Check at 0xe78A0F7E598Cc8b0Bb87894B0F60dD2a88d6a8Ab
? Select which function check2()
✓ Transaction successful. Transaction hash: 0x2f84c974256cf10d5741f90994296ba852759d585265615f6334b60887814433
Events emitted:
 - Status(false)

Thanks so much for reporting it! The project owner will review and triage this issue during the next week.

[frangio]

Thank you for testing this @abcoathup.

@corydickson Can you please rephrase in more detail what is the issue that you see here? I'm not sure I understand.

The semantics of isContract are a bit strange and may be unintuitive, but it's the way the EVM works. We do have a warning in the docs. If you have any suggestions on how the docs could be improved let us know.

[corydickson]

Thanks both @abcoathup @frangio for reaching out to address the concern. It more has to do with the naming of the API and in the documentation it says:

Among others, isContract will return false for the following types of addresses:
-- an address where a contract lived, but was destroyed

But the test case provided shows how if in the same tx a contract is created/selfdestructed this check still returns True. Could we update the docs to reflect this, or maybe change the name of this function to be hasHadCodeDeployed or something in that vein

[nventuro]

I dug a bit into EIPs 1052 and 161 to better understand how selfdestruct interacts with extcodehash, but couldn't really find what I was looking for.

However, I think the current behavior, if perhaps strange, is still valid and useful. Any account for which isContract returns true is a contract and not an EOA. There's a number of reasons that can cause the check to return false, but a return value of true _does_ provide useful information. This is similar to how you can prove an account is an EOA by checking a signature, but cannot prove that it is not an EOA (other than proving it is a contract).

We may want to add a note to that effect to our docs.

[frangio]

The name hasHadCodeDeployed would not be accurate, because a contract that has been selfdestructed indeed has had code deployed at some point, but doesn't anymore. We want isContract to return false in that situation.

[corydickson]

Got it so sounds like we're aligned about the "desired" functionality behind the API semantics. Not sure how to achieve this in the EVM either, but as @nventuro suggested maybe updating the docs is all that can be done for now.

[frangio]

I believe those are the semantics that we have now! Unfortunately we're not testing for the more complex scenarios involving contract creation and selfdestruct, but it would be nice to incorporate that into the test suite.