Summary

A logical flaw in the removeBeneficiary function allows unintended removal of the first beneficiary if an invalid address (one not in the beneficiaries array) is passed as an argument.

Vulnerability Details

The function _getBeneficiaryIndex iterates over the beneficiaries array to find the index of the given address. If the address is not found, the function does not explicitly return an invalid index, causing it to default to 0. Consequently, when removeBeneficiary is called with a non-existent beneficiary, the first beneficiary (index 0) is mistakenly deleted.

Affected Code:

Impact

• If an invalid address is passed, the first beneficiary is erroneously removed.

• This can lead to unintended removal of valid beneficiaries, potentially disrupting contract functionality.

• Could allow malicious actors to exploit this flaw by tricking the owner into calling the function with an invalid address.

Tools Used

• Foundry framework for testing

• Solidity static analysis

• Manual code review

  Proof of Concept (PoC)

  The following Foundry test demonstrates the vulnerability:

  function test_ifBeneficairyNotInArrayAndDeleteBeneficiaryIsCalled() public {

  vm.prank(owner);

  im.addBeneficiery(user1);

  vm.prank(owner);

  address users = makeAddr("user2");

  im.addBeneficiery(users);

  ​

  vm.prank(owner);

  address userNotInArray = makeAddr("user2A");

  im.removeBeneficiary(userNotInArray);

  address[] memory beneficiaries = im.getBeneficiaries();

  ​

  assertEq(user1, beneficiaries[im._getBeneficiaryIndex(user1)]);

  }

• For testing purposes, the following getter function was added in InheritanceManager.sol to retrieve the list of beneficiaries:

  function getBeneficiaries() public view returns (address[] memory) {

  return beneficiaries;

  }

Recommendations

• Modify _getBeneficiaryIndex to return an invalid index if the beneficiary is not found:

  function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {

  _index = type(uint256).max; // Use a large invalid index as default

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

  if (_beneficiary == beneficiaries[i]) {

  _index = i;

  break;

  }

  }

  }

• Before deleting a beneficiary, validate the returned index:

  function removeBeneficiary(address _beneficiary) external onlyOwner {

  uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);

  require(indexToRemove != type(uint256).max, "Beneficiary not found");

  delete beneficiaries[indexToRemove];

  }

• Implement proper error handling to prevent silent failures.

• Add unit tests to cover edge cases, ensuring robustness against such logical flaws.