HonorLt - Wrong indication of ongoing invoice and supported documents complete

[github-actions]

HonorLt

medium

Wrong indication of ongoing invoice and supported documents complete

Summary

Ongoing bounty setInvoiceComplete and setSupportingDocumentsComplete do not respect the decoded bool value.

Vulnerability Detail

setInvoiceComplete and setSupportingDocumentsComplete always push to the respective array, no matter if the boolean flag is true or false:

    function setInvoiceComplete(bytes calldata _data) external onlyOpenQ {
        (bytes32 _claimId, bool _invoiceComplete) = abi.decode(
            _data,
            (bytes32, bool)
        );
        invoiceComplete[_claimId] = _invoiceComplete;
        invoiceCompleteClaimIds.push(_claimId);
    }

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

These arrays are then exposed over getters:

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

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

These results will be included in the getters array no matter if the boolean value was true or false.

Impact

Functions that rely on these values, can misbehave by treating the data as correct when the actual flag was false.

Code Snippet

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

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

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

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

Tool used

Manual Review

Recommendation

Only push to the array if the flag is true. If the flag is false, and the entry was present in the array, then should probably remove it.

Duplicate of #425