Mid_03_Unexpected_Results_In_Paginations

Summary

The AddressPagination::paginate function returns incorrect results when the requested page exceeds the available data, resulting in unexpected behaviour such as trailing address(0) in the output array.

The same faulty logic can be observed in the LockList::getLocks

Vulnerability Detail

The current implementation of the AddressPagination::paginate function does not handle cases where the requested page and limit combination exceeds the bounds of the input array. Specifically, if the computed index exceeds the array length, the function inserts an address(0) into the results array. This behaviour can lead to silent errors and unexpected outputs, which may cause issues in the larger application logic.

https://github.com/sherlock-audit/2024-05-gamma-staking/blob/main/StakingV2/src/libraries/AddressPagination.sol#L15

The current implementation of the LockList::getLocks function does not handle cases where the requested page and limit combination exceeds the bounds of the input array. Specifically, if the computed index exceeds the array length, the function inserts an empty locked balance into the results array. This behaviour can lead to silent errors and unexpected outputs, which may cause issues in the larger application logic.

https://github.com/sherlock-audit/2024-05-gamma-staking/blob/main/StakingV2/src/libraries/LockList.sol#L97

Impact

If the function is called with arguments that request data beyond the available array length, it returns a result array padded with zeros instead of reverting or indicating an error. This could lead to misleading data being processed in subsequent operations, potentially causing logical errors and undermining the reliability of the application.

While the AddressPagination::paginate function is currently not used anywhere, the LockList::getLocks function is used in Lock::withdrawAllUnlockedToken and LockList::lockedBalances functions (these are of high importance to the contract's behaviour).

Code Snippet

Original Functions:

function paginate(
    address[] memory array,
    uint256 page,
    uint256 limit
) internal pure returns (address[] memory result) {
    result = new address[](limit);
    for (uint256 i = 0; i < limit; i++) {
        if (page * limit + i >= array.length) {
            result[i] = address(0);
        } else {
            result[i] = array[page * limit + i];
        }
    }
}

function getLocks( 
    address user, 
    uint256 page, 
    uint256 limit 
)
public view override returns (LockedBalance[] memory) {
    LockedBalance[] memory locks = new LockedBalance[](limit);
    uint256 lockIdsLength = lockIndexesByUser[user].length(); 
    uint256 i = page * limit; 
    for (;i < (page + 1) * limit && i < lockIdsLength; i ++) { 
        locks[i - page * limit]= lockById[lockIndexesByUser[user].at(i)]; 
    } 
    return locks; 
}

Proof Of Concept

Example of Incorrect Behaviour for AddressPagination::paginate:

Let's have a look with the same AddressPagination::paginate function but with integers.

The function looks like this:

function paginate(
    int256[] memory array,
    uint256 page,
    uint256 limit
) external pure returns (int256[] memory result) {
    result = new int256[](limit);
    for (uint256 i = 0; i < limit; i++) {
        if (page * limit + i >= array.length) {
            result[i] = 0;
        } else {
            result[i] = array[page * limit + i];
        }
    }
}

Calling the function with ([1,2,3,4,5], 1, 3) returns [4, 5, 0] instead of [4, 5].
Calling the function with ([1,2,3,4,5], 2, 3) returns [0, 0, 0] instead of reverting or returning [].

Example of Incorrect Behaviour for LockList::getLocks:

Let's write a test to see if the lockCount is the same as if we gather all locks using LockList::getLocks and take the array's length:

function testGetLocksPagination() public {
    vm.startPrank(deployer);
    uint256 i;
    for (i; i < 10; i++) {
        LockedBalance memory lockedBalance = LockedBalance({
            lockId: 0,
            amount: 100,
            unlockTime: i,
            multiplier: i + 1,
            lockTime: i + 2,
            lockPeriod: 0,
            exitedLate: false
        });

        lockList.addToList(user1, lockedBalance);
    }
    uint256 lockCount = lockList.lockCount(user1);
    uint256 lockCountWithPagination = lockList
        .getLocks(user1, 0, 11)
        .length;
    assertEq(lockCountWithPagination, lockCount);
    vm.stopPrank();
}

This test adds 10 locked balances to user1. After this it gets the lockCount using LockList::lockCount and tries to get 11 locks from page 0 using LockList::getLocks. The expected behaviour is for both lockCount and lockCountWithPagination to be equal to each other and to be equal to 10 as that's how much locked balances user1 possesses.

However, the test fails due to the fact that LockList::getLocks will always return a list with the number of elements set to the limit variable (even if the user has no locked balances).

The test can be ran with forge test --match-test testGetLocksPagination.

Tool used

Manual Review

Recommendation

Refactor the functions to include bounds checking and ensure it reverts on invalid input, preventing the return of incorrect and misleading data.

Proposed Fix:

function paginate(
    address[] memory array,
    uint256 page,
    uint256 limit
) external pure returns (address[] memory result) {
+   require(array.length > page * limit, "Invalid request for pagination");
+   uint256 elements = limit;

+   if((page + 1) * limit >= array.length) {
+       elements -= (((page + 1) * limit) - array.length);
+   }

-   result = new address[](limit);
+   result = new address[](elements);

-   for (uint256 i = 0; i < limit; i++) {
+   for (uint256 i = 0; i < elements; i++) {
-       if (page * limit + i >= array.length) {
-           result[i] = address(0);
-       } else {
        result[i] = array[page * limit + i];
-       }
    }

+   return result;
}

This revised AddressPagination::paginate function ensures that any attempt to paginate beyond the available data will result in a revert, thus avoiding silent errors and ensuring consistent and reliable behaviour.

A similar rework should be done to LockList::getLocks.

sherlock-audit / 2024-05-gamma-staking-judging

petarP1998 - Mid_03_Unexpected_Results_In_Paginations #143