Inheritable Smart Contract Wallet

https://github.com/CodeHawks-Contests/2025-03-inheritable-smart-contract-wallet/blob/main/src/InheritanceManager.sol#L163-L166

Summary

The removeBeneficiary function in the InheritanceManager contract attempts to remove a beneficiary from the beneficiaries array. However, it incorrectly uses the delete keyword, which only resets the array element to its default value (address(0)) instead of actually removing it. This results in an array with "gaps" and an incorrect length, which can lead to several issues in other functions that rely on this array, particularly withdrawInheritedFunds and buyOutEstateNFT.

Vulnerability Details

1. removeBeneficiary Function:

   • Intended Functionality: The function aims to remove a beneficiary from the list of beneficiaries (beneficiaries).

   • Incorrect Implementation: It uses delete beneficiaries[indexToRemove];.

   • Actual Behavior: The delete keyword in this context does not remove an element from the array. It only resets the value at the specified index to the default value for its type (which is address(0) for addresses).

   • Result: The beneficiaries array will contain address(0) entries, and its length will remain the same after a beneficiary is "removed."

   • Code Snippet:

1. beneficiaries Array State:

   • Incorrect Data: After calling removeBeneficiary, the beneficiaries array will contain address(0) entries where beneficiaries were supposed to be removed.

   • Incorrect Length: The array's length property will not change, even though logically, the number of valid beneficiaries has decreased.

Proof of Concept

Add this test to the test suite InheritanceManagerTest at path test/InheritanceManagerTest.t.sol and run the command forge test --mt test_removeBeneficiaryBug:

From the test, after removing user1, it is replaced with address(0). The assertion confirms this.

Impact

1. withdrawInheritedFunds Errors:

   • Incorrect Divisor: The withdrawInheritedFunds function calculates the amount to distribute to each beneficiary by dividing the total amount by beneficiaries.length. If address(0) entries are present, the divisor will be incorrect, leading to an incorrect amount being sent to each beneficiary.

   • Failed Transfers: When the loop reaches an address(0) entry, calling call or safeTransfer on it will likely fail or revert.

   • Potential Loss of Funds: Because of the incorrect divisor some eth can get lost forever.

   function withdrawInheritedFunds(address _asset) external {

   uint256 divisor = beneficiaries.length; // Incorrect if address(0) are present

   // ...

   for (uint256 i = 0; i < divisor; i++) {

   address payable beneficiary = payable(beneficiaries[i]);// will contain address(0)

   //...

   }

   }

2. buyOutEstateNFT Errors:

   • Incorrect Divisor: Similar to withdrawInheritedFunds, buyOutEstateNFT also uses beneficiaries.length as a divisor when calculating payment amounts. This will be incorrect if address(0) entries exist.

   • Incorrect Transfers: The loop will attempt to transfer to address(0) entries, leading to incorrect behavior.

   function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {

   uint256 divisor = beneficiaries.length; // Incorrect if address(0) are present

   // ...

   for (uint256 i = 0; i < beneficiaries.length; i++) {

   // will try to transfer to address(0)

   }

   }

3. Incorrect Logic: Other functions that iterate through the beneficiaries array may encounter unexpected behavior due to the presence of address(0) entries. For example, the inherit function that checks if array length is greater than 1.

4. Gas waste: The contract will use more gas than needed, because it will iterate through a longer array than the number of real beneficiaries.

Tools Used

• Manual Code Review

• Foundry for PoC

Recommendations

1. Implement Proper Array Removal: Replace the current incorrect deletion with a correct one. The best way to remove a value in solidity is by:

   • Swap the element to remove with the last one.

   • then use pop() to remove the last one.

   function removeBeneficiary(address _beneficiary) external onlyOwner {

   uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);

   - delete beneficiaries[indexToRemove];

   + // Move the last element into the place to delete

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

   + // Remove the last element

   + beneficiaries.pop();

   }