Inheritable Smart Contract Wallet

Summary

The onlyBeneficiaryWithIsInherited modifier fails to properly enforce the "only inherited-state beneficiaries" restriction, allowing unauthorized access to critical functions even after fixing the array out-of-bounds issue.

Vulnerability Details

1. Missing Explicit Validation:

   • The modifier uses a loop to check if msg.sender is a beneficiary but does not explicitly validate the result after the loop. Even if msg.sender is not a beneficiary, execution continues, permitting unauthorized calls.

   • Example: A non-beneficiary address bypasses the check because the loop completes without reverting.

2. Ignored isInherited State:

   • The modifier does not verify the isInherited flag outside the loop. Even if isInherited is false, calls from beneficiaries are allowed.

3. Unhandled Empty Beneficiaries:

   • If beneficiaries.length == 0, the loop is skipped entirely, granting unrestricted access to any caller.

Impact

• Unauthorized users can invoke protected functions (e.g., buyOutEstateNFT, appointTrustee), leading to:

  • Theft of NFT-backed assets.

  • Malicious trustee appointments.

  • Disruption of inheritance processes.

Tools Used

• Manual code review

• Slither static analysis (to detect missing access controls)

Recommendations

1. Add Explicit Checks:

   modifier onlyBeneficiaryWithIsInherited() {

   require(beneficiaries.length > 0, "No beneficiaries");

   bool isBeneficiary = false;

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

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

   isBeneficiary = true;

   break;

   }

   }

   require(isBeneficiary && isInherited, "Not beneficiary or inactive inheritance");

   _;

   }

2. Optimize with Mappings:
   Replace the beneficiaries array with a mapping for O(1) lookups:

   mapping(address => bool) public isBeneficiary;

   modifier onlyBeneficiaryWithIsInherited() {

   require(isInherited, "Inheritance inactive");

   require(isBeneficiary[msg.sender], "Not beneficiary");

   _;

   }

3. State Consistency:
   Ensure isInherited is validated independently of the loop logic.

This merged issue captures the critical flaws in the modifier’s access control mechanism, prioritizing fixes to prevent unauthorized access and inheritance system compromise.