Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 31 additions & 17 deletions contracts/utils/Arrays.sol
Original file line number Diff line number Diff line change
Expand Up @@ -112,27 +112,41 @@ library Arrays {
* IMPORTANT: Memory locations between `begin` and `end` are not validated/zeroed. This function should
* be used only if the limits are within a memory array.
*/
// @custom:oz-reentrancy-enginespec note
// The function reverts if called with arrays larger than ~170 elements due to EVM stack depth limits
// when using two recursive calls. This limit is raised to >1000 elements by using an iterative
// approach for the larger partition (tail recursion optimization via loop).
function _quickSort(uint256 begin, uint256 end, function(uint256, uint256) pure returns (bool) comp) private pure {
unchecked {
if (end - begin < 0x40) return;

// Use first element as pivot
uint256 pivot = _mload(begin);
// Position where the pivot should be at the end of the loop
uint256 pos = begin;

for (uint256 it = begin + 0x20; it < end; it += 0x20) {
if (comp(_mload(it), pivot)) {
// If the value stored at the iterator's position comes before the pivot, we increment the
// position of the pivot and move the value there.
pos += 0x20;
_swap(pos, it);
while (true) {
if (end - begin < 0x40) return;

// Use first element as pivot
uint256 pivot = _mload(begin);
// Position where the pivot should be at the end of the loop
uint256 pos = begin;

for (uint256 it = begin + 0x20; it < end; it += 0x20) {
if (comp(_mload(it), pivot)) {
// If the value stored at the iterator's position comes before the pivot, we increment the
// position of the pivot and move the value there.
pos += 0x20;
_swap(pos, it);
}
}
}

_swap(begin, pos); // Swap pivot into place
_quickSort(begin, pos, comp); // Sort the left side of the pivot
_quickSort(pos + 0x20, end, comp); // Sort the right side of the pivot
_swap(begin, pos); // Swap pivot into place
// Recurse into the smaller partition and loop for the larger one.
// This converts one recursive call into an iterative tail call, dramatically
// reducing stack depth and raising the maximum sortable array size from ~170 to >1000.
if (pos - begin < end - (pos + 0x20)) {
_quickSort(begin, pos, comp); // Sort the left side of the pivot
begin = pos + 0x20; // do another loop without recursion
} else {
_quickSort(pos + 0x20, end, comp); // Sort the right side of the pivot
end = pos; // do another loop without recursion
}
}
}
}

Expand Down
35 changes: 35 additions & 0 deletions test/utils/Arrays.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,41 @@
}
}

/// Large array sort tests (stack depth fix)
// See https://github.com/OpenZeppelin/openzeppelin-contracts/issues/6289

function testSortLargeArrayDescending() public pure {
// Previously caused stack overflow with N >= 170 (EVM stack depth limit)
// With the iterative fix, this should now succeed for arrays >> 170 elements
uint256 N = 500;

Check failure on line 403 in test/utils/Arrays.t.sol

View workflow job for this annotation

GitHub Actions / lint

Variable name must be in mixedCase N
uint256[] memory a = new uint256[](N);
for (uint256 i = 0; i < N; i++) {
a[i] = N - i; // descending: N, N-1, ..., 1
}
Arrays.sort(a);
// After sort, should be ascending: 1, 2, ..., N
for (uint256 i = 1; i < N; i++) {
assertEq(a[i - 1] < a[i], true);
}
}

function testSortVeryLargeArrayRandom() public pure {
// Even larger array to confirm iterative approach handles high element counts
uint256 N = 1000;

Check failure on line 417 in test/utils/Arrays.t.sol

View workflow job for this annotation

GitHub Actions / lint

Variable name must be in mixedCase N
uint256[] memory a = new uint256[](N);
uint256 seed = 42;
for (uint256 i = 0; i < N; i++) {
// Deterministic pseudo-random for reproducibility
seed = keccak256(abi.encode(seed, i));
a[i] = uint256(seed) % (N * 10);
}
Comment on lines +419 to +424
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Type mismatch: keccak256 returns bytes32, not uint256.

Line 422 assigns a bytes32 value to a uint256 variable without explicit conversion. In Solidity 0.8.x, this implicit conversion is not allowed and will cause a compilation error.

🐛 Proposed fix
         uint256 seed = 42;
         for (uint256 i = 0; i < N; i++) {
             // Deterministic pseudo-random for reproducibility
-            seed = keccak256(abi.encode(seed, i));
-            a[i] = uint256(seed) % (N * 10);
+            seed = uint256(keccak256(abi.encode(seed, i)));
+            a[i] = seed % (N * 10);
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uint256 seed = 42;
for (uint256 i = 0; i < N; i++) {
// Deterministic pseudo-random for reproducibility
seed = keccak256(abi.encode(seed, i));
a[i] = uint256(seed) % (N * 10);
}
uint256 seed = 42;
for (uint256 i = 0; i < N; i++) {
// Deterministic pseudo-random for reproducibility
seed = uint256(keccak256(abi.encode(seed, i)));
a[i] = seed % (N * 10);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/utils/Arrays.t.sol` around lines 419 - 424, The assignment to variable
seed uses keccak256 which returns bytes32, causing a type mismatch; change the
assignment to cast the keccak256 result to uint256 (e.g., set seed =
uint256(keccak256(abi.encode(seed, i)))) so seed remains uint256 and subsequent
use in a[i] = uint256(seed) % (N * 10) is valid; update the line that calls
keccak256(abi.encode(seed, i)) accordingly and keep references to seed,
keccak256, abi.encode, and array a.

Arrays.sort(a);
// Verify sorted ascending
for (uint256 i = 1; i < N; i++) {
assertEq(a[i - 1] <= a[i], true);
}
}

/// Helpers

function _copyArray(uint256[] memory values) internal pure returns (uint256[] memory) {
Expand Down
Loading