Summary

The InheritanceManager smart contract includes three notable areas of concern: the contractInteractions function, which allows risky external calls, and two edge cases in removeBeneficiary and the onlyBeneficiaryWithIsInherited modifier. The external call functionality is flexible but dangerous due to its lack of input validation, potentially allowing the owner to lose funds to malicious contracts. The removeBeneficiary function creates gaps in the beneficiaries array, which can lead to funds being sent to the zero address or miscalculated distributions. The onlyBeneficiaryWithIsInherited modifier has an out-of-bounds loop issue that could cause unnecessary reverts. These issues could disrupt the contract’s core purpose of securely managing inheritance, but they can be audited and mitigated with careful testing and code adjustments.

Vulnerability Details

1. External Calls in contractInteractions

• Code:

  solidity

  function contractInteractions(address _target, bytes calldata _payload, uint256 _value, bool _storeTarget)

  external

  nonReentrant

  onlyOwner

  {

  (bool success, bytes memory data) = _target.call{value: _value}(_payload);

  require(success, "interaction failed");

  if (_storeTarget) {

  interactions[_target] = data;

  }

  }

• Description: This function lets the owner call any external contract with arbitrary data (_payload) and ETH (_value). It’s protected by nonReentrant (using transient storage) and onlyOwner, but there’s no validation of _target or _payload.

• Issue:

  • Arbitrary call{} usage means the owner could accidentally or intentionally interact with a malicious contract.

  • The documentation warns users to validate inputs, but the contract itself doesn’t enforce safety.

  • The ETH balance isn’t checked before sending _value, risking a revert if insufficient.

2. Array Gaps in removeBeneficiary

• Code:

  solidity

  function removeBeneficiary(address _beneficiary) external onlyOwner {

  uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);

  delete beneficiaries[indexToRemove];

  }

• Description: Removes a beneficiary by setting their array slot to 0x0 using delete, but doesn’t resize the beneficiaries array.

• Issue:

  • Leaves gaps (e.g., [addr1, addr2, addr3] becomes [addr1, 0x0, addr3] after removing addr2).

  • Loops in other functions (e.g., withdrawInheritedFunds) treat 0x0 as a valid recipient, potentially sending funds to it.

3. Out-of-Bounds Loop in onlyBeneficiaryWithIsInherited

• Code:

  solidity

  modifier onlyBeneficiaryWithIsInherited() {

  uint256 i = 0;

  while (i < beneficiaries.length + 1) {

  if (msg.sender == beneficiaries[i] && isInherited) {

  break;

  }

  i++;

  }

  _;

  }

• Description: Restricts functions to beneficiaries after inheritance, looping through beneficiaries to verify msg.sender.

• Issue:

  • The condition i < beneficiaries.length + 1 goes one step beyond the array’s valid indices (e.g., for length = 2, it checks index 2, which doesn’t exist).

  • Causes an out-of-bounds revert for non-beneficiaries instead of a clean failure.

Impact

External Calls (contractInteractions)

• Worst Case: A malicious contract drains all ETH or tokens from InheritanceManager via a crafted callback or self-destruct exploit, despite the reentrancy guard.

• Practical Impact: Beginners might misuse this function (e.g., wrong _payload), locking funds in another contract or triggering a revert, undermining the inheritance system’s reliability.

Array Gaps (removeBeneficiary)

• Worst Case: Funds (ETH or ERC20) are sent to 0x0 and lost forever during withdrawInheritedFunds.

• Practical Impact: Unequal distribution—e.g., 9 ETH split among [addr1, 0x0, addr3] gives 3 ETH each, losing 3 ETH to 0x0, instead of 4.5 ETH to addr1 and addr3. This breaks the “equally divided” assumption.

Modifier Loop (onlyBeneficiaryWithIsInherited)

• Worst Case: No severe fund loss, but the contract becomes unusable for valid beneficiaries if the array access bug triggers unexpected reverts.

• Practical Impact: Non-beneficiaries correctly can’t proceed, but the out-of-bounds error is sloppy and could confuse users or tools expecting a clear revert message.

Tools Used

Grok AI
Foundry

Recommendations
For contractInteractions

• Add Validation:

  • Implement a whitelist of trusted _target addresses:

    solidity

    mapping(address => bool) public allowedTargets;

    function setAllowedTarget(address _target, bool _allowed) external onlyOwner {

    allowedTargets[_target] = _allowed;

    }

    modifier onlyAllowedTarget(address _target) {

    require(allowedTargets[_target], "Target not allowed");

    _;

    }

    // Update function

    function contractInteractions(address _target, bytes calldata _payload, uint256 _value, bool _storeTarget)

    external

    nonReentrant

    onlyOwner

    onlyAllowedTarget(_target)

    { ... }

• Balance Check: Add require(address(this).balance >= _value, "Insufficient ETH"); before the call.

• Educate Users: Provide examples of safe _payload construction (e.g., for Aave deposits).

For removeBeneficiary

• Resize Array: Use swap-and-pop to eliminate gaps:

  solidity

  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();

  }

• Test: Verify withdrawInheritedFunds splits funds only among valid addresses after removal.

For onlyBeneficiaryWithIsInherited

• Fix Loop: Rewrite for safety and clarity:

  solidity

  modifier onlyBeneficiaryWithIsInherited() {

  require(isInherited, "Not yet inherited");

  bool isBeneficiary = false;

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

  if (msg.sender == beneficiaries[i]) {

  isBeneficiary = true;

  break;

  }

  }

  require(isBeneficiary, "Not a beneficiary");

  _;

  }

• Benefit: Avoids out-of-bounds access, provides clear error messages, and is easier to debug.