`Address.isContract()` is not a reliable way of checking if the input is a contract

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/assertionStakingPool/StakingPoolCreatorUtils.sol#L16

Vulnerability details

The underlying assumption of pool being an contract can be untrue.

Proof of Concept

The AssertionStakingPoolCreator or EdgeStakingPoolCreator contract invokes getPool() which calls the function inside the StakingPoolCreatorUtils library.

File: StakingPoolCreatorUtils.sol

    if (Address.isContract(pool)) {                                                   
            return pool;
        } else {
            revert PoolDoesntExist();
        }

which calls:

File: Address.sol

    * 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;
    }

However, this check can fail even though input is a smart contract if the function is called in the constructor. Address.isContract() checks for the code length, but during construction code length is 0. Thus the call would revert in this scenario.

Tools Used

Manual Review

Recommended Mitigation Steps

Using require(msg.sender == tx.orign) to test whether an address is a contract since tx.origin can never be a contract. Although the use of tx.origin is not a suitable practice, but its a better alternative.

Assessed type

Invalid Validation

[Shubh0412]

Hello @Picodes

According to the issue described in the report & the natspec of the use of isContract(), possible reverts could take place whenever the getPool() is called. Thus, its use could not be relied upon.

[Picodes]

@Shubh0412 what you're saying is technically true - isContract isn't reliable as it'd revert if called within the constructor. But you need to show how this could have any impact in the context of the codebase. Here it looks like there is absolutely no chance that this function is called within a constructor.