Cred.sol#getPositionsForCurator() reverts when _start > 0.

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L513
https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L514

Vulnerability details

Impact

This vulnerability causes reverts because of out-of-range or gives wrong result, and this makes external users or contracts which depends on this function to suffer.

Proof of Concept

Cred.sol#getPositionsForCurator() function is as follows.

    function getPositionsForCurator(
        address curator_,
        uint256 start_,
        uint256 stop_
    )
        external
        view
        returns (uint256[] memory credIds, uint256[] memory amounts)
    {
        uint256[] storage userCredIds = _credIdsPerAddress[curator_];

        uint256 stopIndex;
        if (userCredIds.length == 0) {
            return (credIds, amounts);
        }
        if (stop_ == 0 || stop_ > userCredIds.length) {
            stopIndex = userCredIds.length;
        } else {
            stopIndex = stop_;
        }

        if (start_ >= stopIndex) {
            revert InvalidPaginationParameters();
        }

505     credIds = new uint256[](stopIndex - start_);
506     amounts = new uint256[](stopIndex - start_);

        uint256 index = 0;
        for (uint256 i = start_; i < stopIndex; i++) {
            uint256 credId = userCredIds[i];
            if (_credIdExistsPerAddress[curator_][credId]) {
                uint256 amount = shareBalance[credId].get(curator_);
513             credIds[i] = credId;
514             amounts[i] = amount;
                index++;
            }
        }
        // Resize the result array to remove unused slots
        assembly {
            mstore(credIds, index)
            mstore(amounts, index)
        }
    }

As we can see above, the length of credIds and amounts created on L505 ~ 506 is stopIndex - start_.
But on L513~514, it used i iterating start_~stopIndex as index.
This causes reverts because of out-of-range.

Tools Used

Manual Review

Recommended Mitigation Steps

Cred.sol#getPositionsForCurator() function has to be modified as follows.

    function getPositionsForCurator(
        address curator_,
        uint256 start_,
        uint256 stop_
    )
        external
        view
        returns (uint256[] memory credIds, uint256[] memory amounts)
    {
        uint256[] storage userCredIds = _credIdsPerAddress[curator_];

        ///

        credIds = new uint256[](stopIndex - start_);
        amounts = new uint256[](stopIndex - start_);

        uint256 index = 0;
        for (uint256 i = start_; i < stopIndex; i++) {
            uint256 credId = userCredIds[i];
            if (_credIdExistsPerAddress[curator_][credId]) {
                uint256 amount = shareBalance[credId].get(curator_);
-               credIds[i] = credId;
-               amounts[i] = amount;
+               credIds[index] = credId;
+               amounts[index] = amount;
                index++;
            }
        }
        // Resize the result array to remove unused slots
        assembly {
            mstore(credIds, index)
            mstore(amounts, index)
        }
    }

Assessed type

Error

[ZaK3939]

Let me update by Mitigation Steps

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[McCoady]

Given that this is an external read only function and it's functionality does still work providing the user passes _start == 0, the impact of the issue is effectively zero, therefore the severity should likely be low.

[mcgrathcoutinho]

Hi @fatherGoose1, I believe this issue should be of QA severity.

This function will most likely be used by the UI, so a similar loop functionality on the frontend to retrieve these values from the storage directly could also be used. Overall, since there is no significant impact on the contract level i.e. no other function uses this getter, it should be QA at best.

[kuprumxyz]

There is a solid reason why getPositionsForCurator is implemented by the sponsor with pagination, i.e. the ability to retrieve the positions piecewise: this means there is an external system which depends on the proper functioning of this code. And, according to C4 Severity Categorization this warrants Medium severity:

Assets not at direct risk, but the function of the protocol or its availability could be impacted

[mcgrathcoutinho]

@kuprumxyz Please state which external system you're talking about here - there is an external system.

This getter function will most likely be used by the frontend. Provided the contest README did not mention/provide information about any other external contract systems, a non-existent impact cannot be determined and associated to it. This is of QA-severity at best. You have yourself submitted this as a QA.

[kuprumxyz]

@mcgrathcoutinho I am not the sponsor to comment which is the external system they are using: they know this better. In another finding they commented e.g.

The real problem is that this mismatch could cause failures in frontend search functionality and other features.

I guess the above comment applies equally to this finding. Taking into account that the sponsor did confirmed the finding, and said "Let me update by Mitigation Steps" it means that they, knowing their external systems, agree with the severity. Unless the sponsor comments to the contrary, this finding is valid.

[mcgrathcoutinho]

@kuprumxyz Why raise this issue when the sponsor has already mentioned it is going to be used on the frontend?

Just because a finding is confirmed by the sponsor that does not mean the severity of the issue is a Medium. Let's leave this upto the judge to decide considering we both have brought forth our arguments here.

[kuprumxyz]

This function will most likely be used by the UI, so a similar loop functionality on the frontend to retrieve these values from the storage directly could also be used. Overall, since there is no significant impact on the contract level i.e. no other function uses this getter, it should be QA at best.

Just a side note, but: where in the rules do you see that issues affecting external systems, such as UIs / frontends are QA? Could you please kindly provide me with a link?

Suppose the following scenario:

• getPositionsForCurator is called from a frontend, and returns a wrong result, as outlined in this finding: e.g. instead of a position with credId being 100, it returns 101.
• The user looks up in the frontend who is the artist / artworks with id 101, decides that those are uninteresting to them
• The user decides to sell the position; and the frontend, due to the above error, sells the position with id 100 instead.
• The user experiences a loss, because id 100 actually represents a respected artist with valuable artworks.

As another example, suppose due to the external view function, which is used nowhere in the contract itself, your frontend (say Metamask) shows an incorrect balance on your ERC-20 account. Is it a serious error? I suppose it is, as you will be either not aware of the funds you have, or try to spend wrong amounts with reverting transactions.

[fatherGoose1]

Sponsor confirmed that this only affects Frontend currently. There are no references to this external function from any other Phi contracts. This is valid QA.

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-a

[c4-judge]

fatherGoose1 marked the issue as not selected for report