Inheritable Smart Contract Wallet

Summary

The removeBeneficiary function in the InheritanceManager.sol uses the delete keyword to remove an element from the beneficiaries array. However, delete beneficiaries[indexToRemove]; does not properly remove the element but instead sets it to its default value, creating a "hole" in the array. This causes storage fragmentation, leading to increased gas costs, inconsistent contract behavior, and potential security risks when iterating over the array.

Vulnerability Details

Storage Fragmentation (Array Holes)

When delete beneficiaries[indexToRemove]; is used:

• The element at indexToRemove is not actually removed; it is set to its default value (address(0), false, 0, etc.).

• The array length remains the same, meaning any function that loops through the array may read and process an empty slot.

This causes:

• Incorrect beneficiary retrieval (since a deleted slot remains in the array).

• Looping issues in functions iterating over beneficiaries, possibly counting address(0) as a valid entry.

Impact

Deleting an element with delete still performs a storage write, setting the slot to its default value.

• Unnecessary gas is spent when iterating over the array, since it still has the same length and includes empty slots.

• Future operations involving iteration over beneficiaries will require additional logic to skip empty slots, making operations more expensive.

• If another function relies on beneficiaries.length to determine active members, it may incorrectly assume a deleted slot is still valid.

Tools Used

manual review

Recommendations

Instead of using delete, use the swap-and-pop pattern to efficiently remove elements

Benefits of This Fix:

• No empty slots (prevents fragmentation).

• Reduces gas costs (no unnecessary storage writes).

• Maintains a compact, iterable array.