Spectra - 14si2o_Flint's results

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

[L-01] anyone can set the ibt/pt rate for anyone through updateYield

The updateYield function lacks any access control, allowing anyone to set the ibtRate and ptRate for anyone.

If this is set before a user interacts with the protocol, the below check will be circumvented and computeYield will called with value set by someone in the past.

        // Check for skipping yield update when the user deposits for the first time or rates decreased to 0.
        if (_oldIBTRateUser != 0) {
            updatedUserYieldInIBT = PrincipalTokenUtil._computeYield(
                _user,
                yieldOfUserInIBT[_user],
                _oldIBTRateUser,
                _ibtRate,
                _oldPTRateUser,
                _ptRate,
                yt
            );
        }

[L-02] The Mint event gives incorrect information

The Mint event in PrincipalToken.sol sets the msg.sender as originator ('from')of the minting coins. This is obviously incorrect since it is the PT contract which mints the token, hence there is a Transfer event with address(0) as originator.

The recommandation is to set the 'from' in the Mint event to address(0) to be consistent with standard minting events.

[L-03] Inconsistent application of SAFETY_BOUND

In _computeYield in PrincipalTokenUtil.sol, a SAFETY_BOUND is applied with the stated goal of favoring the protocol in case of approximation.

However, this safety is only applied in the second part of the if statement. Since it is perfectly possible for the first part to resolve to an amount smaller than the SAFETY_BOUND, it should be applied consistenly in all cases.

        } else {
            if (_oldPTRate > _ptRate) {
                // PT depeg happened
                uint256 yieldInAssetRay;
                if (_ibtRate >= _oldIBTRate) {
                    // both negative and positive yield happened, more positive
                    yieldInAssetRay =
                        _convertToAssetsWithRate(
                            userYTBalanceInRay,
                            _oldPTRate - _ptRate,
                            RayMath.RAY_UNIT,
                            Math.Rounding.Floor
                        ) +
                        _convertToAssetsWithRate(
                            ibtOfPTInRay,
                            _ibtRate - _oldIBTRate,
                            RayMath.RAY_UNIT,
                            Math.Rounding.Floor
                        );
                } else {
                    // either both negative and positive yield happened, more negative
                    // or only negative yield happened
                    uint256 actualNegativeYieldInAssetRay = _convertToAssetsWithRate(
                        userYTBalanceInRay,
                        _oldPTRate - _ptRate,
                        RayMath.RAY_UNIT,
                        Math.Rounding.Floor
                    );
                    uint256 expectedNegativeYieldInAssetRay = Math.ceilDiv(
                        ibtOfPTInRay * (_oldIBTRate - _ibtRate),
                        RayMath.RAY_UNIT
                    );
                    yieldInAssetRay = expectedNegativeYieldInAssetRay >
                        actualNegativeYieldInAssetRay
                        ? 0
                        : actualNegativeYieldInAssetRay - expectedNegativeYieldInAssetRay;
                    yieldInAssetRay = yieldInAssetRay.fromRay(
                        IERC4626(IPrincipalToken(IYieldToken(_yt).getPT()).underlying()).decimals()
                    ) < SAFETY_BOUND 
                        ? 0
                        : yieldInAssetRay;
                }

[NC-01] Incorrect naming of burnWithoutUpdate

The burnWithoutUpdate function name in YieldToken.sol implies that this will function will not call the _update function. Yet it calls the _burn function which calls _update.

After checking with the protocol, the without update refers to the updateYield function. So the function should be renamed to burnWithoutYieldUpdate to avoid confusion.