Inheritable Smart Contract Wallet

Summary

The onlyBeneficiaryWithIsInherited modifier in the InheritanceManager contract contains a critical vulnerability that can lead to unauthorized access to restricted functions and potentially compromise the security of the inheritance system.

Vulnerability Details

The onlyBeneficiaryWithIsInherited modifier is implemented as follows:

This modifier has several critical issues:

1. Array Out-of-Bounds Access: The while loop condition i < beneficiaries.length + 1 allows i to reach beneficiaries.length, which is outside the valid array bounds. This is not appropriate for array access validation.

2. Intended Behavior vs. Implementation: According to the comment, the loop is supposed to "revert on array out of bounds if not called by a beneficiary." However:

   • If the caller is a valid beneficiary, the loop will break and execution continues

   • If the caller is not a valid beneficiary, the loop will increment i until it equals beneficiaries.length, then attempt to access beneficiaries[i] which will revert due to out-of-bounds access

3. Holes in Array Not Handled: The removeBeneficiary function uses delete beneficiaries[indexToRemove] which sets that element to address(0) but doesn't remove it from the array. If address(0) is accessed in the modifier, it could lead to false validations or failures.

4. Boolean Logic Error: The condition msg.sender == beneficiaries[i] && isInherited requires both the sender to be a beneficiary and isInherited to be true. If isInherited is false, the modifier will always revert even for legitimate beneficiaries.

Impact

The impact of this vulnerability is high:

1. Beneficiaries may be unable to access their funds after inheritance if:

   • The array contains "holes" (address(0) entries) from removeBeneficiary

   • The implementation logic changes (no out-of-bounds error)

2. Gas wastage on array out-of-bounds revert, which is an expensive way to implement access control.

3. The modifier is used in critical functions like buyOutEstateNFT and appointTrustee, affecting core inheritance functionality.

Tools Used

Manual code review.

Recommendations

Replace the current implementation with a more explicit and robust approach:

Additional improvements:

1. Create a mapping to track beneficiaries for more efficient validation:

1. Update the addBeneficiery and removeBeneficiary functions to maintain this mapping.

2. Add proper events for beneficiary management operations.

3. Consider implementing the same access control for the withdrawInheritedFunds function, which currently allows any address to trigger fund distribution once inheritance is activated.