Inconsistent Implementation for supported asset token decimal

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L304-L305

Vulnerability details

Impact

In the creation of PrizeVault, if _tryGetAssetDecimals fails, the decimal is directly regarded as 18. This can be incompatible with some tokens that haven't implemented the decimal function and is not intended to be used as 18 decimals.

Proof of Concept

In the constructor of PrizeVault, the _tryGetAssetDecimals is called to get the decimal of the underlying asset. However, if the call fails, the decimal is presumed to be 18.

        IERC20 asset_ = IERC20(yieldVault_.asset());
        (bool success, uint8 assetDecimals) = _tryGetAssetDecimals(asset_);
        _underlyingDecimals = success ? assetDecimals : 18;

This is incompatible with some tokens(ex. a valid token that hasn't implemented the decimals function and is not intended to be used as 18 decimals). Since decimals is not a standard function in EIP-20 but is added later.

As a result, this may cause mis-information and cause potential loss for users.

Tools Used

Manual

Recommended Mitigation Steps

It's proper to simply revert when _tryGetAssetDecimals fails instead of pre-assuming all tokens that haven't implemented decimals function to be all 18 decimals.

Assessed type

MEV

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #25

[raymondfam]

ERC20 token not having decimals() implemented is prone to incorrect assignment to _underlyingDecimals. Although PrizeVault.decimals() isn't used in the contract, it could provide faulty info to the caller dependent on it.

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

trmid (sponsor) confirmed

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report

[wangxx2026]

Hi @hansfriese,I think this submission should be considered as QA.
This writeup comes from OZ, and tokens that don't implement decimals are rare anymore

[jes16jupyter]

Hi, just want provide some additional information if needed:

1. There are indeed some tokens with 0 decimals that are still active. For example, DGD, CloutContracts. I agree that it's rare, yet in EIP20 it says that OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.
2. Different Orgs are treating this issue differently.
   1. Etherscan is taking 0 decimals for erc20 tokens without decimals implementation.
   2. Metamask will need user to specify the decimal for erc20 tokens without decimals implementation.

Thanks.

[hansfriese]

While reconsidering the severity, the only impact is that PrizeVault.decimals() will return incorrect decimals and users won't incur any losses during deposit/withdrawal.
I think it's fair to downgrade to QA.

[c4-judge]

hansfriese marked the issue as not selected for report

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c

[trmid]

mitigation: GenerationSoftware/pt-v5-vault#95