Reduce reliance on underlying decimals in ERC4626

[thedavidmeister]

4626 uses _asset.decimals() to calculate initial share/asset conversions

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L167

EIP4626 states:

All ERC-4626 tokenized Vaults MUST implement ERC-20’s optional metadata extensions.

But EIP20 states for decimals:

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

So even if the vault exposes decimals for shares, the 20 spec says not to expect assets to expose decimals.

Even if both tokens expose decimals the method is for usability, not for onchain calculations.

Maybe a virtual _initialShareRatio() function could be exposed that implementing contracts can override without needing to override the entire conversion process

[frangio]

This is a good point, we can't assume decimals is present. For ERC20Wrapper we handled it properly:

openzeppelin-contracts/contracts/token/ERC20/extensions/ERC20Wrapper.sol

Lines 28 to 34 in 6ab8d6a

	function decimals() public view virtual override returns (uint8) {
	try IERC20Metadata(address(underlying)).decimals() returns (uint8 value) {
	return value;
	} catch {
	return super.decimals();
	}
	}

Even if both tokens expose decimals the method is for usability, not for onchain calculations.

This is also a very good point.

I agree with adding a function like _initialShareRatio() returns (uint shares, uint assets).

[Amxx]

From the ERC:

Although the convertTo functions should eliminate the need for any use of an ERC-4626 Vault’s decimals variable, it is still strongly recommended to mirror the underlying token’s decimals if at all possible, to eliminate possible sources of confusion and simplify integration across front-ends and for other off-chain users.

It would be easy to just do that, but then what if someone overrides our 4626 with a custom decimal that doesn't not match the underlying asset. Should we make the decimal in 4626 "non-virtual override" ?

[Amxx]

So we could do

    function decimals() public view override(IERC20Metadata, ERC20) returns (uint8) {
        try IERC20Metadata(address(_asset)).decimals() returns (uint8 value) {
            return value;
        } catch {
            return super.decimals();
        }
    }
    
    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? assets
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        return
            (supply == 0)
                ? shares
                : shares.mulDiv(totalAssets(), supply, rounding);
    }

and no possibility to override decimal ... or

    function decimals() public view virtual override(IERC20Metadata, ERC20) returns (uint8) {
        try IERC20Metadata(address(_asset)).decimals() returns (uint8 value) {
            return value;
        } catch {
            return super.decimals();
        }
    }
    
    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? assets.mulDiv(10**decimals(), 10**ERC4626.decimals(), rounding)
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        return
            (supply == 0)
                ? shares.mulDiv(10**ERC4626.decimals(), 10**decimals(), rounding)
                : shares.mulDiv(totalAssets(), supply, rounding);
    }

That way devs can still override

[frangio]

I don't understand the second option, can you explain it in words?

We can add the implementation of decimals based on ERC20Wrapper, but I think we should also add _initialAssetToShareRatio().

[thedavidmeister]

maybe this then:

    function decimals() public view override(IERC20Metadata, ERC20) returns (uint8) {
        try IERC20Metadata(address(_asset)).decimals() returns (uint8 value) {
            return value;
        } catch {
            return super.decimals();
        }
    }

    function _initialConvertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        shares = assets;
    }

    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? _initialConvertToShares(assets, rounding)
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function _initialConvertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        assets = shares;
    }

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        return
            (supply == 0)
                ? _initialConvertToAssets(shares, rounding)
                : shares.mulDiv(totalAssets(), supply, rounding);
    }
    ```

[Amxx]

What are the _initialConvertToXxx for. They make the code more complexe, and I don't see a reason for that.

[Amxx]

I don't understand the second option, can you explain it in words?

We can add the implementation of decimals based on ERC20Wrapper, but I think we should also add _initialAssetToShareRatio().

The second option is:

• By default we use super.decimals()
• Unless we _asset implements the decimals(), in which case we use that → this is ERC4626.decimals() and that is our reference for the asset's decimals
• This can still be overridden in the child contract.

If the decimals are not overridden, then decimals and ERC4626.decimals() are equal, and the 4626 vault uses the same decimal as the underlying asset. In that case, the muldiv doesn't change anything (numerator and denominator are equals)

If the decimals are overridden, then decimals and ERC4626.decimals() are not equal. The first one is the decimals or the vault (obviously) while the second one is the decimals of the asset (with the fallback). In that case the muldiv does the scaling just like today.

IMO this is the most elegant approach.

EDIT: it could be rewritten as

    function decimals() public view virtual override(IERC20Metadata, ERC20) returns (uint8) {
        return _assetDecimals();
    }
    
    function _assetDecimals() internal view virtual returns (uint8) {
        try IERC20Metadata(address(_asset)).decimals() returns (uint8 value) {
            return value;
        } catch {
            return super.decimals(); // or 18?
        }
    }
    
    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? assets.mulDiv(10**decimals(), 10**_assetDecimals(), rounding)
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        return
            (supply == 0)
                ? shares.mulDiv(10**_assetDecimals(), 10**decimals(), rounding)
                : shares.mulDiv(totalAssets(), supply, rounding);
    }

[Amxx]

Also, any approach that makes decimals() non virtual would be in violation of our "make functions virtual" rule.

[pedrommaiaa]

So we could do

    function decimals() public view override(IERC20Metadata, ERC20) returns (uint8) {
        try IERC20Metadata(address(_asset)).decimals() returns (uint8 value) {
            return value;
        } catch {
            return super.decimals();
        }
    }
    
    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? assets
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        return
            (supply == 0)
                ? shares
                : shares.mulDiv(totalAssets(), supply, rounding);
    }

and no possibility to override decimal ... or

    function decimals() public view virtual override(IERC20Metadata, ERC20) returns (uint8) {
        try IERC20Metadata(address(_asset)).decimals() returns (uint8 value) {
            return value;
        } catch {
            return super.decimals();
        }
    }
    
    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
        uint256 supply = totalSupply();
        return
            (assets == 0 || supply == 0)
                ? assets.mulDiv(10**decimals(), 10**ERC4626.decimals(), rounding)
                : assets.mulDiv(supply, totalAssets(), rounding);
    }

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        return
            (supply == 0)
                ? shares.mulDiv(10**ERC4626.decimals(), 10**decimals(), rounding)
                : shares.mulDiv(totalAssets(), supply, rounding);
    }

That way devs can still override

Really like the first option @Amxx