Inheritable Smart Contract Wallet

InheritanceManager::onlyBeneficiaryWithIsInherited triggers an out-of-bounds error instead of an explicit message, making debugging more difficult

Description: The modifier InheritanceManager::onlyBeneficiaryWithIsInherited follows the natspec documentation, but instead of explicitly reverting when msg.sender is not a beneficiary, it relies on an out-of-bounds array access (panic: out-of-bounds) to cause a revert.

While this achieves the intended behavior, it is not a best practice to rely on implicit Solidity panics for access control. This approach makes debugging more difficult since the error message is generic (panic: out-of-bounds) instead of providing a clear reason, such as "InvalidBeneficiaries()".

Additionally, the current implementation evaluates isInherited inside the loop, meaning that the condition (msg.sender == beneficiaries[i] && isInherited) performs two comparisons on each iteration. This is inefficient because:

If msg.sender != beneficiaries[i], the contract still evaluates isInherited, even though it is unnecessary.
If isInherited == false, the loop still runs through the entire array before failing, wasting gas.
Separating condition checks is generally more optimal, as it avoids unnecessary evaluations when the first condition is false.

Impact: Harder debugging: The error panic: out-of-bounds does not provide clear information on why the transaction reverted.
Lack of transparency: No explicit error message appears in the logs, making contract auditing more challenging.
Unnecessary gas consumption: The isInherited check is performed multiple times inside the loop instead of once before iterating.
Inefficient condition evaluation: A combined condition (msg.sender == beneficiaries[i] && isInherited) forces Solidity to evaluate both conditions in every iteration, even when unnecessary.

Tools Used

• Manual review

• Foundry for testing

Recommended Mitigation: We recommend modifying the implementation for better clarity or replacing the modifier with our proposed solution, which improves gas efficiency.
In our proposal, the isInherited check is moved outside the loop so that it is evaluated only once rather than in every iteration. Additionally, separating conditions ensures that if msg.sender != beneficiaries[i], the contract does not perform the redundant isInherited check.