Add warning on SELFDESTRUCT usage with isContract

[pcaversaccio]

As title. An interesting scenario in the context of upgradable contracts that could be "exploited" (not sure this is the correct wording tho) is the following:

• deploy a new implementation contract including a selfdestruct possibility;
• upgrade the implementation and call in the same transaction selfdestruct;
  -> newImplementation == EOA

The check require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract"); would be successful in such a case.

[frangio]

This looks like a warning that should be part of the NatSpec rather than a part of the implementation comments. To be honest even the existing comment that we have inside the function seems redundant too.

[pcaversaccio]

I actually agree - I was not sure however what you planned to change for the upcoming releases. You want me to put it into the NatSpec section instead?

[frangio]

The plan is to remove this function in 5.0 (#3417) but that is still a couple of months away. In any case we will want to keep the docs about how this kind of check can cause issues.

Yes I think we should move this to the NatSpec... I'm not sure exactly where it fits, I feel it's somewhat covered already where it says "isContract will return false for ... an address where a contract lived, but was destroyed". I guess the new observation is that it can return true even if the contract is already scheduled for selfdestruction?

[pcaversaccio]

Right, in theory, it's nothing new that you can interact with a selfdestructed contract within the same transaction, but many people don't know about this behavior, to be honest. That's why I want to cover the true case as well (informing people about possible foot guns is very important). What do you think about the following NatSpec?

     * [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, this method returns `true` for any predeployed `account`
     * contract that is destroyed using `SELFDESTRUCT` within the same transaction
     * where `isContract` is used. This is possible because the contract code of
     * `account` is destroyed at the _end_ of the current 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;
    }

[frangio]

I would go with "Furthermore, this function will return true even if the contract is already scheduled for destruction via SELFDESTRUCT, which has an effect only at the end of a transaction."

[pcaversaccio]

quickly pushed an adjusted version of yours: "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." I think it's important to emphasise the within the same transaction. I also quickly fixed the merge conflict. If you don't agree on my final wording, feel free to adjust it according to your opinion :-D. I'm not too picky on this, just important that we reflect it the codebase.

[frangio]

Thanks! Docs changes don't need to go in the changelog.