FadoBagi - Incorrect Handling of fixedOngoingWithdrawalUsers Leads to Inconsistencies

FadoBagi

High

Incorrect Handling of `fixedOngoingWithdrawalUsers` Leads to Inconsistencies

Summary

The LidoVault contract manages a dynamic array fixedOngoingWithdrawalUsers to track users with ongoing fixed withdrawals. However, it uses the delete operation to remove users from this array, which sets the element to address(0) without removing it. This results in gaps within the array, leading to inconsistent state management and increased gas consumption during array iterations.

Vulnerability Detail

In the finalizeVaultOngoingFixedWithdrawals, claimOngoingFixedWithdrawals, claimFixedVaultOngoingWithdrawal functions, the contract removes users from the fixedOngoingWithdrawalUsers array by setting their address to address(0).

Example:

function finalizeVaultOngoingFixedWithdrawals() external {
    // ...
    uint256 arrayLength = fixedOngoingWithdrawalUsers.length;
    for (uint i = 0; i < arrayLength; i++) {
      if (fixedOngoingWithdrawalUsers[i] == msg.sender) {
        delete fixedOngoingWithdrawalUsers[i];
      }
    }
    // ...
}

This is leaving gaps with address(0). Using delete fixedOngoingWithdrawalUsers[i]; sets the array element to address(0) without reducing the array's length. The array retains its original length, with address(0) occupying the spot of the deleted user.

As the array grows with address(0) entries, loops become more gas-intensive.

Extremely large arrays with numerous address(0) entries can cause critical functions to fail due to exceeding gas limits.

Proof of Concept:

function poc() pure external {
    address[1] memory fixedOngoingWithdrawalUsers = [
        0x1234567890123456789012345678901234567890
    ];

    uint256 arrayLength = fixedOngoingWithdrawalUsers.length;
    for (uint256 i = 0; i < arrayLength; i++) {
        if (fixedOngoingWithdrawalUsers[i] == 0x1234567890123456789012345678901234567890) {
            delete fixedOngoingWithdrawalUsers[i];
        }
    }

    // Log the length and elements of the array
    console.log("Array Length:", arrayLength);
    console.log("Element 0:", fixedOngoingWithdrawalUsers[0]);
}

Impact

The improper handling of the fixedOngoingWithdrawalUsers array leads to increased gas costs, out-of-gas errors, and inconsistent contract states. This can prevent users from successfully withdrawing their funds.

Code Snippet

Function: finalizeVaultOngoingFixedWithdrawals https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L595-L607

Function: claimOngoingFixedWithdrawals https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L690-L697

Function: claimFixedVaultOngoingWithdrawal https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L812-L843

Tool used

Manual Review

Recommendation

Replace the fixedOngoingWithdrawalUsers array with a mapping to track ongoing withdrawals, eliminating gaps and reducing gas consumption. If maintaining an array is necessary for enumeration, implement an efficient removal method, such as swapping the user with the last element and popping the array, to prevent address(0) entries and ensure consistent state management.

sherlock-audit / 2024-08-saffron-finance-judging

FadoBagi - FadoBagi - Incorrect Handling of fixedOngoingWithdrawalUsers Leads to Inconsistencies #118