Inheritable Smart Contract Wallet

Summary

The InheritanceManager contract is intended to facilitate asset inheritance by allowing an owner to designate beneficiaries who can claim assets after a specified period of inactivity. However, a critical vulnerability exists in the removeBeneficiary function: it does not properly remove beneficiaries from the beneficiaries array. Instead of resizing the array to eliminate the beneficiary’s entry, it merely sets the entry to address(0), creating a "gap." This leads to incorrect behavior in functions that iterate over the array, such as asset distribution logic, potentially causing assets to be sent to address(0) resulting in their loss or unintended burning.

Vulnerability Details

1. Failure to Shrink the Array:
   In Solidity, the delete keyword resets the value at the specified index to its default value (address(0) for an address type) but does not alter the array’s length. As a result, the beneficiaries array retains its original size, with an invalid address(0) entry remaining in place.

2. Creation of Gaps:
   After the delete operation, the array contains address(0) at the removed index, representing an invalid beneficiary. This "gap" persists in the array, disrupting its integrity.

3. Impact on Iterating Functions:
   Functions such as withdrawInheritedFunds, which iterate over the beneficiaries array to distribute assets, treat address(0) as a valid entry due to the unchanged array length. This causes:

   • Miscalculation of Shares: The total number of beneficiaries (including address(0)) inflates the divisor, reducing the share each legitimate beneficiary receives.

   • Unintended Transfers: Assets may be sent to address(0), depending on the asset type and transfer logic.

4. Asset Loss via address(0):

   • ETH: Sending Ether to address(0) typically results in the funds being burned or lost, as no account can claim them.

   • ERC20 Tokens: Depending on the token contract, transfers to address(0) may fail (reverting the transaction) or succeed (burning the tokens), both of which disrupt the intended distribution.

Vulnerable Code

The removeBeneficiary function is currently implemented as follows:

Example Scenario

Consider the following:

• Initial State: beneficiaries = [Alice, Bob, Charlie] (length = 3).

• Action: The owner calls removeBeneficiary(Bob).

• Resulting State: beneficiaries = [Alice, address(0), Charlie] (length remains 3).

• Asset Distribution: In withdrawInheritedFunds:

  • Assets are split into three shares (based on array length).

  • Alice receives 1/3, address(0) "receives" 1/3 (lost or burned), and Charlie receives 1/3.

• Outcome: Instead of a fair two-way split between Alice and Charlie (50% each), each receives only 33%, with 33% of the assets effectively discarded.

Proof of Concept

Below is a Foundry test demonstrating the vulnerability:

Impact

This flaw has severe consequences:

• Asset Loss:
  Assets sent to address(0) are unrecoverable (for ETH) or potentially burned (for ERC20 tokens), resulting in permanent financial loss.

• Incorrect Distribution:
  The inclusion of address(0) in the array length distorts share calculations, reducing the inheritance received by legitimate beneficiaries.

• Unreliable Behavior:
  Functions interacting with address(0) may fail unpredictably or produce inconsistent results, undermining the contract’s reliability.

• Dilution of Shares:
  Valid beneficiaries receive less than intended due to the inflated beneficiary count.

• Violation of Intent:
  The owner’s action to remove a beneficiary is not fully executed, as the array continues to account for the removed entry.

Tools Used

Manual Review

Recommendations

To resolve this flaw and ensure proper beneficiary management, the following steps are recommended:

1. Implement Proper Array Resizing:
   Modify removeBeneficiary to eliminate the target entry and reduce the array’s length by shifting elements. A common approach is to swap the element to be removed with the last element and then remove the last element:

   function removeBeneficiary(address _beneficiary) external onlyOwner {

   uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);

   require(beneficiaries[indexToRemove] != address(0), "Beneficiary not found");

   beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];

   beneficiaries.pop();

   }

   • Benefits: Eliminates gaps, ensures array length reflects active beneficiaries, and prevents address(0) from affecting distribution.

2. Address Edge Cases:

   • Last Element Removal: The updated logic naturally handles removing the last element without errors.

   • Non-Existent Beneficiaries: Add validation to revert if the beneficiary is not found or is already address(0).