Summary

The InheritanceManager.sol::removeBeneficiary() function incorrectly removes a beneficiary by setting beneficiaries[index] to address(0) instead of properly adjusting the array. This results in an inconsistent beneficiaries.length, leading to permanent DoS in critical functions like withdrawInheritedFunds() and buyOutEstateNFT() when trying to transfer and allocate ERC20 tokens among beneficiaries. Both functions will always revert due to trying transfer tokens to the zero address.

Vulnerability Details

Affected code:

• removeBeneficiary()

• withdrawInheritedFunds()

• buyOutEstateNFT()

The main problem of the current implementation is that the delete keyword in Solidity does not reduce the array’s length - it only resets the specified index to address(0), leaving a “hole” in the array.

For example, if the owner set three beneficiaries and later removed one, although there are only 2 beneficiers the length will still be 3. Then, whenever withdrawInheritedFunds() or buyOutEstateNFT() are called to transfer ERC20 tokens, the functions will always revert because of trying to send tokens to the zero address and thus preventing other beneficiaries to receive their deserved share of the funds.

PoC

Add the following tests to the InheritanceManagerTest.t.sol contract.

The PoC demonstrates the DoS in the withdrawInheritedFunds() when transferring and allocating ERC20 tokens like USDC among beneficiaries.

Impact

• Permanent DoS of the withdrawInheritedFunds() and buyOutEstateNFT() when transferring ERC20 tokens.

It's important to mention that funds are not lost but a part of the core functionality of the protocol is broken that's why I believe that although the likelihood is medium, the impact is high (broken core functionality).

Tools Used

• Manual review

• Foundry

Recommendations

I would recommend is using the pop() method of the dynamic array in Solidity and swapping the last element with the removed one.