iberry - too many roles suggests a potential collision issue

[sherlock-admin2]

iberry

high

too many roles suggests a potential collision issue

Summary

The usage of onlyRole exist potential collision issue

Vulnerability Detail

To align with the design, we have categorized roles into BUILDER_ROLE, MAINTAINER_ROLE, SUPPORT_ROLE, ADMIN_ROLE, and EXECUTOR_ROLE and have different person to work. Certain functions in StakingRewardsManager require specific roles, but when sub-functions are called in StakingRewards with the onlyOwner modifier, it may lead to collisions.

For instance, consider the recoverERC20FromStaking function. The recoverERC20FromStaking has the onlyRole(SUPPORT_ROLE) modifier, while the sub-call to staking.recoverERC20 uses onlyOwner. To prevent collisions, we require msg.sender == SUPPORT_ROLE == owner.

Another example is the topUp function. We need msg.sender == EXECUTOR_ROLE == rewardsDistribution == owner to align with the design.

To summarize:

msg.sender == SUPPORT_ROLE == EXECUTOR_ROLE == ADMIN_ROLE == BUILDER_ROLE == rewardsDistribution == owner The current setting of only MAINTAINER_ROLE to avoid collisions with the owner deviates from the intended design. Therefore, it is necessary to adjust certain roles to adhere to the design

Impact

high ,design collision

Code Snippet

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

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewards.sol#L260

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewards.sol#L207-L209

msg.sender == topUp（EXECUTOR_ROLE） == staking.setRewardsDuration（owner） == staking.notifyRewardAmount（rewardsDistribution）

Tool used

Manual Review

Recommendation

Adjusting certain roles is necessary to adhere to the design and minimize role collisions. Removing intermediate functions can further reduce the likelihood of such collisions.

[sherlock-admin2]

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

takarez commented:

invalid because { the watson did not show how that will lead to collision; making it invalid}

[nevillehuang]

Invalid, this is simply a design improvement suggestion