Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #90

Open
code423n4 opened this issue Jun 1, 2022 · 1 comment
Open

QA Report #90

code423n4 opened this issue Jun 1, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

QA report

[L-01] Event emitted when nothing is updated

In the method setRewardContracts() in the contract Booster.sol the event RewardContractsUpdated is emitted even though nothing is updated.

The method is found at L174-L190:

function setRewardContracts(
    address _rewards,
    address _stakerRewards,
    address _stakerLockRewards
) external {
    require(msg.sender == owner, "!auth");

    //reward contracts are immutable or else the owner
    //has a means to redeploy and mint cvx via rewardClaimed()
    if (lockRewards == address(0)) {
        lockRewards = _rewards;
        stakerRewards = _stakerRewards;
        stakerLockRewards = _stakerLockRewards;
    }

    emit RewardContractsUpdated(_rewards, _stakerRewards, _stakerLockRewards);
}

The reward contracts are only updated if lockRewards == address(0), however the event is emitted every time the method is called, which is misleading. The emit of the event should be moved inside the if statement, so it is only emitted if the contracts are actually updated.

[L-02] Roles missing setter method

The roles operator and rewardManager in BaseRewardPool.sol and the role rewardManager in VE3DRewardPool.sol are immutables set in the constructors.

In the situation where the assigned roles needs to be changed, this can only be done by deploying new contracts, which could be circumvented by having appropriate setter methods, like there is for all the other roles in the remaining contracts.

[NC-01] Check for zero address

In methods where address to other contracts are assigned to a state variable, it is recommended to add in a zero address check to prevent human mistakes.

Examples of methods assigning addresses could be setFeeManager() and setPoolManager() in the contract Booster.sol.

This would mean that the method setFeeManager() found at L129-133 could look like this:

function setFeeManager(address _feeM) external {
    require(msg.sender == feeManager, "!auth");
    require(_feeM != address(0), "zero address")
    feeManager = _feeM;
    emit FeeManagerUpdated(_feeM);
}
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 1, 2022
code423n4 added a commit that referenced this issue Jun 1, 2022
@GalloDaSballo
Copy link
Collaborator

[L-01] Event emitted when nothing is updated

Valid Refactoring

## [L-02] Roles missing setter method
I don't agree with this being valid

[NC-01] Check for zero address

Valid Low

1L, 1R

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants