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

Open
code423n4 opened this issue Aug 6, 2022 · 0 comments
Open

QA Report #293

code423n4 opened this issue Aug 6, 2022 · 0 comments
Labels
bug Something isn't working old-submission-method QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax valid

Comments

@code423n4
Copy link
Contributor

Typos


HomeFiProxy.sol: L22

    /// @notice bytes2 array of upgradable contracts initials

Change contracts to contract


HomeFiProxy.sol: L29

    /// @dev mapping that tell if a particular address is active(latest version of contract)

Change tell to tells or shows


HomeFiProxy.sol: L32

    /// @dev mapping that maps contract initials with there implementation address

Change there to their


Disputes.sol: L51

        // Revert if project not originated of HomeFi

Change of to by


Disputes.sol: L179

        // Revert is signer is not builder, contractor or subcontractor.

Change first instance of is to if


HomeFi.sol: L115

        lenderFee = _lenderFee; // the percentage must be multiplied with 10

Change with to by


Project.sol: L214

        // Allocate funds to tasks and mark then as allocated

Change then to them or else remove then


Project.sol: L455

                    // As as needs to go though funding process again.

Replace repeated word as


The same typo (Revet) occurs in both lines referenced below:

Project.sol: L514

Project.sol: L520

Example (Project.sol: L514):

            // Revet if sender is not builder or contractor

Change Revet to Revert in both cases


Project.sol: L574

        // Max amount out times this loop will run

Change out to of


Project.sol: L575

        // This is to ensure the transaction do not run out of gas (max gas limit)

Change do to does


The same typo (is) occurs in both lines referenced below:

Project.sol: L613

Project.sol: L660

                // If there is enough funds to allocate this task

Change is to are in both cases


Community.sol: L164

        // Emit event if _hash. This way this hash needs not be stored in memory.

Change needs to need


Community.sol: L271

        // If _publishFee is zero than mark publish fee as paid

Change than to then


The same typo (address - address) occurs in both lines referenced below:

Project.sol: L866

Community.sol: L821

Example (Project.sol: L866):

     * @param _address address - address checked for validity

Suggestion:

     * @param _address address, which is checked for validity

Community.sol: L618

        // Initiate empty equal equal to member count length

Remove repeated word equal



Unclear comments

Comments should communicate clearly, immediately and without ambiguity. The comments below could be improved, as shown:


Project.sol: L379

        // If balance is present then it to the builder.

Suggestion: Change then it to then transfer it


Community.sol: L752

     * @param _communityID uint256 - the the uuid of the community

Suggestion:

     * @param _communityID uint256 - the uuid of community the project is held in


Long single line comments

In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable, there are cases where very long comments interfere with readability.


Below are five instances of long comments whose readability could be improved, as shown:

HomeFiProxy.sol: L50

     * @notice Initialize all the homeFi contract in the correct sequential order and generate upgradable proxy for them.

Suggestion:

     * @notice Initialize all the homeFi contract in the correct sequential order
     *  and generate upgradable proxy for them.

HomeFiProxy.sol: L123

     * @param _contractAddresses address array of contract implementation address that needs to be upgraded

Suggestion:

     * @param _contractAddresses address array of contract implementation —  
     *  address that needs to be upgraded.

Project.sol: L785

     * If contractor is assigned but not delegated then only checks for builder and contractor signature.

Suggestion:

     * If contractor is assigned but not delegated, then only checks 
     *  for builder and contractor signature.

Project.sol: L819

     * @dev Check if recovered signatures match with builder, contractor and subcontractor address for a task.

Suggestion:

     * @dev Check if recovered signatures match with builder, contractor 
     *  and subcontractor address for a task.

SignatureDecoder.sol: L58

    * @param pos which signature to read. A prior bounds check of this parameter should be performed, to avoid out of bounds access

Suggestion:

    * @param pos which signature to read — a prior bounds check of this parameter should be performed 
    *  to avoid out of bounds access.


Update sensitive terms

Terms incorporating "white," "black," "master" or "slave" are potentially problematic. Substituting more neutral terminology is becoming common practice. It is apparent that use of whitelist has been avoided in Rigor Protocol. There is one instance of master, however, as shown below:


Project.sol: L86

    /// @dev Added to make sure master implementation cannot be initialized


@code423n4 code423n4 added bug Something isn't working old-submission-method QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Aug 6, 2022
code423n4 added a commit that referenced this issue Aug 6, 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 old-submission-method QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax valid
Projects
None yet
Development

No branches or pull requests

2 participants