Inconsistent Yield Update Handling for Redemptions Post-Expiry

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L341 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L253-L262 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L812-L819

Vulnerability details

Impact

Users who deposit before the expiry but redeem after the expiry do not receive their due rewards because the updateYield function is conditionally called only if block.timestamp < expiry. This implementation oversight results in a scenario where users' yields are not properly updated upon redemption after the contract has expired, leading to potential loss of rewards for those users.

Proof of Concept

The core of the issue lies within the _beforeRedeem function in the PrincipalToken.sol contract, specifically in the conditional logic that determines when the updateYield function is called. The _beforeRedeem function: link to code The conditional block that only updates yield if block.timestamp < expiry means users redeeming after the expiry won't have their yields updated, as shown below:

if (block.timestamp >= expiry) {
    if (!ratesAtExpiryStored) {
        storeRatesAtExpiry();
    }
} else {
    updateYield(_owner);
    IYieldToken(yt).burnWithoutUpdate(_owner, _shares);
}

This leads to a situation where, if a user has not redeemed or had their yield updated through some other means before the expiry, they will not receive the rewards they have accrued up until that point.

The updateYield function's implementation suggests that it should be possible to update a user's yield to reflect the accrued interest up until the point of expiry, but due to the conditional logic in _beforeRedeem, this update does not occur for redemptions post-expiry.

Tools Used

Manual

Recommended Mitigation Steps

To ensure users receive their appropriate yields regardless of when they redeem, the logic within _beforeRedeem should be adjusted to account for post-expiry redemptions. Specifically, the method for updating yields should consider whether the expiry has passed and use the stored rates at expiry to calculate the final yield for users redeeming after this point.

A proposed change could involve adjusting the updateYield call to ensure it appropriately accounts for the expiry condition. This might look like integrating a conditional within updateYield that checks if block.timestamp >= expiry and, if so, uses the stored rates (ptRate and ibtRate) instead of fetching new ones. The suggested modification to the updateYield function could be:

function updateYield(address _user) public override returns (uint256 updatedUserYieldInIBT) {
    // Determine rates based on whether the expiry has passed
    (uint256 _ptRate, uint256 _ibtRate) = block.timestamp >= expiry 
        ? (ptRate, ibtRate) 
        : _updatePTandIBTRates();

    // Existing implementation...
}

Assessed type

Context

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

The conditional block that only updates yield if block.timestamp < expiry means users redeeming after the expiry won't have their yields updated, as shown below:

Before expiry, we update yield when redeeming shares / withdrawing assets because those actions imply a YT burn. After expiry, those actions do not imply a YT burn anymore and updating yield when those functions are called becomes unnecessary.

This leads to a situation where, if a user has not redeemed or had their yield updated through some other means before the expiry, they will not receive the rewards they have accrued up until that point.

That is incorrect, since users can still call either claimYield or claimYieldInIBT to get the yield they have accrued up until the rates were stored after expiry.

Therefore, this issue is wrong and our protocol works as intended. We thus dispute it.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid