convertToPrincipal() and previewWithdraw() have got inconsistent implementation which violates EIP-5095

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L488-L490
tps://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L446-L456

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 implement convertToPrincipal() function. Both convertToPrincipal() and previewWithdraw() implementation should be consistent, according to EIP-5095 any unfavorable discrepancy between convertToPrincipal and previewWithdraw SHOULD be considered slippage in price-per-principal-token or some other type of condition. However, convertToPrincipal() implementation differs too much from the previewWithdraw() - thus its implementation should be classified as incorrect one. The implementation of convertToPrincipal() should be changed.

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

According to EIP-5095:

Note that any unfavorable discrepancy between convertToPrincipal and previewWithdraw SHOULD be considered slippage in price-per-principal-token or some other type of condition.

This suggests, that convertToPrincipal() and previewWithdraw() should have a similar implementation - while any deviation/discrepancy should be considered slippage.

Function convertToPrincipal() is defined as below:

File: PrincipalToken.sol

    function convertToPrincipal(uint256 underlyingAmount) external view override returns (uint256) {
        return _convertIBTsToShares(IERC4626(ibt).previewDeposit(underlyingAmount), false);
    }

Function previewWithdraw() is defined as below:
File: PrincilapToken.sol

    function previewWithdraw(
        uint256 assets
    ) external view override whenNotPaused returns (uint256) {
        uint256 ibts = IERC4626(ibt).previewWithdraw(assets);
        return previewWithdrawIBT(ibts);
    }

    /** @dev See {IPrincipalToken-previewWithdrawIBT}. */
    function previewWithdrawIBT(uint256 ibts) public view override whenNotPaused returns (uint256) {
        return _convertIBTsToShares(ibts, true);
    }

which is basically the same as:

    function previewWithdraw(
        uint256 assets
    ) external view override whenNotPaused returns (uint256) {
        return _convertIBTsToShares(IERC4626(ibt).previewWithdraw(assets), true)
    }

This can be summarized as:

• convertToPrincipal(): _convertIBTsToShares(IERC4626(ibt).previewDeposit(X), false);
• previewWithdraw(): _convertIBTsToShares(IERC4626(ibt).previewWithdraw(X), true)

The first function uses ERC4626 previewDeposit() and rounds down, while the second function uses previewWithdraw() and rounds up.

Since for ERC4626 the inverse of the deposit() function is a withdraw() function - it suggests that convertToPrincipal() is not correctly implemented. Since according to EIP-5095 discrepancy between convertToPrincipal and previewWithdraw SHOULD be considered slippage, those values should be just slightly different - however, they aren't (previewDeposit() and previewWithdraw() returns different values).

It's obvious, that the implementation of convertToPrincipal() is incorrect and previewDeposit() should be changed to previewWithdraw().

Tools Used

Manual code review

Recommended Mitigation Steps

Function convertToPrincipal() should use previewWithdraw() instead of previewDeposit().

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

[c4-judge]

JustDravee marked the issue as not a duplicate

[c4-judge]

JustDravee marked the issue as satisfactory

[c4-judge]

JustDravee marked the issue as primary issue

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

The two following points explain why we dispute this issue.

I - Reason 1

The problem raised here is basically the difference between:

    function convertToPrincipal(uint256 underlyingAmount) external view override returns (uint256) {
        return _convertIBTsToShares(IERC4626(ibt).previewDeposit(underlyingAmount), false);
    }

and

    function previewWithdraw(
        uint256 assets
    ) external view override whenNotPaused returns (uint256) {
        return _convertIBTsToShares(IERC4626(ibt).previewWithdraw(assets), true)
    }

in respect with this rule of the 5095: "Note that any unfavorable discrepancy between convertToPrincipal and previewWithdraw SHOULD be considered slippage in price-per-principal-token or some other type of condition."

The first difference comes from a call to the IBT's previewDeposit method vs a call to the IBT's previewWithdraw method. That being said, the IBT must follow the erc4626 and using the same argument as the auditor:
"Note that any unfavorable discrepancy between convertToShares and previewDeposit SHOULD be considered slippage in share price or some other type of condition, meaning the depositor will lose assets by depositing." (as a rule for previewDeposit)
"Note that any unfavorable discrepancy between convertToShares and previewWithdraw SHOULD be considered slippage in share price or some other type of condition, meaning the depositor will lose assets by depositing." (as a rule for previewWithdraw)

Then, the second difference comes from the rounding up vs rounding down. This slight difference is irrelevant and can be considered as "slippage in price-per-principal-token or some other type of condition." to respect the standards.

II - Reason 2

Also, when it comes to the erc5095 standard, you can find the following information concerning the convert methods:

• "This calculation MAY NOT reflect the “per-user” price-per-principal-token, and instead should reflect the “average-user’s” price-per-principal-token, meaning what the average user should expect to see when exchanging to and from."
• "The convertToUnderlying method is an estimate useful for display purposes, and do not have to confer the exact amount of underlying assets their context suggests." which comes quite naturally from the additional information that is found in the ERC4626: "The convertTo functions serve as rough estimates that do not account for operation specific details like withdrawal fees, etc. They were included for frontends and applications that need an average value of shares or assets, not an exact value possibly including slippage or other fees. For applications that need an exact value that attempts to account for fees and slippage we have included a corresponding preview function to match each mutable function.".

Conclusion

Therefore, we are confident on the fact that we do follow the standard and we dispute the issue.

[c4-judge]

JustDravee marked the issue as unsatisfactory:
Invalid