BAICE - Different `stakingRewards` contract address by using a same index id, `StakingRewardsManager:removeStakingRewardsContract` will cause the index ->address map to be out of order

[sherlock-admin2]

BAICE

high

Different stakingRewards contract address by using a same index id, StakingRewardsManager:removeStakingRewardsContract will cause the index ->address map to be out of order

Summary

Both StakingRewardsFactory and StakingRewardsManager contract use index to getStakingRewardsContract. But when call removeStakingRewardsContract in StakingRewardsManager , the index of the two contracts will dismatch .

Vulnerability Detail

The BUILDER_ROLE, create new stakingReward contract by calling stakingRewardsFactory.createStakingRewards method, and then , both contracts pull new stakingRewardcontract to a StakingRewards[] array .

StakingRewardsFactory.sol

 StakingRewards[] public stakingRewardsContracts;
 ···
  // add contract to list
 stakingRewardsContracts.push(stakingRewards);

StakingRewardsManager.sol

 /// @dev Array of managed StakingRewards contracts
    StakingRewards[] public stakingContracts;

 // push staking onto stakingContracts array
   stakingContracts.push(staking);

but if we call StakingRewardsManager:removeStakingRewardsContract, the specified index in StakingRewardsManager:stakingContracts will be poped , and the order by index will dismatch .

ForExample

1. before remove

   stakingRewardsContracts  : [0x1, 0x2,0x3, 0x4, 0x5]
   stakingContracts  :  [0x1, 0x2,0x3, 0x4, 0x5]

2. after remove 0x3 address

   stakingRewardsContracts  : [0x1, 0x2,0x3, 0x4, 0x5]
   stakingContracts  :  [0x1, 0x2, 0x5, 0x4 ]

3. add a new stakingReward contract

   stakingRewardsContracts  : [0x1, 0x2, 0x3, 0x4, 0x5, 0x6 ]
   stakingContracts  :       [0x1, 0x2, 0x5, 0x4, 0x6 ]

Impact

Unmatched array elements by same index .

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsFactory.sol#L73-L77

 function getStakingRewardsContract(
        uint index
    ) external view returns (StakingRewards) {
        return stakingRewardsContracts[index];
    }

remove one element https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L166-L179

 function removeStakingRewardsContract(
        uint256 i
    ) external onlyRole(BUILDER_ROLE) {
        StakingRewards staking = stakingContracts[i];

        // un-mark this staking contract as included in stakingContracts
        stakingExists[staking] = false;
        // replace the removed staking contract with the last item in the stakingContracts array
        stakingContracts[i] = stakingContracts[stakingContracts.length - 1];
        // pop the last staking contract off the array
        stakingContracts.pop();

        emit StakingRemoved(staking);
    }

Get staking contract address in StakingRewardsManager https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L92C3-L98C1

  function getStakingContract(
        uint256 i
    ) external view returns (StakingRewards) {
        return stakingContracts[i];
    }

Tool used

Manual Review

Recommendation

Change StakingRewardsManager:stakingContracts index -> stakingReward contract address to zero address when calling removeStakingRewardsContract

@- stakingContracts[i] = stakingContracts[stakingContracts.length - 1];
        // pop the last staking contract off the array
@-  stakingContracts.pop();

@+ stakingContracts[i] = StakingRewards(0x0000000000000000000000000000000000000000);

Duplicate of #104

[sherlock-admin2]

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

takarez commented:

invalid because { There is no place where it stated that they must match}