Accumulated rewards would be frozen when the token's index is 0

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/pendle-rewards/RewardManagerAbstract.sol#L18-L19
https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/pendle-rewards/RewardManagerAbstract.sol#L52-L79
https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/pendle-rewards/RewardManager.sol#L118-L133

Vulnerability details

Background

The contracts for taking care of the Pendle rewards was added to the scope for this update audit, after conversations with the sponsors it was understood that this implementation (of RewardManagerAbstract & RewardManager ) was inspired/forked by/from the Pendle implementation found here, problem however is that Loopfi's implementation is incomplete as it leaves out a core check which was included in the native Pendle implementation at RewardManagerAbstract.sol#L58 to ensure accumulated rewards are never blocked nor frozen.

After further discussions with the sponsors it was concluded that leaving out the check (i.e using if (userIndex == index) continue; instead of if (userIndex == index || index == 0) continue;) makes the implementation vulnerable to the very same case from the Q3 Spearbit Audit of Pendle where accumulated rewards could be frozen for some tokens linked here which they had not known about prior to their audit with Code4rena.

Proof of Concept

First note how the initial reward index has been set as 1: https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/pendle-rewards/RewardManagerAbstract.sol#L18-L19

    uint256 internal constant INITIAL_REWARD_INDEX = 1;

Now take a look at https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/pendle-rewards/RewardManagerAbstract.sol#L52-L79

    function _distributeRewardsPrivate(
        address user,
        uint256 collateralAmountBefore,
        address[] memory tokens,
        uint256[] memory indexes
    ) private {
        assert(user != address(0) && user != address(this));

        //  uint256 userShares = _rewardSharesUser(user);
        uint256 userShares = collateralAmountBefore;
        for (uint256 i = 0; i < tokens.length; ++i) {
            address token = tokens[i];
            uint256 index = indexes[i];
            uint256 userIndex = userReward[token][user].index;

            if (userIndex == 0) {
                userIndex = INITIAL_REWARD_INDEX.Uint128();
            }

            if (userIndex == index) continue;//@audit

            uint256 deltaIndex = index - userIndex;

            uint256 rewardDelta = userShares.mulDown(deltaIndex);
            uint256 rewardAccrued = userReward[token][user].accrued + rewardDelta;
            userReward[token][user] = UserReward({index: index.Uint128(), accrued: rewardAccrued.Uint128()});
        }
    }

This function is the end method used to distribute the rewards, issue however is that in the case where the index of the token is 0 the execution attempts to deduct userIndex from it, however userIndex can never be lower than 1, considering even if it's zero, it gets set to INITIAL_REWARD_INDEX, which has been initialised as 1, and as such the attempt to subtract userIndex from index, would revert here, due to having 0 - x where in this case the lowest value of x is 1 as it would be in our case when (userIndex == 0) is true.

NB: The call to _distributeRewardsPrivate() is made from _updateAndDistributeRewardsForTwo() which passes the call from _updateAndDistributeRewards() in the Abstract reward manger that is being called from the main reward manager via handleRewardsOnWithdraw()/handleRewardsOnDeposit().

Now the case for having an index == 0 is possible, this is because in some cases it is allowed to expand SY reward tokens list which can happen after _setPostExpiryData() has fixed the expiry state in one of the YTs linked to that SY. i.e this could be due to say expanding the reward token list right after linked YT expiration on Pendle, as is used to actualise the same bug case here (see 5.1.1), That's to say if a new reward token was added to the Nitro pool for e.g, but not yet added to rewardTokens of an SY, let's use CamelotV1Volatile's SY, an attacker can run any YT operation involving _setPostExpiryData() in the first block with its expiry <= block.timestamp and then run public updateRewardTokensList() of SY:

https://github.com/pendle-finance/pendle-core-v2-public/blob/7e451be619353f57a8ab722234b8f9ebe0632836/contracts/core/StandardizedYield/implementations/CamelotV1Volatile/PendleCamelotV1VolatileSY.sol#L95C1-L104C6

    /// @notice allows anyone to add new rewardTokens to this SY if a new rewardToken is added to the Nitro pool
    function updateRewardTokensList() public virtual {
        if (nitroPool == address(0)) return; // if nitroPool is not set, we don't need to update rewardTokens list

        address token1 = ICamelotNitroPool(nitroPool).rewardsToken1().token;
        address token2 = ICamelotNitroPool(nitroPool).rewardsToken2().token;

        if (token1 != address(0) && token1 != xGRAIL && !rewardTokens.contains(token1)) rewardTokens.push(token1);//@audit
        if (token2 != address(0) && token2 != xGRAIL && !rewardTokens.contains(token2)) rewardTokens.push(token2);//@audit
    }

