Deploying an ERC4626 vault with an asset without code reverts in v4.8.0-rc.0

[tinchoabbate]

In v4.7.0, deploying an ERC4626 vault for an asset with no code worked. Either for an asset that still hasn't been deployed, or an asset under construction.

In v4.8.0-rc.0, the same fails. As far as I can tell, this is due to the changes introduced in #3639.

Here's a Foundry test:

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
import "forge-std/Test.sol";

contract Vault is ERC4626 {
    constructor(IERC20Metadata asset) ERC20("vToken", "vToken") ERC4626(asset) {}
}

contract ExampleToken is ERC20 {
    Vault public vault;

    constructor() ERC20("Token", "Token") {
        vault = new Vault(
            IERC20Metadata(address(this))
        );
    }
}

contract VaultTest is Test {
    ExampleToken token;

    function test_deploy() public {
        // Deploy token that deploys vault
        // In 4.7.0 this works
        // In 4.8.0-rc.0 this fails
        token = new ExampleToken();
    }
}

The root cause seems to be that after #3639, the constructor of ERC4626 executes a try/catch to call the decimals function of the passed asset.

constructor(IERC20 asset_) {
        uint8 decimals_;
        try IERC20Metadata(address(asset_)).decimals() returns (uint8 value) { ... }

However, when the asset doesn't have code, the call to decimals returns no data. Solidity attempts to decode it as a uint8, but fails. This triggers a revert in the constructor of the ERC4626 contract, instead of going into the catch clause. This is explained by the Solidity docs:

If an error happens during the decoding of the return data inside a try/catch-statement, this causes an exception in the currently executing contract and because of that, it is not caught in the catch clause.

To make the error nicer, we could include a require in the constructor of the ERC4626 to ensure the asset has code. In any case, it seems this new behavior is worth including as a breaking change.

[frangio]

This feels like an edge case, but the use case presented in ExampleToken looks like something that could show up in practice, so I think we should try to support this.

The way that decimals are set for the vault is not properly documented and we need to improve that too.

I also have some concerns around calling super.decimals() in the constructor. I would prefer if we simplified the constructor to just make a best effort to retrieve and cache the decimals of the underlying, and set the cached value to 0 if anything fails. Then ERC4626.decimals() should decide whether to return the cached value if it's 0, and call super.decimals() otherwise.