Incorrect maturity calculations for YieldToken

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/YieldToken.sol#L124

Vulnerability details

Impact

According to the provided documentation: Principal Token is ERC5095. This is the main invariant defined in the project's description:

File: README.md

This is the core contract of Spectra. The Principal Token is [EIP-5095](https://eips.ethereum.org/EIPS/eip-5095)

File: README.md

**Principal Token is ERC5095**

During the code-review process, some deviations from the EIP-5095 had been detected. Lack of compliance may cause unexpected behavior. Other protocols that integrate with contract may incorrectly assume that it's EIP-5095 compliant - especially that documentation states that it's ERC-5095. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a contract and describes it as ERC-5095, it should fully conform to ERC-5095 standard.

The current implementation does not correctly define the maturity. According to EIP-5095: Principal Tokens become redeemable for underlying at or after this timestamp. However, the current implementation treats tokens at the timestamp as non-mature (even though, according to EIP-5095, they are matured).

During the previous C4 contests, lack of EIP compliance was usually evaluated as High/Medium

• (Lack of EIP-4626 compliance) Vault.sol is not EIP-4626 compliant  2022-09-y2k-finance-findings#47 - has been evaluated as High
• (Lack of EIP-4524 compliance) HolographERC20 breaks composability by forcing usage of draft proposal EIP-4524 2022-10-holograph-findings#440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) HolographERC721.safeTransferFrom not compliant with EIP-721 2022-10-holograph-findings#203 - has been evaluated as Medium

Proof of Concept

EIP-5095 defines, that:

maturity: The timestamp (unix) at which a Principal Token matures. Principal Tokens become redeemable for underlying at or after this timestamp.

Now, let's take a look how a matuirity is being check:

File: YieldToken.sol

return (block.timestamp < IPrincipalToken(pt).maturity()) ? super.balanceOf(account) : 0;

According to above EIP, when block.timestamp == IPrincipalToken(pt).maturity(), PT reaches the maturity, thus function balanceOf() should return super.balanceOf(account) instead of 0.
The current implementation, however, when block.timestamp == IPrincipalToken(pt).maturity() returns 0. This violates the EIP-5095, because PT reaches maturity not only after the timestamp, but: at or after this timestamp. Thus, PT should be treated as matured when block.timestamp == IPrincipalToken(pt).maturity().

Tools Used

Manual code review

Recommended Mitigation Steps

Line: return (block.timestamp < IPrincipalToken(pt).maturity()) ? super.balanceOf(account) : 0;
should be changed to:
return (block.timestamp <= IPrincipalToken(pt).maturity()) ? super.balanceOf(account) : 0;

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #33

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[JustDravee]

There seem to be a misunderstanding between < and >.

Here, it returns the balance strictly before maturity and 0 at and after maturity

    function balanceOf(
        address account
    ) public view override(IYieldToken, ERC20Upgradeable) returns (uint256) {
        return (block.timestamp < IPrincipalToken(pt).maturity()) ? super.balanceOf(account) : 0;
    }

LGTM

[c4-judge]

JustDravee marked the issue as not a duplicate

[c4-judge]

JustDravee marked the issue as unsatisfactory:
Invalid