Incorrect staking index handling inside topUp function of StakingRewardsManager contract

Summary

In topUp function of StakingRewardsManager contract, indices parameter (type uint256[]) is passed to indicate which staking contracts should be topped up. However, the elements of indices are not used, but the index (starts from 0 – indices.length) of indices is used to select stakingContracts to top up. This could potentially lead to problems: 1) top up incorrect stakingContracts; 2) can’t just top up some certain stakingContracts, have to start from index 0.

Vulnerability Detail

/// @notice Top up multiple staking contracts
/// @param source address from which tokens are taken
/// @param indices array of staking contract indices
function topUp(
    address source,
    uint256[] memory indices
) external onlyRole(EXECUTOR_ROLE) {
    for (uint i = 0; i < indices.length; i++) {
        // get staking contract and config
        StakingRewards staking = stakingContracts[i];
        StakingConfig memory config = stakingConfigs[staking];

        // will revert if block.timestamp <= periodFinish
        staking.setRewardsDuration(config.rewardsDuration);

        // pull tokens from owner of this contract to fund the staking contract
        rewardToken.transferFrom(
            source,
            address(staking),
            config.rewardAmount
        );

        // start periods
        staking.notifyRewardAmount(config.rewardAmount);

        emit ToppedUp(staking, config);
    }
}

According to function interface and comments, indices array’s elements indicate which staking contracts should be topped up, but they’re never used.

for (uint i = 0; i < indices.length; i++) {
    // get staking contract and config
    StakingRewards staking = stakingContracts[i];
    StakingConfig memory config = stakingConfigs[staking];

Index of indices array is used directly to select stakingContracts.

Impact

1) Could top up incorrect stakingContracts if simply follows function interface and comment's instruction; 2) topUp functionality is broken since no way to top up selected stakingContracts, but have to top up sequentially;

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L251-L278

Tool used

Manual Review

Recommendation

Use indices elements to select stakingContracts, as below:

for (uint i = 0; i < indices.length; i++) {
    // get staking contract and config
    uint256 idx = indices[i];
    StakingRewards staking = stakingContracts[idx];
    StakingConfig memory config = stakingConfigs[staking];

Duplicate of #16

sherlock-audit / 2024-01-telcoin-judging

DMoore - Incorrect staking index handling inside topUp function of StakingRewardsManager contract #53