sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

0x_Sanzcy - StakingRewards Contract can't be managed StakingRewardsManager due to lack of ownership #156

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

0x_Sanzcy

high

StakingRewards Contract can't be managed StakingRewardsManager due to lack of ownership

Summary

// in order to manage this contract we have to own it

According to the comments the stakingRewards contract needs to be owned by the stakingRewardsManager in order to manage by it, this allows the RewardManger to send/ increase the rewardTokens in a case where it's not enough in the staking contract or remove the stakingRewards contract among other things.

Vulnerability Detail

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit%2Fcontracts%2Ftelx%2Fcore%2FStakingRewardsFactory.sol#L43-L66

function createStakingRewards(
        address rewardsDistribution,
        IERC20 rewardsToken,
        IERC20 stakingToken
    ) external onlyOwner returns (StakingRewards) {
        // create contract
        StakingRewards stakingRewards = new StakingRewards(
            rewardsDistribution,
            rewardsToken,
            stakingToken
        );

        // add contract to list
        stakingRewardsContracts.push(stakingRewards);
        //emit values associated
        emit NewStakingRewardsContract(
            getStakingRewardsContractCount() - 1,
            rewardsToken,
            stakingToken,
            StakingRewards(stakingRewards)
        );

        return StakingRewards(stakingRewards);
    }

The newly created stakingRewards contract will be owned by the factory owner if the ownership isn't transferred to the stakingRewardsManager

When adding the stakingRewardsContract the ownership isn't transferred to the stakingRewardsManager

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit%2Fcontracts%2Ftelx%2Fcore%2FStakingRewardsManager.sol#L144-L160

    function _addStakingRewardsContract(
        StakingRewards staking,
        StakingConfig calldata config
    ) internal {
        // in order to manage this contract we have to own it
        // staking.acceptOwnership();
        // in order to top up rewards, we have to be rewardsDistribution. this is an onlyOwner function
        staking.setRewardsDistribution(address(this));

        // push staking onto stakingContracts array
        stakingContracts.push(staking);
        // set staking config
        stakingConfigs[staking] = config;
        // mark inclusion in the stakingContracts array
        stakingExists[staking] = true;

        emit StakingAdded(staking, config);

Impact

This will make managing the stakingRewards contract unmanageable by the stakingRewardsManager

Code Snippet

Tool used

Manual Review

Recommendation

Ensure the ownership is transferred.

Duplicate of #100

sherlock-admin2 commented 6 months ago

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

takarez commented:

invalid because { This is invalid asif there is need of ownership transfer ; it should be after deployment and the admin will do that}