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

Open
code423n4 opened this issue May 16, 2022 · 0 comments
Open

QA Report #83

code423n4 opened this issue May 16, 2022 · 0 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 acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lack of empty address checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Source code:

As stated in the OpenZeppelin source comments, the OpenZeppeling ERC20 safeApprove() function has been deprecated.

Using this deprecated function could result in accidental reverts and possibly fund locking. The deprecation of this function is discussed in greater depth in the OZ issue #2219.

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.

Source code:

Unsecure Ownership change

The modification of an owner is a delicate procedure, as it may jeopardize the governance of our contract and therefore the project. As a result, it is advised that the owner's modification logic be changed to one that allows for verification that the new owner is valid and exists.

It is required to establish a logic for the owner's modification in which a new owner is proposed first, the owner approves the suggestion, and we ensure that the new owner's address is written correctly.

Source code:

Reentrancy pattern

Although it has not been possible to exploit the reentrancy attack, the logic of the methods named below, make the changes of the validation flags after a method that allows reentrancy, it is convenient to modify the flags before the external calls to contracts.

Source code:

@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 16, 2022
code423n4 added a commit that referenced this issue May 16, 2022
@0xMaharishi 0xMaharishi added duplicate This issue or pull request already exists sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels May 25, 2022
@dmvt dmvt removed the duplicate This issue or pull request already exists label Jul 7, 2022
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 acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants