sherlock-audit / 2023-11-covalent-judging

3 stars 2 forks source link

nobody2018 - setValidatorCommissionRate should be called by staking manager #45

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

nobody2018

medium

setValidatorCommissionRate should be called by staking manager

Summary

In readme.md, it is mentioned that the responsibilities of the staking manager are as follows:

  • Set the validator commission rate

  • Add validator instances to the contract

  • Reward validators

  • Disable/Enable validators

What the owner can do:

  • Deposit tokens into the contract that will be distributed (reward pool)

  • Withdraw tokens from the contract that are supposed to be distributed. The owner cannot withdraw tokens allocated for the past checkpoints that have not yet been redeemed by the delegators

  • Set the validator max cap multiplier

  • Set the maximum number of tokens the validator can stake

  • Set the StakingManager address

  • Renounce his role and disable all the following listed actions by calling renounceOwnership

  • Transfer the ownership to another address by calling transferOwnership

  • Set or change the stakingManager by calling setStakingManagerAddress

These actions of owner do not include setting the validator commission rate.

But the current implementation of setValidatorCommissionRate can only be called by the owner. Staking manager doesn't set the validator commission rate.

Vulnerability Detail

File: cqt-staking\contracts\OperationalStaking.sol
362:->   function setValidatorCommissionRate(uint128 validatorId, uint128 amount) external onlyOwner {
363:         require(validatorId < validatorsN, "Invalid validator");
364:         require(amount < DIVIDER, "Rate must be less than 100%");
365:         _validators[validatorId].commissionRate = amount;
366:         emit ValidatorCommissionRateChanged(validatorId, amount);
367:     }

setValidatorCommissionRate has the onlyOwner modifier, which means that the function can only be called by the owner.

Impact

This breaks the assumption of the protocol: staking manager can set the commission rate.

Code Snippet

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L362

Tool used

Manual Review

Recommendation

File: cqt-staking\contracts\OperationalStaking.sol
362:-    function setValidatorCommissionRate(uint128 validatorId, uint128 amount) external onlyOwner {
362:+    function setValidatorCommissionRate(uint128 validatorId, uint128 amount) external onlyStakingManager {

Duplicate of #42

sherlock-admin2 commented 7 months ago

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

takarez commented:

invalid: admin function