VAD37 - `StakingRewardsManager.sol` have `BUILDER_ROLE` operation remove staking contracts may interfere with `EXECUTOR_ROLE` operation. Causing wrong config and rewards setup

[sherlock-admin2]

VAD37

medium

StakingRewardsManager.sol have BUILDER_ROLE operation remove staking contracts may interfere with EXECUTOR_ROLE operation. Causing wrong config and rewards setup

Summary

BUILDER_ROLE can remove staking contracts from stakingContracts array. This operation is extreme hazardous. It swap stakingContracts array index.

And EXECUTOR_ROLE operation topUp() use stakingContracts array index to setup config and rewards.

If stakingContracts array index is swapped by BUILDER_ROLE without EXECUTOR_ROLE aware of the change. The config and rewards setup will be wrong. Causing wrong rewards amount and rewards duration for staking contracts.

Vulnerability Detail

Look at how BUILDER_ROLE remove staking contracts from stakingContracts array.

File: StakingRewardsManager.sol
166:     function removeStakingRewardsContract(
167:         uint256 i
168:     ) external onlyRole(BUILDER_ROLE) {//@audit M builder role can remove staking contracts during TopUp() operation causing problem for setting config
169:         StakingRewards staking = stakingContracts[i];
170: 
171:         // un-mark this staking contract as included in stakingContracts
172:         stakingExists[staking] = false;
173:         // replace the removed staking contract with the last item in the stakingContracts array
174:         stakingContracts[i] = stakingContracts[stakingContracts.length - 1];
175:         // pop the last staking contract off the array
176:         stakingContracts.pop();
177: 
178:         emit StakingRemoved(staking);
179:     }

It work by swapping latest staking contract in the array to the index of the staking contract to be removed. Then remove last staking contract in the array.

This remove operation change how stakingContracts array index map to staking contracts.

Look at how topUp() function is depend on stakingContracts array index. https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L251-L278 topUp() use previous known array index as ID to loop through stakingContracts array and send rewards, updating new config. If admin is not aware of this change while updating new config, the rewards will simply send to wrong staking contracts. Because one staking contracts will be moved to the end of the array.

Impact

Possible send rewards and update config to wrong staking contracts. Possible admin send wrong staking contracts receive rewards token with no way to recover.

Code Snippet

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

Tool used

Manual Review

Recommendation

The best options is using OpenZeppelin Enumerable as mapping for stakingContracts.

Or use nonce as stakingContracts ID which is much safer, disable contracts by simply set boolean or updating config.

[sherlock-admin2]

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

takarez commented:

invalid because { This is invalid as its unlikely to happen; the admin roles are trusted as said in ReadMe.md}

[nevillehuang]

Invalid, this is speculating on admin call order, which is invalid based on sherlocks rule see point 5.