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.

Sign up for GitHub

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

Jump to bottom

QA Report #2

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

QA Report #2

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

Impact

[1] By default, function types and state variables/constants are internal, so the internal keyword can be omitted.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/CollateralAdapter.sol#L24
  2. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/CollateralAdapter.sol#L27
  3. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/CollateralAdapter.sol#L29
  4. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/CollateralAdapter.sol#L49
  5. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L28
  6. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L29
  7. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L30
  8. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L48
  9. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L55
  10. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L117
  11. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L145
  12. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L146
  13. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L148
  14. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L161
  15. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L173
  16. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L49
  17. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L52
  18. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L53
  19. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L36
  20. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L37
  21. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L39
  22. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L46

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[2] Magic number, consider using named constant instead.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L123
  2. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L48
  3. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L136

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[3] Consider using "_" separate digit capacity i.e "100000" could be replaced to "100_000".
This increases code readability.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L48

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[4] Consider using IERC20 type instead of address.
Or IERC20[] type instead of address[].

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L28
  2. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L29
  3. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L37
  4. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L93
  5. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L94
  6. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L108
  7. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L43
  8. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L64
  9. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L93
  10. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L94
  11. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L106
  12. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L106
  13. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L195
  14. https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L202

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[5] Typo: variable name supposed to be 'decimals'.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L122

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[6] Consider reducing if nesting by having early continue/return and else contents clause can be placed right after.
This increases readability of the code.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L220-L229
  2. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L158-L167
  3. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L128-L147

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[7] Usually when you leave function empty it is a good practice to place a comment inside brackets { /* reason why here is no code */ }
Consider adding explanation in comments.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L246
  2. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L255
  3. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L265
  4. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L24

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[8] Consider adding here require(msg.value == 0); since it is non-ETH token.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L96

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[9] Concern: Isn't it better to break the for-loop instead of reverting whole transaction?

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L122

Proof of Concept

Tools Used

Recommended Mitigation Steps

Impact

[10] Brackets aren't necessary here, consider making this code one-liner.

Affected code:

  1. https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L199-L201

Proof of Concept

Tools Used

Recommended Mitigation Steps
@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 13, 2022
code423n4 added a commit that referenced this issue May 13, 2022
@code423n4
MaratCerby issue #2
6cc584e
This was referenced May 15, 2022
Open
Open
This was referenced Jun 3, 2022
Closed
Closed
Closed
Closed
@HickupHH3
Copy link
Collaborator

HickupHH3 commented Jun 6, 2022 •
edited
Loading

NC issues: 1, 2, 3, 4, 5, 6, 7, 10
Low issues: #3, #4, #5, 9

8 has been bumped to medium severity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

No branches or pull requests
2 participants
@code423n4 @HickupHH3