_distributeRewardsPrivate wrongly set unserIndex = 1 on first interaction, leading to erroneous reward distribution

[howlbot-integration]

Lines of code

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

Vulnerability details

Summary

User reward index is set to the wrong value on its first interaction with the Reward Manager.

Vulnerability details

RewardManagerAbstract::_distributeRewardsPrivate distibute rewards to users based on:

• an index, ever increasing value representing the accrual of rewards in the vault
• their share of the vault total shares

The index works like an exchange rate: when a user deposit, the vault index is saved for that user. Then when withdrawing, the saved index is compared to the new vault index, and tell how much of the generated rewards he now own.

Let's see how works the current implementation, see snippet below to follow.

1. If a user depositing for the first time, represented by userIndex == 0 , its userIndex is set to a constant INITIAL_REWARD_INDEX == 1.
2. As the current vault index will likely not be 1, the function continue and compute the deltaIndex with userIndex == INITIAL_REWARD_INDEX == 1

    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();		<(@1)
            }

            if (userIndex == index) continue;				<(@2)

            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()});
        }
    }

So, the question is: how is index computed, and would be its value? We can find this in rewardManager::_updateRewardIndex:

1. We see that the index at its very beginning starts with an index of INITIAL_REWARD_INDEX == 1
2. then, index is incremented by a value which is represented with decimals based on the reward token as it can be infered by the code in RewardManager::_updateRewardIndex() where index is incremented by accrued.divDown(totalShares), where both variables are token decimals, and divDown is from the PMath library

    function _updateRewardIndex()
        internal
        virtual
        override
        returns (address[] memory tokens, uint256[] memory indexes)
    {

            // .... some code ... //


            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;			<(@1)
                if (totalShares != 0) index += accrued.divDown(totalShares);	<(@2)

            // .... some code ... //

Now let's say that vault index == 1.5e18 and userIndex == 1
We can see now how this line from the first snippet represent an issue:

    uint256 deltaIndex = index - userIndex;

The deltaIndex will be roughly equal to index, which represent a huge amount of accrual, while the user just deposited for the first time!

Impact

Completely wrong computation of reward distribution

Tools Used

Manual review

Recommended Mitigation Steps

For its first interaction, userIndex should be set to the actual vault index.
The fix implementation should be review carefully as this might bring another issues.

Assessed type

Math

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto marked the issue as unsatisfactory:
Invalid