Inheritable Smart Contract Wallet

Components Affected: _getBeneficiaryIndex function, removeBeneficiary function

Code Containing Vulnerability

Description

The _getBeneficiaryIndex function contains a logical flaw. When searching for a beneficiary address that isn't present in the array, the function reaches the end of the loop without finding a match but doesn't explicitly handle this case. In Solidity, when a function with a return value doesn't explicitly return a value, it returns the default value for that type - in this case, 0 for uint256.

This means that non-existent addresses will cause the function to return 0, falsely indicating that the address is at index 0 of the array. When this function is used by removeBeneficiary, it leads to the improper deletion of the first beneficiary, rather than detecting the error condition.

Impact Analysis

The vulnerability has multiple severe consequences:

1. Accidental Deletion: The legitimate beneficiary at index 0 can be accidentally deleted when attempting to remove a non-existent beneficiary.

2. Privilege Escalation: Any user with permission to call removeBeneficiary can target the first beneficiary without needing to know their address.

3. Cascading Fund Loss: Combined with the array gap issue, this vulnerability creates empty slots in the array, leading to fund loss when distributions occur.

4. Data Integrity Compromise: The beneficiary list becomes corrupted and no longer accurately represents the intended recipients.

5. Trust Breakdown: Beneficiaries cannot trust the system to maintain their status correctly, potentially leading to reputational damage.

Attack Vectors

1. Malicious Owner Attack: An owner could deliberately attempt to remove a non-existent beneficiary to silently eliminate the first beneficiary without raising suspicion.

2. Accidental Misuse: Even without malicious intent, contract administrators might accidentally remove the wrong beneficiary by specifying an incorrect address.

3. Denial of Service: An attacker could repeatedly call this function with invalid addresses to systematically remove legitimate beneficiaries from index 0.

Example Exploitation Scenario

Initial State

• beneficiaries = [Alice, Bob, Charlie]

• Contract balance: 3 ETH

Step 1: Owner attempts to remove non-existent beneficiary "Dave"

• removeBeneficiary(Dave) is called

• _getBeneficiaryIndex(Dave) returns 0 (default value)

• delete beneficiaries[0] executes, removing Alice instead

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

Step 2: Funds Distribution

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

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

• Bob receives 1 ETH

• Charlie receives 1 ETH

Loss Quantification

• Alice loses her rightful share (1 ETH)

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

Proof of Concept

Fix Recommendation

The vulnerability should be fixed by explicitly handling the case where a beneficiary is not found:

This approach:

1. Explicitly checks for non-existent beneficiaries

2. Reverts the transaction if the beneficiary is not found

3. Prevents accidental deletion of the beneficiary at index 0

4. Provides clear error messaging

Additional Recommendations

1. Strict Validation: Add validation in all functions that modify the beneficiary list.

2. Two-Step Operations: Consider implementing a two-step process for removing beneficiaries (propose + confirm).

3. Events: Emit events for all beneficiary modifications to provide transparency.

4. Access Control Review: Review all functions that can modify the beneficiary list to ensure proper access controls.

5. Consider Enumerable Sets: Use a data structure like OpenZeppelin's EnumerableSet that provides safe operations for membership testing and removal.

6. Comprehensive Testing: Implement tests that verify correct behavior with both existing and non-existent beneficiaries.