Incorrect implementation of the EIP-5095 standard for maxRedeem() and maxWithdraw()

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L482-L485
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L458-L462

Vulnerability details

Impact

The EIP-5095 standard states that the function MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

When the protocol is paused and redemption is temporarily disabled, this function should return 0.

Yet, both functions returns the value as if redemption is still enabled, which is a clear-cut violation of the EIP-5095 standard.

Proof of Concept

From: EIP-5095

maxRedeem  

Maximum amount of principal tokens that can be redeemed from the holder balance, through a redeem call.

MUST return the maximum amount of principal tokens that could be transferred from holder through redeem and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

MUST NOT revert.

maxWithdraw

Maximum amount of the underlying asset that can be redeemed from the holder principal token balance, through a withdraw call.

MUST return the maximum amount of underlying tokens that could be redeemed from holder through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

MUST NOT revert.

PrincipalToken.sol

    /** @dev See {IPrincipalToken-maxWithdraw}.
     */
    function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
        return convertToUnderlying(_maxBurnable(owner));
    }

PrincipalToken.sol

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

PrincipalToken.sol

    /** @dev See {IPrincipalToken-maxRedeem}. */
    function maxRedeem(address owner) public view override returns (uint256) {
        return _maxBurnable(owner);
    }

PrincipalToken.sol

    /**
     * @notice Computes the maximum amount of burnable shares for a user
     * @param _user The address of the user
     * @return maxBurnable The maximum amount of burnable shares
     */
    function _maxBurnable(address _user) internal view returns (uint256 maxBurnable) {
        if (block.timestamp >= expiry) {
            maxBurnable = balanceOf(_user);
        } else {
            uint256 ptBalance = balanceOf(_user);
            uint256 ytBalance = IYieldToken(yt).balanceOf(_user);
            maxBurnable = (ptBalance > ytBalance) ? ytBalance : ptBalance;
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Change the functions so that it will return 0 when the protocol is paused.

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 satisfactory

[c4-judge]

JustDravee marked the issue as partial-75