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

Open
code423n4 opened this issue Feb 9, 2022 · 2 comments
Open

QA Report #253

code423n4 opened this issue Feb 9, 2022 · 2 comments
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 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

  1. LOW: Solidity version pragma uses caret.

Example
Note that this issue is present in pretty much every other contract in the project.

This is a bad practice because ^0.8.11 means that the contracts can be compiled with solidity versions greater than or equal to 0.8.11 but less than 0.9.0. Because any number of solidity versions can be used, it is possible that the contracts are tested with one version of solidity and deployed to production using another version of solidity, which can lead to issues if there are bugs in the solidity compiler.
Instead, the lines should be replaced with pragma solidity 0.8.11; so that 0.8.11 is used in production and testing.

  1. informational: safeTransfer to msg.sender in StakingRewards.withdraw should come after all effects.
    location
    This follows the check-effects-interactions pattern, and is good defense-in-depth (since there function uses nonReentrant modifier).

  2. Informational: Code layout in StakingRewards doesn't obey conventions, events are on the bottom.
    Example
    They should come before functions.
    Putting events at the end is inconsistent with the other contracts in this repo.
    It is also inconsistent with solidity's standard conventions

@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Feb 9, 2022
code423n4 added a commit that referenced this issue Feb 9, 2022
@r2moon r2moon added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 15, 2022
@GalloDaSballo
Copy link
Collaborator

LOW: Solidity version pragma uses caret.
Disagree, the version can be known

informational: safeTransfer to msg.sender in StakingRewards.withdraw should come after all effects.
The sponsor chose to use nonReentrant, I can make the argument that the propose solution is still vulnerable as the function connects to more than one external contract, for that reason I must disagree

Informational: Code layout in StakingRewards doesn't obey conventions, events are on the bottom.
Agree with this as a coding convention

@GalloDaSballo
Copy link
Collaborator

++

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