Protocol may be incompatible with some OpenZeppelin ERC4626 based vaults

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L41 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L107

Vulnerability details

Impact

Impossible to create principal tokens for OpenZeppelin vaults that has decimalsOffset > 0.

Proof of Concept

Since v4.9 OpenZeppelin introduced configurable virtual assets and shares in the form of _decimalsOffset to mitigate the risk of "donation" attacks. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L222-L234

    /**
     * @dev Internal conversion function (from assets to shares) with support for rounding direction.
     */
    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256) {
        return assets.mulDiv(totalSupply() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
    }

    /**
     * @dev Internal conversion function (from shares to assets) with support for rounding direction.

     */
    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256) {
        return shares.mulDiv(totalAssets() + 1, totalSupply() + 10 ** _decimalsOffset(), rounding);
    }

This way offset is added to the shares decimal count

    function decimals() public view virtual override(IERC20Metadata, ERC20) returns (uint8) {
        return _underlyingDecimals + _decimalsOffset();
    }

Assuming that most of underlying tokens has 18 decimals, offsets bigger than 0 would make such vaults incompatible with Spectra principal tokens, because there is a strict check on ERC4626 decimals that prevents the principal token initialization. https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L143-L148

        if (
            _assetDecimals < MIN_DECIMALS ||
            _assetDecimals > _ibtDecimals ||
>>          _ibtDecimals > MAX_DECIMALS
        ) {
            revert InvalidDecimals();
        }

Tools Used

Manual review

Recommended Mitigation Steps

Consider making exception for vaults with decimalsOffset function.

Assessed type

ERC4626

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[gzeon-c4]

more than 18 decimals is out of scope

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

more than 18 decimals is out of scope

As stated above, we accept only assets and IBTs with respective decimals dA and dIBT such that 6 <= dA <= dIBT <= 18.

We therefore dispute this issue.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid