ArrakisMetaVaultFactory can't deploy pools with non-string symbol ERC20 tokens

Summary

When deploying new pools in the ArrakisMetaVaultFactory, the function _getPublicVaultConstructorPayload() in order to get the token name and symbol, which will be used the creation code of the new vault, this function proceeds to call the getTokenName() and getTokenSymbol() functions which query the name() and symbol() functions on the ERC20 contract. The problem is two-fold:

It assumes that the symbol returned would be a string, but this is optional for ERC20 standard, there are tokens which return a bytes32 instead of a string.
It's implemented in a try-catch block in which the "catch" is empty, which would mean that if the symbol() function fails, the catch block won't execute alternative logic leading to the failure of the vault deployment.

Vulnerability Detail

Within the ERC20 standard, the symbol() is optional, as seen from here:

symbol
Returns the symbol of the token. E.g. “HIX”.

OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.

As a frequent example of a differently implemented symbol function is the MKR token.

As seen in the _getPublicVaultConstructorPayload() function, it expects a string to be returned when querying the symbol, and it implements an empty catch block:


function _getPublicVaultConstructorPayload(
        address timeLock_,
        address token0_,
        address token1_
    ) internal view returns (bytes memory) {
        string memory name = "Arrakis Modular Vault";
        string memory symbol = "AMV";

        try this.getTokenName(token0_, token1_) returns (
            string memory result
        ) {
            name = result;
        } catch {} // solhint-disable-line no-empty-blocks

        try this.getTokenSymbol(token0_, token1_) returns (
            string memory result
        ) {
            symbol = result;
        } catch {} // solhint-disable-line no-empty-blocks

        return abi.encode(
            timeLock_,
            name,
            symbol,
            moduleRegistryPublic,
            manager,
            token0_,
            token1_
        );
    }

Impact

The deployment of vaults for tokens which have a differently implemented symbol() function (i.e. doesn't return a string) will fail.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisMetaVaultFactory.sol#L445-L470

Tool used

Manual Review

Recommendation

Implement some kind of a logic to be executed in the catch block in case the try fails.

sherlock-audit / 2024-03-arrakis-judging

iamandreiski - ArrakisMetaVaultFactory can't deploy pools with non-string symbol ERC20 tokens #31