Incorrect implementation of the EIP-5095 standard for covertToUnderlying()

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L492-L495
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L680-L693

Vulnerability details

Impact

The EIP-5095 standard states that the amount returned MUST NOT show any variations depending on the caller.

Yet, the convertToUnderlying function utilises previewDeposit.

When we check the previewDeposit EIP-4626 standard, it states that it MUST be inclusive of deposit fees. Since deposit fees can vary depending on the caller and the specific ERC4626 implementation (f.e. fee reduction as is used in the spectra protocol), it cannot be guaranteed that there is no variation in the amount returned for every caller.

The EIP-5095 standard also states that before maturity, the amount of underlying returned is as if the PTs would be at maturity.

However, the convertToUnderlying function utilises _convertIBTsToShares, which uses current rates. These rates are subject to change and have no relation to the rates at maturity.

Proof of Concept

From: EIP-5095

convertToUnderlying

The amount of underlying that would be exchanged for the amount of PTs provided, in an ideal scenario where all the conditions are met.

Before maturity, the amount of underlying returned is as if the PTs would be at maturity.

MUST NOT be inclusive of any fees that are charged against redemptions.

MUST NOT show any variations depending on the caller.

MUST NOT reflect slippage or other on-chain conditions, when performing the actual redemption.

MUST NOT revert unless due to integer overflow caused by an unreasonably large input.

MUST round down towards 0.

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.

PrincipalToken.sol

    /** @dev See {IPrincipalToken-convertToUnderlying}. */
    function convertToUnderlying(uint256 principalAmount) public view override returns (uint256) {
        return IERC4626(ibt).previewRedeem(_convertSharesToIBTs(principalAmount, false));
    }

IERC4626.sol

    /**
     * @dev Allows an on-chain or off-chain user to simulate the effects of their deposit at the current block, given
     * current on-chain conditions.
     *
     * - MUST return as close to and no more than the exact amount of Vault shares that would be minted in a deposit
     *   call in the same transaction. I.e. deposit should return the same or more shares as previewDeposit if called
     *   in the same transaction.
     * - MUST NOT account for deposit limits like those returned from maxDeposit and should always act as though the
     *   deposit would be accepted, regardless if the user has enough tokens approved, etc.
     * - MUST be inclusive of deposit fees. Integrators should be aware of the existence of deposit fees.
     * - MUST NOT revert.
     *
     * NOTE: 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.
     */
    function previewDeposit(uint256 assets) external view returns (uint256 shares);

    /**
     * @dev Converts amount of IBT to amount of PT shares with current rates
     * @param _ibts amount of IBT to convert to shares
     * @param _roundUp true if result should be rounded up
     * @return shares resulting amount of shares
     */
    function _convertIBTsToShares(
        uint256 _ibts,
        bool _roundUp
    ) internal view returns (uint256 shares) {
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
        if (_ptRate == 0) {
            revert RateError();
        }
        shares = _ibts.mulDiv(
            _ibtRate,
            _ptRate,
            _roundUp ? Math.Rounding.Ceil : Math.Rounding.Floor
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Change the function to not use previewDeposit and use the rates at maturity.

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 big confusion between convertToUnderlying and convertToPrincipal. Therefore the whole argument using EIP-5095 is wrong

[c4-judge]

JustDravee marked the issue as not a duplicate

[c4-judge]

JustDravee marked the issue as unsatisfactory:
Invalid

[14si2o]

Hello,

Could you elaborate on why this finding is invalid based on convertToPrincipal?

The finding was that the convertToUnderlying implementation is incorrect since it states that it MUST not show any variations based on caller. But since it uses previewDeposit, which is inclusive of deposit fees which can change based on the caller.

I'm not clear why convertToPrincipal has any role here?

[JustDravee]

Hello @14si2o .
Could you tell me exactly where convertToUnderlying() is using previewDeposit()?
I'm mentioning convertToPrincipal() because you're quoting the standard on convertToUnderlying() while talking about convertToPrincipal() code:

    /** @dev See {IPrincipalToken-convertToPrincipal}. */
    function convertToPrincipal(uint256 underlyingAmount) external view override returns (uint256) {
        return _convertIBTsToShares(IERC4626(ibt).previewDeposit(underlyingAmount), false);
    }

    /** @dev See {IPrincipalToken-convertToUnderlying}. */
    function convertToUnderlying(uint256 principalAmount) public view override returns (uint256) {
        return IERC4626(ibt).previewRedeem(_convertSharesToIBTs(principalAmount, false));
    }

As far as I can see, convertToUnderlying() is using previewRedeem() (even by checking the whole call trace).

This is why I said that it seems like there's a big confusion in this issue.

[14si2o]

@JustDravee

You are 100% correct, I don't understand how I did not see this.