PT must not expect decimals() of the underlying ERC20 to be present

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L142 https://github.com/code-423n4/2024-02-spectra/blob/main/src/libraries/PrincipalTokenUtil.sol#L137-L148

Vulnerability details

Description

In the documentation it is explained that:

The protocol can only interact with IBTs between 6 and 18 decimals (inclusive).

And:

The protocol is expected to interact with any ERC4626 compliant vault.

The EIP-4626 explains that

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

But the underlying ERC20 decimals() function is optional. So a compliant ERC4626 vault could return a decimal but the underlying asset not. The protocol should work with an underlying ERC20 without decimals() function since the protocol is expected to interact with any ERC4626 compliant vault.

But on the intialize function, it calls the decimals() function of the underlying:

_assetDecimals = PrincipalTokenUtil._tryGetTokenDecimals(_asset);

In the case where there is no decimals function it reverts.

    function _tryGetTokenDecimals(address _token) external view returns (uint8) {
        (bool success, bytes memory encodedDecimals) = _token.staticcall(
            abi.encodeCall(IERC20Metadata.decimals, ())
        );
        if (success && encodedDecimals.length >= 32) {
            uint256 returnedDecimals = abi.decode(encodedDecimals, (uint256));
            if (returnedDecimals <= type(uint8).max) {
                return uint8(returnedDecimals);
            }
        }
        revert AssetDoesNotImplementMetadata();
    }

Moreover, even in the case where the decimals function of the underlying asset returns a value which is not between 6 and 18, the conversion in the Ray.sol library could revert or not be as precise as wanted (the library's description says "Library for number conversions from decimals between 6 and 18 to 27 decimals (ray)").

ibtRate = IERC4626(ibt).previewRedeem(ibtUnit).toRay(_assetDecimals);

Tools Used

Manual review

Recommended Mitigation Steps

There are 2 differents solutions.

The first solution is to change documentation to warn developers. They need to know that :

The protocol can only interact with IBTs between 6 and 18 decimals (inclusive) AND with an underlying asset which implement the decimals() function which returns a uint8 between 6 and 18 decimals.

The second solution is to consider a default value for assets with no decimals. For example, the IBT's decimals can be used in this case. In the constructor:

        _ibtDecimals = IERC4626(_ibt).decimals();
        try _assetDecimals = PrincipalTokenUtil._tryGetTokenDecimals(_asset); {
            return;
        } catch {
            _assetDecimals = _ibtDecimals;
        }

Before pushing this modification, it should make sure that it is not missleading for users and then to cause bad behaviors.

Moreover, a full logic may be implemented to make sure if _assetDecimals is not between 6 and 18, it does not cause troubles.

Assessed type

ERC20

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

If the ERC20 asset does not implement the decimals() method (which is optional indeed) then our PT initialization would revert, which is the intended behavior.

Therefore, we do not consider this to be an issue and dispute it.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid