Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Off-by-One Error in Beneficiary Validation

teoslaf

Summary

The onlyBeneficiaryWithIsInherited modifier contains an off-by-one error in its array bounds check, allowing access to memory beyond the beneficiaries array and potentially matching random addresses.

Vulnerability Details

modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
while (i < beneficiaries.length + 1) { // CRITICAL: Should be just length
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}

Critical issues:

  1. Array Bounds Error

    • Loop runs one iteration too many (length + 1)

    • Accesses array out of bounds

    • Reads arbitrary memory after array

    • Could match random addresses in memory

  2. Memory Safety

    • Solidity arrays are not null-terminated

    • Reading past array reads arbitrary storage

    • Could match any address stored in memory

    • Unpredictable and unsafe behavior

  3. Affected Functions

function buyOutEstateNFT(uint256 _nftID)
external
onlyBeneficiaryWithIsInherited
{ ... }
​
function appointTrustee(address _trustee)
external
onlyBeneficiaryWithIsInherited
{ ... }

Impact

CRITICAL - The vulnerability enables:

  1. Access Control Bypass

    • Random addresses might get access

    • Array bounds violation

    • Unpredictable authorization

    • Memory safety violation

  2. System Compromise

    • NFT buyout system exposed

    • Trustee system exposed

    • Arbitrary memory access

    • Potential system corruption

Proof of Concept

contract BeneficiaryTest {
InheritanceManager target;
function testAccess() external {
// 1. Array has length N
// 2. Loop goes to N+1
// 3. At index N, reads arbitrary memory
// 4. If that memory matches our address:
target.buyOutEstateNFT(1); // We get access!
}
}

Recommendations

  1. Fix Array Bounds:

modifier onlyBeneficiaryWithIsInherited() {
bool isBeneficiary = false;
for(uint i = 0; i < beneficiaries.length; i++) { // Correct bound
if(msg.sender == beneficiaries[i]) {
isBeneficiary = true;
break;
}
}
require(isBeneficiary && isInherited, "Not authorized");
_;
}

  1. Use Safe Data Structures:

contract InheritanceManager {
mapping(address => bool) public isBeneficiary;
modifier onlyBeneficiaryWithInherited() {
require(isBeneficiary[msg.sender], "Not beneficiary");
require(isInherited, "Not inherited");
_;
}
}

  1. Add Safety Checks:

    • Proper array bounds validation

    • Memory safety guarantees

    • Clear error messages

    • Event logging

Updates

Lead Judging Commences

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

Support

FAQs

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

Give us feedback!