If one contract has sufficient reward other contracts cannot receive tokens

Summary

Within StakingRewardsManager.sol::topUp(), the executor admin is responsible for sending and adding rewards to staking contracts as needed. However, the current implementation may fail if one contract already possesses sufficient rewards.

Vulnerability Detail

The method adopts a Push-over-Pull transfer approach but lacks the capability for the executor to explicitly specify the stakingReward contracts. Instead, it relies on an array of staking contract indices, initiating a loop for staking contracts based on the provided indices. This implementation is unorthodox and suboptimal because the function fails to distinguish which contract requires a top-up. Consequently, if one contract already possesses sufficient rewards, other contracts in need of essential reward tokens may go without.

The issue leading to reversion stems from this line staking.notifyRewardAmount(config.rewardAmount);, where it invokes and verifies the reward rate as follows:

// StakingRewards::notifyRewardAmount()
require(
    rewardRate <= balance / rewardsDuration,
    "Provided reward too high"
);

Impact

StakingReward contracts in need of topping up will be denied the reception of necessary tokens due to the flawed implementation.

Code Snippet

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

Tool used

Manual Review

Recommendation

1) Use the Pull-Over-Push method of transferring tokens Sample 2) Whitelist the stakingRewards contract in the StakingRewardsManager and then allow the executor to send reward tokens to the whitelisted stakingRewards contracts.

sherlock-admin2 commented 7 months ago

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

takarez commented:

invalid because { The admin that is to set call the topUp has the ability specify the reward that need topUp via the indices parameter;had been been the case of wrong implementation of the indices in stakingReward this would be valid; but there is no mention of such by the watson here}

goheesheng commented 7 months ago

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

takarez commented:

invalid because { The admin that is to set call the topUp has the ability specify the reward that need topUp via the indices parameter;had been been the case of wrong implementation of the indices in stakingReward this would be valid; but there is no mention of such by the watson here}

I had stated "Consequently, if one contract already possesses sufficient rewards, other contracts in need of essential reward tokens may go without"

goheesheng commented 7 months ago

Hi @nevillehuang , why is this excluded? It is a DoS and other contracts not able to receive tokens if affected.

nevillehuang commented 7 months ago

@goheesheng please refrain from commenting on any other issues other than the one i requested poc from. Rest of the issues will have invalidation reasons before preliminary results