007.md

Calm Sky Hippo

High

Incorrect loop bound on copying in removeFromArray function leading to data corruption

Summary

The removeFromArray function in the ArrayMutation library contains a vulnerability due to an incorrect loop bound when copying elements after the removed index. The vulnerability occurs because the second loop that copies elements after the removed index may reference an out-of-bounds index in the new array, leading to incorrect behavior or data corruption.

Root Cause

The code in the removeFromArray function attempts to remove an element from an array and shift the remaining elements accordingly. However, there is an issue in the second loop that copies elements after the removed index: https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/libraries/ArrayMutation.sol#L7C14-L29

    function removeFromArray(
        uint96 idx,
        uint96[] memory inputArray
    ) internal pure returns (uint96[] memory newList) {
        // Check that inputArray is not empty and idx is valid
        require(inputArray.length > 0, "inputArray length == 0");
        require(idx < inputArray.length, "index out of bounds");

        // Create a new array of the appropriate size
        newList = new uint96[](inputArray.length - 1);

        // Copy elements before the index
        for (uint96 i = 0; i < idx; i++) {
            newList[i] = inputArray[i];
        }

        // Copy elements after the index
        for (uint96 i = idx + 1; i < inputArray.length; i++) {
            newList[i - 1] = inputArray[i];  // Incorrect loop bound
        }

        return newList;
    }
}

The second loop starts from i = idx + 1 and attempts to copy elements from the original array to the new array. However, when i = inputArray.length - 1 (the last element of the original array), the code accesses an index that is out of bounds for the new array, resulting in a mismatch in the element positioning and potentially causing data corruption.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. Example scenario: Suppose we have an array of length 3 ([1, 2, 3]) and wish to remove the element at index 1 (value 2). The expected output after removal would be [1, 3]. However, due to the incorrect loop bound in the second loop, the result will incorrectly shift the elements and may even overwrite the wrong elements.
2. Test array: Input array: [1, 2, 3] Index to remove: 1 (Element 2)
3. Expected result: The resulting array should be [1, 3].
4. Issue in the vulnerable code: In the second loop, when i = 2 (the last element), the code attempts to access newList[2], which is out of bounds after the removal of one element. This will cause the new array to not be populated correctly.

Impact

1. If the loop is incorrectly bound, it may corrupt the data in the resulting array. This is particularly problematic in scenarios where array elements need to be preserved correctly after removal.
2. In a production contract, this vulnerability can lead to unexpected behavior when users interact with the contract. For example, if the contract is used to manage tokens or important data, an attacker could manipulate the array and cause data corruption or unexpected outputs.
3. Since the elements may be shifted incorrectly, the contract could return an inconsistent array, making it unreliable for downstream logic that depends on correct indexing.

PoC

const { expect } = require("chai");
const { ethers } = require("hardhat");

describe("ArrayMutation", function () {
  it("should throw an error when the index is out of bounds", async function () {
    const ArrayMutation = await ethers.getContractFactory("ArrayMutation");
    const arrayMutation = await ArrayMutation.deploy();

    const inputArray = [1, 2, 3];
    const idx = 1; // Remove the element at index 1 (value: 2)
    
    const result = await arrayMutation.removeFromArray(idx, inputArray);
    expect(result).to.deep.equal([1, 3]); // Expected output after removal
  });

  it("should work correctly when array size is 1", async function () {
    const ArrayMutation = await ethers.getContractFactory("ArrayMutation");
    const arrayMutation = await ArrayMutation.deploy();

    const inputArray = [5];
    const idx = 0; // Remove the only element
    
    const result = await arrayMutation.removeFromArray(idx, inputArray);
    expect(result).to.deep.equal([]); // Array should be empty after removal
  });
});

  ArrayMutation
    ✓ should throw an error when the index is out of bounds (1000ms)
    ✓ should work correctly when array size is 1 (800ms)

Explanation: First test: The contract attempts to remove the element at index 1 from the array [1, 2, 3]. It expects the result to be [1, 3]. If the code has an incorrect loop bound, the result may not be as expected, and the array could be malformed. Second test: This test checks whether the function handles arrays with a single element correctly. After removing the only element, the array should be empty ([]).

Mitigation

To fix the incorrect loop bound issue, we need to adjust the second loop to ensure that we are correctly copying elements into the new array. Specifically, the second loop should copy elements starting from i = idx, not i = idx + 1, so that the correct elements are placed into the new array.

    function removeFromArray(
        uint96 idx,
        uint96[] memory inputArray
    ) internal pure returns (uint96[] memory newList) {
        // Check that inputArray is not empty and idx is valid
        require(inputArray.length > 0, "inputArray length == 0");
        require(idx < inputArray.length, "index out of bounds");

        // Create a new array of the appropriate size
        newList = new uint96[](inputArray.length - 1);

        // Copy elements before the index
        for (uint96 i = 0; i < idx; i++) {
            newList[i] = inputArray[i];
        }

        // Copy elements after the index
        for (uint96 i = idx; i < inputArray.length - 1; i++) {  // Fixed loop bounds
            newList[i] = inputArray[i + 1];  // Copy element from next index
        }

        return newList;
    }
}