Out-of-Bounds Error in getPositionsForCurator Function

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The current implementation of the getPositionsForCurator function has a potential out-of-bounds error. Specifically, the credIds and amounts arrays are populated using the i variable, which starts from start_, causing an incorrect index that can lead to an out-of-bounds error and unexpected behavior.

If the provided start_ value is greater than zero, the index will start incorrectly, leading to potential array bounds errors resulting in corruption or exceptions during execution. This can disrupt the proper functionality of the getPositionsForCurator function, leading to unexpected results and breaking the logic of the contract.

Proof of Concept

Below is the problematic code in the getPositionsForCurator function:

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)
}

Tools Used

Manual code review

Recommended Mitigation Steps

Use the index variable instead of i to populate the credIds and amounts arrays, ensuring they are populated correctly even when start_ is greater than zero.

Here is the final corrected implementation for clarity:

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();
    }

    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;
+            credIds[index] = credId;
-            amounts[i] = amount;
+            amounts[index] = amount;
            index++;
        }
    }
    // Resize the result array to remove unused slots
    assembly {
        mstore(credIds, index)
        mstore(amounts, index)
    }
}

Assessed type

Invalid Validation

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-a