060.md

Glamorous Plum Baboon

High

Improper Handling of Arrays in getReservesList

Summary

The function getReservesList improperly handles array operations by assuming that the index i - droppedReservesCount is always valid. In cases where droppedReservesCount > i, this assumption can lead to an underflow in older Solidity versions or a runtime revert in Solidity >= 0.8.0, potentially causing contract malfunction or crashes.

Description

The issue occurs within the following snippet of code:

if (_reservesList[i] != address(0)) {
    reservesList[i - droppedReservesCount] = _reservesList[i];
} else {
    droppedReservesCount++;
}

The logic assumes that the index i - droppedReservesCount will always be valid. However, if the value of droppedReservesCount becomes greater than i, the subtraction results in an invalid index:

• In Solidity versions < 0.8.0, this would cause an underflow, leading to unintended behavior and potentially incorrect data storage.
• In Solidity versions >= 0.8.0, the subtraction will revert due to the introduction of overflow/underflow checks, resulting in the termination of the function execution.

The issue arises because edge cases where droppedReservesCount > i are not explicitly handled.

Root Cause

The code assumes that i - droppedReservesCount will always be valid without accounting for cases where droppedReservesCount > i. This results in unsafe array operations.

Line of Code (LoC):

reservesList[i - droppedReservesCount] = _reservesList[i];

https://github.com/sherlock-audit/2025-01-aave-v3-3/blob/main/aave-v3-origin/src/contracts/protocol/pool/Pool.sol#L532-L542

Impact

If this vulnerability is exploited, the following risks are present:

1. Malfunction of the contract: The function may not complete successfully, leading to unexpected behavior or incorrect array manipulation.
2. Crash of the contract: In Solidity >= 0.8.0, the function will revert when the invalid index is accessed, causing disruption to the contract's execution flow.

Proof of Concept (PoC)

Consider the following test scenario:

1. _reservesList contains an array of 5 elements where only the first element is non-zero.
2. droppedReservesCount is incremented with each zero address.
3. When processing the second element (i = 1), droppedReservesCount = 1, resulting in 1 - 1 = 0. This is valid.
4. When processing the third element (i = 2), if droppedReservesCount = 3 at that point, i - droppedReservesCount = 2 - 3 = -1. This causes:
   • Underflow in Solidity < 0.8.0.
   • Reversion in Solidity >= 0.8.0.

Foundry Test Code:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "forge-std/Test.sol";

contract ReservesListTest is Test {
    address[] private _reservesList;
    address[] private reservesList;
    uint256 private droppedReservesCount;

    function setUp() public {
        // Initialize _reservesList with 5 elements, only the first is non-zero
        _reservesList = new address      _reservesList[0] = address(0x1); // Non-zero address
        for (uint256 i = 1; i < 5; i++) {
            _reservesList[i] = address(0); // Zero addresses
        }

        // Initialize reservesList and droppedReservesCount
        reservesList = new address ;
   droppedReservesCount = 0;
    }

    function testReservesListProcessing() public {
        // Process the array
        for (uint256 i = 0; i < _reservesList.length; i++) {
            if (_reservesList[i] != address(0)) {
                // Check that i - droppedReservesCount is valid
                if (i >= droppedReservesCount) {
                    reservesList[i - droppedReservesCount] = _reservesList[i];
                } else {
                    // This should not happen if the logic is correct
                    fail("Invalid index calculation: underflow detected");
                }
            } else {
                droppedReservesCount++;
            }
        }

        // Assertions
        assertEq(reservesList[0], address(0x1), "First element should be non-zero");
        for (uint256 i = 1; i < reservesList.length - droppedReservesCount; i++) {
            assertEq(reservesList[i], address(0), "Other elements should remain zero");
        }
    }

    function testReversionOnInvalidIndex() public {
        // Simulate a condition where droppedReservesCount > i
        droppedReservesCount = 3;

        vm.expectRevert(); // Expect the function to revert
        reservesList[2 - droppedReservesCount] = _reservesList[2];
    }
}

Run Test forge test This will verify both correct handling and edge case reversion for the issue.

Recommended Mitigation

To fix this issue:

1. Use safe array operations that validate indices before assigning values.
2. Explicitly check that i >= droppedReservesCount before performing the subtraction. For example:

if (_reservesList[i] != address(0)) {
    if (i >= droppedReservesCount) {
        reservesList[i - droppedReservesCount] = _reservesList[i];
    } else {
        // Handle edge case if necessary
    }
} else {
    droppedReservesCount++;
}

3. Alternatively, consider rewriting the logic to avoid relying on subtraction entirely. Use an additional index variable to keep track of valid elements instead.