Inheritable Smart Contract Wallet

Summary

The _getBeneficiaryIndex function in the InheritanceManager contract has a flaw in its return value handling. When an address is not found in the beneficiaries array, it doesn't initialize the return value (_index), which leads to returning the default value of uint256 (0), which is indistinguishable from finding a beneficiary at index 0. This is used in the removeBeneficiary function and could lead to accidentally removing the wrong beneficiary.

Vulnerability Details

The current implementation:

This function has the following issue:

1. If the beneficiary is not found, it returns 0 (default value for uint256)

2. If the beneficiary is at index 0, it also returns 0

This ambiguity is dangerous because the function is used in removeBeneficiary where we can accidentaly remove the first beneficiary:

Impact

HIGH

• Funds are indirectly at risk as legitimate beneficiaries could lose access to their inheritance

• If beneficiary at index 0 is accidentally removed, they permanently lose their ability to:

  • Receive their share of ETH/tokens when withdrawInheritedFunds is called

  • Participate in estate settlements when buyOutEstateNFT is called

  • Call appointTrustee in order to participate in trustee appointment

• Disrupts core protocol functionality (beneficiary management)

• Affects the fundamental inheritance mechanism of the contract

Likelihood: Medium

• Can occur whenever owner tries to remove a non-existent beneficiary

• Requires specific conditions (beneficiary at index 0 exists)

• Reasonably likely to occur during normal protocol operation

• Protected by onlyOwner modifier but still a common operation

The vulnerability could lead to:

1. Accidentally removing the wrong beneficiary (the one at index 0) when trying to remove a non-existent beneficiary

2. Disruption of intended inheritance distribution

3. Complete loss of inheritance rights and associated funds for legitimate beneficiaries

Example Attack Scenario:

1. Contract has 3 beneficiaries: Alice (index 0), Bob (index 1), and Charlie (index 2)

2. Owner tries to remove non-existent beneficiary Dave

3. _getBeneficiaryIndex(Dave) returns 0

4. removeBeneficiary deletes beneficiaries[0], removing Alice instead of failing

5. When inheritance is triggered, Alice has lost all access to her share of the inheritance

6. Alice's portion of ETH, tokens, and NFT rights are permanently lost or redistributed to other beneficiaries

Tools Used

• Manual review

• Code inspection

• Foundry tests

Recommendations

1. Add a return value to indicate whether the beneficiary was found:

1. Update the removeBeneficiary function to use this return value:

1. Add appropriate error:

These changes would:

• Prevent accidental removal of wrong beneficiaries

• Provide clear feedback when a beneficiary is not found

• Make the code more robust and maintainable