Inheritable Smart Contract Wallet

Code Containing Vulnerability

Description

The removeBeneficiary function contains a critical vulnerability in its array management logic. When removing a beneficiary address from the beneficiaries array, the function uses Solidity's delete operation, which resets the array element to its default value (address(0)) but does not reduce the array's length. This creates "gaps" in the array rather than maintaining a contiguous list of valid beneficiary addresses.

Impact Analysis

The vulnerability has multiple severe impacts:

1. Direct Fund Loss: During distribution functions (e.g., withdrawInheritedFunds), funds sent to these address(0) entries are permanently lost as they're sent to the zero address, which has no known private key.

2. Distribution Calculation Errors: Functions that calculate distribution amounts based on the array length will divide funds incorrectly, as they count address(0) entries as valid beneficiaries.

3. Unfair Distribution: Legitimate beneficiaries receive less than their intended share because portions are allocated to non-existent beneficiaries.

4. Gas Inefficiency: Operations that iterate through the array waste gas processing these invalid entries.

5. Potential Smart Contract Locking: If distribution logic requires actions from all beneficiaries, these gaps could prevent contract progression since address(0) cannot perform transactions.

Attack Vectors

1. Malicious Owner Attack: A malicious owner could deliberately remove beneficiaries without adding new ones, creating a situation where a significant portion of funds are sent to address(0) during distribution.

2. Social Engineering Attack: An attacker could convince the owner to remove a beneficiary under pretenses, knowing this will lead to fund loss rather than proper redistribution.

Example Exploitation Scenario

Initial State

• beneficiaries = [Alice, Bob, Charlie] (3 addresses)

• Contract balance: 3 ETH

Step 1: Owner removes Bob

• removeBeneficiary(Bob) is called

• Result: beneficiaries = [Alice, address(0), Charlie]

Step 2: Funds Distribution

• 3 ETH is divided by 3 beneficiaries = 1 ETH each

• Alice receives 1 ETH

• address(0) receives 1 ETH (permanently lost)

• Charlie receives 1 ETH

Loss Quantification

• 1 ETH (33% of total funds) is permanently burned

Proof of Code

Fix Recommendation

The vulnerability should be fixed by implementing a proper "swap-and-pop" pattern:

This approach:

1. Replaces the element to be removed with the last element in the array

2. Removes the last element using pop()

3. Maintains a contiguous array with no gaps

4. Properly reduces the array length

Additional Recommendations

1. Validation: Add explicit validation to ensure the beneficiary exists before removal.

2. Event Emission: Emit an event after beneficiary removal for transparency.

3. Zero Address Check: Implement a zero address check when adding beneficiaries.

4. Active Beneficiary Count: Consider maintaining a separate counter for active beneficiaries.

5. Testing: Implement comprehensive unit tests to verify proper array management.

References

1. Solidity Documentation on Array Operations: https://docs.soliditylang.org/en/latest/types.html#arrays

2. Common Smart Contract Vulnerabilities: Array Manipulation Issues

3. OpenZeppelin's EnumerableSet library as an alternative data structure