chaduke - getSupportingDocumentsComplete() might return claimIds who have actually not completed their supporting documents

[github-actions]

chaduke

medium

getSupportingDocumentsComplete() might return claimIds who have actually not completed their supporting documents

Summary

getSupportingDocumentsComplete() might return claimIds who have actually not completed their supporting documents.

Vulnerability Detail

getSupportingDocumentsComplete() might return claimIds who have actually not completed their supporting documents because setSupportingDocumentsComplete() will push the _claimID to the list even when _supportingDocumentsComplete == false.

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/OngoingBountyV1.sol#L188-L198

As a result, getSupportingDocumentsComplete() will return a list claimIds including some that have not actually completed their supporting documents.

function getSupportingDocumentsComplete()
        external
        view
        returns (bytes memory)
    {
        return abi.encode(supportingDocumentsCompleteClaimIds);
    }

Impact

getSupportingDocumentsComplete() will return a list claimIds including some that have not actually completed their supporting documents.

Code Snippet

Remix

Tool used

Manual Review

Recommendation

Only push the _claimId only when _supportingDocumentsComplete == true.

function setSupportingDocumentsComplete(bytes calldata _data)
        external
        onlyOpenQ
    {
        (bytes32 _claimId, bool _supportingDocumentsComplete) = abi.decode(
            _data,
            (bytes32, bool)
        );
        supportingDocumentsComplete[_claimId] = _supportingDocumentsComplete;
+     if( _supportingDocumentsComplete)
                 supportingDocumentsCompleteClaimIds.push(_claimId);
    }

Duplicate of #425

[FlacoJones]

Will fix by removing Ongoing for now

[FlacoJones]

OpenQDev/OpenQ-Contracts#112

[IAm0x52]

https://github.com/sherlock-audit/2023-02-openq/blob/ba7f35654d6fa7637ef0f6db346e851fb978cde2/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L482

Should be low. It may be pushed to the array but it still uses the mapping to determine if the documents have actually been provided. So setting it to a false bool would still cause a revert, even though it's in the array. An error that it is in the list but no issue results from it.

[hrishibhat]

Agree with Lead watson comment