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

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

QA Report #202

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

  • There are some unsafe casts, meaning if the value is greater, it will be truncated, e.g.:
  _locked.amount += int128(int256(_value));

Better utilize SafeCast library where possible.

  • The current best practice is to use safe ERC20 library for token interactions (safeApprove and safeTransfer).
    There are some instances in code where regular transfers are used, e.g.:
   assert(IERC20(token).transfer(msg.sender, value));
   assert(IERC20(token).transferFrom(from, address(this), _value));
  • setGovernor and setEmergencyCouncil could be a 2-step (propose-accept) process to reduce the possibility of an error.

  • When an old reward token is replaced by swapOutRewardToken, the old token balance will be left in the contract. Consider extracting this balance before updating the tokens. Or even better, add token sweep functions for unprotected tokens.

  • Consider keeping the rewards list in Gauge and Bribe in sync.

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

There are some unsafe casts, meaning if the value is greater, it will be truncated, e.g.:

Valid Low

The current best practice is to use safe ERC20 library for token interactions (safeApprove and safeTransfer).

Because the token is known, the finding is not valid

setGovernor and setEmergencyCouncil could be a 2-step (propose-accept) process to reduce the possibility of an error.

Valid NC

When an old reward token is replaced by swapOutRewardToken, the old token balance will be left in the contract. Consider extracting this balance before updating the tokens. Or even better, add token sweep functions for unprotected tokens.

Disagree because it would allow the governance to rug

Consider keeping the rewards list in Gauge and Bribe in sync.

Would have liked more detail

Short and sweet, ideally would like more findings / more details

1 L, 1 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