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

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

QA Report #95

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

Summary

I've found some low risk issues including wrong comments and I recommend to use same PRECISION for fee denominator.
And I see many external functions don't have dev comments and natspecs and it would be good to add for good understandings.

Low Risk Issues

  1. Wrong comments
    contracts\Minter.sol#L30
    0.05% => 5%

contracts\Minter.sol#L41
0.03% => 3%

contracts\Minter.sol#L85
2% => 99%

contracts\Pair.sol#L17
You wrote "not immutable" for stable

contracts\VeloGovernor.sol#L21
0.02% => 0.2%

  1. It would be good to add one more require() for _fantomSender.
    contracts\redeem\RedemptionReceiver.sol#L44
    If _fantomSender is zero address, then it will be considered as non-initialized even after successful deposit.
    You need to insert below code at #L45.

require(_fantomSender != address(0), "ZERO ADDRESS");

  1. rewards array of Gauge.sol would have duplicate tokens inside.
    contracts\Gauge.sol#L104-L119

Impact
You add 3 tokens without checking isReward[_token] == false, and I think it's possible _token = _token0 or _token1.
In this case, rewards array will have duplicate tokens and you can add at most 15 tokens(original limit = 16)
unless you remove the duplicate token using swapOutRewardToken() function manually.
And when you call addRewardToken() of Bribe.sol at #L106, #L113, #L116, Bribe contract will filter tokens properly
so the length of rewards array of Gauge.sol and Bribe.sol will be different.

Recommended Mitigation Steps
You need to check isReward is false before you add _token0, _token1.

Non-critical Issues

  1. Event should use indexed fields if there are three or more fields
    contracts\RewardsDistributor.sol#L23

  2. Needless uint casting
    contracts\Voter.sol#L118
    contracts\Voter.sol#L168
    contracts\Voter.sol#L169

  3. require()/revert() statements should have descriptive reason strings
    There are more than 30 cases and I will show one.
    contracts\Bribe.sol#L24

@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 29, 2022
code423n4 added a commit that referenced this issue May 29, 2022
@GalloDaSballo
Copy link
Collaborator

GalloDaSballo commented Jul 2, 2022

Wrong comments

Valid NC

It would be good to add one more require() for _fantomSender.

Valid Low -> Pretty sure this is Med -> Dup of #36

rewards array of Gauge.sol would have duplicate tokens inside.

Valid Low

Event should use indexed fields if there are three or more fields

Valid NC

Needless uint casting

Valid NC (nice find)

## require()/revert() statements should have descriptive reason strings
Valid NC

Nice and short. 2L 4 NC

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