Aamirusmani1552 - StakingRewardsManager::topUp(...) Misallocates Funds to StakingRewards Contracts

[sherlock-admin]

Aamirusmani1552

high

StakingRewardsManager::topUp(...) Misallocates Funds to StakingRewards Contracts

Summary

The StakingRewardsManager::topUp(...) contract exhibits an issue where the specified StakingRewards contracts are not topped up at the correct indices, resulting in an incorrect distribution to different contracts.

Vulnerability Detail

The StakingRewardsManager::topUp(...) function is designed to top up multiple StakingRewards contracts simultaneously by taking the indices of the contract's addresses in the StakingRewardsManager::stakingContracts array. However, the flaw lies in the distribution process:

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

GitHub: [254-278]

The rewards are not appropriately distributed to the StakingRewards contracts at the specified indices. Instead, they are transferred to the contracts at the loop indices. For instance, if intending to top up contracts at indices [1, 2], the actual top-up occurs at indices [0, 1].

Impact

The consequence of this vulnerability is that rewards will be distributed to the incorrect staking contract, leading to potential misallocation and unintended outcomes

Code Snippet

Here is a test for PoC:

Add the below given test in StakingRewardsManager.test.ts File. And use the following command to run the test

npx hardhat test --grep "TopUp is not done to intended staking rewards contracts"

TEST:

        it("TopUp is not done to intended staking rewards contracts", async function () {
            // add index 2 to indices
            // so topup should be done to index 0 and 2
            indices = [0, 2];

            await rewardToken.connect(deployer).approve(await stakingRewardsManager.getAddress(), tokenAmount * indices.length);
            
            // create 3 staking contracts
            await stakingRewardsManager.createNewStakingRewardsContract(await stakingToken.getAddress(), newStakingConfig);
            await stakingRewardsManager.createNewStakingRewardsContract(await stakingToken.getAddress(), newStakingConfig);
            await stakingRewardsManager.createNewStakingRewardsContract(await stakingToken.getAddress(), newStakingConfig);

            // topup index 0 and 2
            await expect(stakingRewardsManager.connect(deployer).topUp(await deployer.address, indices))
                .to.emit(stakingRewardsManager, "ToppedUp");


            // getting the staking contract at index 0, 1 and 2
            let stakingContract0 = await stakingRewardsManager.stakingContracts(0);
            let stakingContract1 = await stakingRewardsManager.stakingContracts(1);
            let stakingContract2 = await stakingRewardsManager.stakingContracts(2);

            // Staking contract at index 2 should be empty
            expect(await rewardToken.balanceOf(stakingContract2)).to.equal(0);

            // Staking contract at index 0 and 1 should have 100 tokens
            expect(await rewardToken.balanceOf(stakingContract0)).to.equal(100);
            expect(await rewardToken.balanceOf(stakingContract1)).to.equal(100);

        });

Output:

AAMIR@Victus MINGW64 /d/telcoin-audit/telcoin-audit (main)
$ npx hardhat test --grep "TopUp is not done to intended staking rewards contracts"


  StakingRewards and StakingRewardsFactory
    topUp
      ✔ TopUp is not done to intended staking rewards contracts (112ms)


  1 passing (2s)

Tool used

• Manual Review

Recommendation

It is recommended to do the following changes:

    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];
+           StakingRewards staking = stakingContracts[indices[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);
        }
    }

[amshirif]

https://github.com/telcoin/telcoin-audit/pull/27

[sherlock-admin2]

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

takarez commented:

valid because { I consider this a high severity and avalid issues; the watson was able to explain how the topUp function will perform an unintended actions by topping up from the 0 index of the array always due to lack of good implementation of the indices that was supposed to be added before the (i) }

[nevillehuang]

@amshirif Will this allow the stakers of the wrong contract funded to retrieve unintended rewards? If yes I will remain as high severity.

[amshirif]

@nevillehuang Yes this would potentially cause those who should have gotten rewards to have received less or non at all, and those who were not intended to get any or less than their desired amount to get more than they should have.

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/27.

[sherlock-admin]

The Lead Senior Watson signed-off on the fix.