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 #329

Open
code423n4 opened this issue May 25, 2022 · 1 comment
Open

QA Report #329

code423n4 opened this issue May 25, 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 Findings

1. Incorrect Lockduration used

The documentation and code-comment mentions the use of 16 week lock duration, but the code uses 17 week lock duration in contracts/AuraLocker.sol#L83

Recommended solution

Update the documentation and comment to 17 weeks or use 16 week lock duration in code

    uint256 public constant rewardsDuration = 86400 * 7;
    //     Duration of lock/earned penalty period
    uint256 public constant lockDuration = rewardsDuration * 16;

2. use solidity native types such as days and weeks instead of numbers for reward and lock duration

in contracts/AuraLocker.sol#L81-L83, the rewards duration and lock duration uses numbers as uint, this can be replaced with solidity native time units such as days and weeks.

    // Before

    //     Duration that rewards are streamed over
    uint256 public constant rewardsDuration = 86400 * 7;
    //     Duration of lock/earned penalty period
    uint256 public constant lockDuration = rewardsDuration * 17;

    // After
    //     Duration that rewards are streamed over
    uint256 public constant rewardsDuration = 7 days;
    //     Duration of lock/earned penalty period
    uint256 public constant lockDuration = 17 weeks;



3. Use 2 step process while setting the owner in setOwner() methods

In order to protect from accidentally setting new owner to a dead address, zero address, It is a good practice to use 2 step process of setting new owner.

  1. Use a pendingAdmin state variable to set the admin inside setOwner()
  2. Implement claimOwnership() function to be called by pendingAdmin to claim the ownership

Usages in

convex-platform/contracts/contracts/Booster.sol#L128

convex-platform/contracts/contracts/VoterProxy.sol#L73

@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 May 25, 2022
code423n4 added a commit that referenced this issue May 25, 2022
@0xMaharishi
Copy link

first point is invalid

@0xMaharishi 0xMaharishi added the duplicate This issue or pull request already exists label May 28, 2022
@dmvt dmvt removed the duplicate This issue or pull request already exists label Jul 7, 2022
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

3 participants