VAD37 - `StakingRewardsManager.sol` function `topUp()` does not use array index or `indices` to setup config

[sherlock-admin]

VAD37

medium

StakingRewardsManager.sol function topUp() does not use array index or indices to setup config

Summary

The topUp() function in StakingRewardsManager.sol have input uint256[] memory indices but this was not used correctly. Instead it just use indices.length to loop through all staking contracts array from 0 to length.

The expected behaviour is changing config for specific staking contract by using indices as index. But what happen is it change all staking contracts in the array from index 0 to input length.

Vulnerability Detail

Looking at topUp() function in StakingRewardsManager.sol

    /// @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++) {//@audit H forloop forget using indices array for index of stakignContracts. Now it overflow array
            // 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);
        }
    }

Input variable indices is only used as array length. It suppose to be used like this

uint index = indices[i];
StakingRewards staking = stakingContracts[index];

If admin intention is topUp all staking contracts at once then there is no need to input entire array and only need to input uint length of array is enough.

Impact

• It is possible that topUp() will run out of gas if stakingContracts array length is too large.
• Admin have no way to change config for specific staking contract.
• Admin can only change all staking contracts config at once.

Code Snippet

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

Tool used

Manual Review

Recommendation

Use indices array as index

        for (uint i = 0; i < indices.length; i++) {
            uint index = indices[i];
            require(stakingContracts.length > index, "StakingRewardsManager::topUp: index out of bounds");
            // get staking contract and config            
            StakingRewards staking = stakingContracts[index];
            StakingConfig memory config = stakingConfigs[staking];

Duplicate of #16

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { even though its quietly different than the issue 016 but the underlying cause is the same; i will consider it valid and a dupp of it; and also the recommendation here is not so robust; so i protest againts using it.}