Inheritable Smart Contract Wallet

Details:
In the onlyBeneficiaryWithIsInherited modifier, the while-loop condition is set as while (i < beneficiaries.length + 1). This condition causes the loop to iterate one time too many. When i equals beneficiaries.length, the code attempts to access beneficiaries[i], which is outside the bounds of the array. This off-by-one error may cause an unintended revert when the caller is not found in the beneficiaries list.

Root Cause:
The root cause is the incorrect loop termination condition. By using beneficiaries.length + 1 instead of the correct beneficiaries.length, the code unintentionally allows the loop to access an array element that does not exist.

Impact:

• Unexpected Reverts: Legitimate function calls from valid beneficiaries could inadvertently revert if the loop iterates beyond the valid range.

• Denial of Service (DoS): An attacker or even an improperly configured call could trigger the off-by-one error, causing a denial of service for intended users.

• Faulty Access Control: The modifier may fail to correctly enforce beneficiary checks, leading to unpredictable behavior in contract execution.

Recommendation:

• Fix the Loop Condition: Change the while-loop condition from i < beneficiaries.length + 1 to i < beneficiaries.length to ensure that the loop only iterates over valid array indices.

• Use a For-Loop: Alternatively, refactor the modifier to use a for-loop, which inherently limits the iteration to valid indices:

  modifier onlyBeneficiaryWithIsInherited() {

  bool isValid = false;

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

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

  isValid = true;

  break;

  }

  }

  require(isValid, "Caller is not a beneficiary or inheritance not active");

  _;

  }

Proof of Concept:

1. Deploy the contract with a non-empty beneficiaries array.

2. Call a function protected by the onlyBeneficiaryWithIsInherited modifier using an address that is not in the beneficiaries list.

3. When the loop reaches i == beneficiaries.length, the contract attempts to access beneficiaries[i], causing an out-of-bounds error and the transaction to revert unexpectedly.

This PoC demonstrates that the loop condition indeed leads to an off-by-one error, confirming the vulnerability.