Inheritable Smart Contract Wallet

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

Summary

The _getBeneficiaryIndex function, designed to find the index of a beneficiary within the beneficiaries array, has a critical flaw: if the provided beneficiary address is not found in the array, the function will silently return 0. This can lead to incorrect behavior in the removeBeneficiary function, where it's used, potentially causing the wrong beneficiary to be removed or even modifying the state in an unexpected way.

Vulnerability Details

1. _getBeneficiaryIndex Function:

   • Purpose: The function iterates through the beneficiaries array to find the index of a given _beneficiary address.

   • Logic: It loops through the array, and if a match is found, it sets the _index return variable to the current index (i) and breaks the loop.

   • Flaw: If no match is found, the loop completes without ever setting _index. Because _index is initialized as 0, the function will return 0 in this scenario.

   • Code:

     function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) { // @audit-issue if no beneficiary is found, it will return 0 and remove 0

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

     if (_beneficiary == beneficiaries[i]) {

     _index = i;

     break;

     }

     }

     // If no match is found, _index remains 0

     }

2. removeBeneficiary Dependency:

   • Usage: The removeBeneficiary function directly uses the result of _getBeneficiaryIndex to determine which element to remove.

   • Issue: If _getBeneficiaryIndex incorrectly returns 0 when the beneficiary isn't found, removeBeneficiary will always try to remove the element at index 0 in the beneficiaries array, even if that's not the intended target.

   • Code:

     function removeBeneficiary(address _beneficiary) external onlyOwner {

     uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary); // Incorrect return value if _beneficiary not found

     // ... now the code remove indexToRemove

     }

Impact

1. Incorrect Removal: If an owner attempts to remove a beneficiary who is not actually in the beneficiaries array, the function will silently remove the beneficiary at index 0 instead.

2. Accidental Removal of First Beneficiary: This can lead to the unintended removal of the first added beneficiary, as index 0 will be wrongly targeted.

Tools Used

• Manual Code Review

Recommendations

1. Return a Boolean and Update the Index: Modify _getBeneficiaryIndex to return a bool indicating success or failure and update _index as an output. This makes the function much more informative.

   - function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) { // @audit-issue if no beneficiary is found, it will return 0 and remove 0

   + function _getBeneficiaryIndex(address _beneficiary) public view returns (bool found, uint256 _index) { // @audit-issue if no beneficiary is found, it will return 0 and remove 0

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

   if (_beneficiary == beneficiaries[i]) {

   _index = i;

   + found = true;

   break;

   }

   }

   + return (found, _index);

   }

2. Update removeBeneficiary: Modify removeBeneficiary to check the new found return value from _getBeneficiaryIndex and handle the case where the beneficiary is not found.

   function removeBeneficiary(address _beneficiary) external onlyOwner {

   - uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);

   + (bool found, uint256 indexToRemove) = _getBeneficiaryIndex(_beneficiary);

   + if (!found) return; // revert or do nothing if no beneficiary found

   + if (indexToRemove >= beneficiaries.length) return; // just in case

   // ...

   }