The above allows anyone to add new rewardTokens to this SY if a new rewardToken is added to the Nitro pool, which would then cause for our attempt on handling rewards on withdrawal or deposits fail, cause the call inherently falls on _updateAndDistributeRewards() that reverts when attempting to distribute the rewards here.

Impact

Handling rewards on withdrawal or deposits would fail for that specific YT, cause the call inherently falls on _updateAndDistributeRewards() that reverts when attempting to distribute the rewards via _distributeRewardsPrivate() here.

Resources:

• Very similar attack window in 5.1.1 from Spearbit's Q3 2024 Pendle audit.

Recommended Mitigation Steps

As hinted in Spearbit's Q3 2024 Pendle audit consider ignoring indices that are not initialised, this way we never have the accumulated rewards being stuck, i.e

    function _distributeRewardsPrivate(
        address user,
        uint256 collateralAmountBefore,
        address[] memory tokens,
        uint256[] memory indexes
    ) private {
        assert(user != address(0) && user != address(this));

        //  uint256 userShares = _rewardSharesUser(user);
        uint256 userShares = collateralAmountBefore;
        for (uint256 i = 0; i < tokens.length; ++i) {
            address token = tokens[i];
            uint256 index = indexes[i];
            uint256 userIndex = userReward[token][user].index;

            if (userIndex == 0) {
                userIndex = INITIAL_REWARD_INDEX.Uint128();
            }

-            if (userIndex == index) continue;
+            if (userIndex == index || index == 0) continue;

            uint256 deltaIndex = index - userIndex;

            uint256 rewardDelta = userShares.mulDown(deltaIndex);
            uint256 rewardAccrued = userReward[token][user].accrued + rewardDelta;
            userReward[token][user] = UserReward({index: index.Uint128(), accrued: rewardAccrued.Uint128()});
        }
    }

Which has also been implemented by Pendle here.

Assessed type

Token-Transfer

[c4-sponsor]

@amarcu Sponsors are not allowed to close, reopen, or assign issues or pull requests.

[koolexcrypto]

@Bauchibred can you clarify this part? Is this applicable here? In PJQA please.

IMPORTANT NOTE:
• The issue only affects SY that can add rewards tokens. None of the active markets are affected by this issue.
• Principal of PT remains withdrawable even if the issue occurs

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[pkqs90]

This is valid in pendle because PendleYieldToken is also a RewardManagerAbstract, but in Loopfi, the only RewardManagerAbstract is RewardManager, which defaults to index = 1 if it doesn't exist.

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/pendle-rewards/RewardManager.sol#L73

So in the scope of Loopfi, this issue shouldn't be valid.

    function _updateRewardIndex()
        internal
        virtual
        override
        returns (address[] memory tokens, uint256[] memory indexes)
    {
        tokens = market.getRewardTokens();
        indexes = new uint256[](tokens.length);

        if (tokens.length == 0) return (tokens, indexes);

        if (lastRewardBlock != block.number) {
            // if we have not yet update the index for this block
            lastRewardBlock = block.number;

            uint256 totalShares = _rewardSharesTotal();
            // Claim external rewards on Market
            market.redeemRewards(address(vault));

            for (uint256 i = 0; i < tokens.length; ++i) {
                address token = tokens[i];

                // the entire token balance of the contract must be the rewards of the contract

                RewardState memory _state = rewardState[token];
                (uint256 lastBalance, uint256 index) = (_state.lastBalance, _state.index);

                uint256 accrued = IERC20(tokens[i]).balanceOf(vault) - lastBalance;

@>              if (index == 0) index = INITIAL_REWARD_INDEX;
                if (totalShares != 0) index += accrued.divDown(totalShares);

                rewardState[token] = RewardState({
                    index: index.Uint128(),
                    lastBalance: (lastBalance + accrued).Uint128()
                });

                indexes[i] = index;
            }
        } else {
            for (uint256 i = 0; i < tokens.length; i++) {
                indexes[i] = rewardState[tokens[i]].index;
            }
        }
    }

[koolexcrypto]

Correct. Indexs won't have index zero

[c4-judge]

koolexcrypto marked the issue as unsatisfactory:
Invalid