code-423n4 / 2022-12-gogopool-findings

1 stars 0 forks source link

QA Report #728

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

See the markdown file with the details of this report here.

GalloDaSballo commented 1 year ago

| [L‑01] | Inflation not locked for four years | 1 | R Code as is will revert after 4 years, so it's enforced via config

| [L‑02] | Contract will stop functioning in the year 2106 | 1 | L

| [L‑03] | Lower-level initializations should come first | 1 | R in lack of specific risk

| [L‑04] | Cycle end may be be too soon | 1 | I don't think this is valid

| [L‑05] | Incorrect percentage conversion | 1 | L

| [L‑06] | Missing checks for value of msg.value | 1 | Invalid: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L401

| [L‑07] | Loss of precision | 2 | L

| [L‑08] | Signatures vulnerable to malleability attacks | 1 | L | [L‑09] | require() should be used instead of assert() | 1 | R

| [L‑10] | Open TODOs | 1 | NC

| [N‑01] | Common code should be refactored | 1 | R

| [N‑02] | String constants used in multiple places should be defined as constants | 1 | R

| [N‑03] | Constants in comparisons should appear on the left side | 1 | R

| [N‑04] | Inconsistent address separator in storage names | 1 | NC

| [N‑05] | Confusing function name | 1 | NC

| [N‑06] | Misplaced punctuation | 1 | NC

| [N‑07] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 1 | Disputing for TokenAvax as it's the child contract

| [N‑08] | Import declarations should import specific identifiers, rather than the whole file | 13 | NC

| [N‑09] | Missing initializer modifier on constructor | 1 | R

| [N‑10] | The nonReentrant modifier should occur before all other modifiers | 2 | R

| [N‑11] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings | 1 | NC

| [N‑12] | constants should be defined rather than using magic numbers | 2 | R

| [N‑13] | Missing event and or timelock for critical parameter change | 1 | NC

| [N‑14] | Events that mark critical parameter changes should contain both the old and the new value | 2 | NC

| [N‑15] | Use a more recent version of solidity | 1 | NC

| [N‑16] | Use a more recent version of solidity | 1 | See N-15

| [N‑17] | Constant redefined elsewhere | 2 | R

| [N‑18] | Lines are too long | 1 | NC

| [N‑19] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 2 | R

| [N‑20] | Using >/>= without specifying an upper bound is unsafe | 2 | R

| [N‑21] | Typos | 3 | NC

| [N‑22] | File is missing NatSpec | 3 | NC

| [N‑23] | NatSpec is incomplete | 27 | NC

| [N‑24] | Not using the named return variables anywhere in the function is confusing | 1 | R

| [N‑25] | Contracts should have full test coverage | 1 | R

| [N‑26] | Large or complicated code bases should implement fuzzing tests | 1 | R

| [N‑27] | Function ordering does not follow the Solidity style guide | 15 | NC

| [N‑28] | Contract does not follow the Solidity style guide's suggested layout ordering | 9 | NC

GalloDaSballo commented 1 year ago

2L from dups

4L 15R 15NC

GalloDaSballo commented 1 year ago

6L 15R 15NC including dups

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-a

c4-judge commented 1 year ago

GalloDaSballo marked the issue as selected for report

c4-judge commented 1 year ago

GalloDaSballo marked the issue as not selected for report

c4-judge commented 1 year ago

GalloDaSballo marked the issue as selected for report

GalloDaSballo commented 1 year ago

Best report by far, so far I though the second best was the best (this one scores above 100%)

Well played