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

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

QA Report #89

code423n4 opened this issue Jun 3, 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 resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Summary

It was easy to understand the logic for me because the codes have detailed explanations.
I recommended adding some more require() for better performance.

Low Risk Issues

  1. Add additional require() for better performance.

i) contracts\BkdLocker.sol#L49
require(_govToken != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
require(_rewardToken != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);

ii) contracts\BkdLocker.sol#L60
"maxBoost" must be greater than "startBoost", otherwise L276-L278 will be revoked.
require(maxBoost >= startBoost, Error.INVALID_AMOUNT);

iii) contracts\BkdLocker.sol#L60
"increasePeriod" must be positive, otherwise L276-L278 will be revoked.
require(increasePeriod != 0, Error.INVALID_AMOUNT);

iv) contracts\BkdLocker.sol#L123
"amount" must be positive, otherwise the user can prepare unlock endlessly.
require(amount != 0, Error.INVALID_AMOUNT);

v) contracts\BkdLocker.sol#L215
require(claimable != 0, Error.INVALID_AMOUNT);

vi) contracts\tokenomics\VestedEscrow.sol#L60
In this setFundAdmin() function at L75, it checks the first require() already.
require(fundadmin_ != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
require(rewardToken_ != address(0), , Error.ZERO_ADDRESS_NOT_ALLOWED);

  1. Wrong comment
    contracts\BkdLocker.sol#L87

There are no "deposit" or "depositFor" functions in this contract.
You need to write "lock" or "lockFor" instead.

  1. executeKeeperPoolWeight() and batchExecuteKeeperPoolWeights() functions in InflationManager.sol must have some role restrictions.
    There are 2 other functions. executeLpPoolWeight(), executeAmmTokenWeight().

contracts\tokenomics\InflationManager.sol#L151
contracts\tokenomics\InflationManager.sol#L241
contracts\tokenomics\InflationManager.sol#L326

With the above functions, the batch functions have the modifier "onlyRoles2(Roles.GOVERNANCE, Roles.INFLATION_MANAGER)".
I think the above functions should have the same modifier also.

  1. claim() function in VestedEscrow.sol should have nonReentrant modifier same as other claim() function at L138.

contracts\tokenomics\VestedEscrow.sol#L113-L115

Otherwise, you can change the function like this(same as VestedEscrowRevocable.sol).

function claim() external virtual override {
    claim(msg.sener);
}

@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 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@chase-manning chase-manning added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) labels Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

claim() function in VestedEscrow.sol should have nonReentrant modifier same as other claim() function at L138.

Valid

rest seem minor, personally the formatting takes away from what could be a pretty decent report.

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 resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants