Beginner FriendlySolidity
100 EXP
View results
Submission Details
Severity: medium
Invalid

Off-By-One Error in `InheritanceManager::onlyBeneficiaryWithIsInherited` Causing Potential Denial of Service

Summary

The onlyBeneficiaryWithIsInherited modifier in the Solidity contract contains a potential out-of-bounds array access issue due to an off-by-one error in the loop condition. This can cause transactions to revert unexpectedly, leading to denial of service (DoS) vulnerabilities and preventing legitimate beneficiaries from executing functions that depend on this modifier.

Vulnerability Details

The modifier iterates over the beneficiaries array to check if msg.sender is a valid beneficiary and if isInherited is true. However, the loop condition is written as:

while (i < beneficiaries.length + 1) {

Since i iterates from 0 to beneficiaries.length + 1, there is a scenario where i reaches beneficiaries.length, leading to an attempt to access beneficiaries[beneficiaries.length], which is out of bounds. In Solidity, this results in a runtime exception, reverting the transaction.

Impact

  1. Denial of Service (DoS): If an out-of-bounds error occurs, transactions calling functions that rely on this modifier will always revert, blocking access to critical contract functionality.

  2. Invalid State Handling: The contract may fail to execute legitimate operations, preventing rightful beneficiaries from claiming their inheritance.

  3. Unexpected Contract Behavior: The off-by-one logic flaw introduces inconsistencies in execution, which could lead to unpredictable behavior if not properly handled.

Tools Used

  • Manual Code Review

Recommendations

  1. Fix the Loop Condition: Change the loop condition to iterate correctly within valid bounds:

    while (i < beneficiaries.length) {
  2. Implement Boundary Checks: Before accessing an array index, ensure that i is within the allowed range.

    require(i < beneficiaries.length, "Index out of bounds");
  3. Add Unit Tests: Create test cases to validate boundary conditions, including scenarios with an empty beneficiaries array.

  4. Consider Using for Loop: A for loop with an explicit boundary check may improve readability and safety.

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

By implementing these fixes, the contract will avoid unexpected failures and ensure legitimate beneficiaries can access their inheritance without disruption.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.