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

Array Gap Vulnerability in InheritanceManager.sol

nomadic_bear

Summary

The removeBeneficiary() function in the InheritanceManager contract creates gaps in the beneficiaries array rather than maintaining a compact array structure. This leads to incorrect fund distribution during inheritance and potential loss of funds sent to the zero address.

Vulnerability Details

The vulnerability exists in the removeBeneficiary() function:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}

This function uses Solidity's delete keyword, which sets the array element to its default value (address(0)) but doesn't resize the array or shift elements. The array remains the same length with a "gap" at the deleted index.

This causes two critical issues:

  1. During fund distribution in withdrawInheritedFunds(), the contract uses beneficiaries.length as the divisor, including the zero addresses from deleted elements:

    uint256 divisor = beneficiaries.length;
    uint256 amountPerBeneficiary = ethAmountAvailable / divisor;

  2. The contract then attempts to send funds to all addresses in the array, including address(0):

    for (uint256 i = 0; i < divisor; i++) {
    address payable beneficiary = payable(beneficiaries[i]);
    (bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
    require(success, "something went wrong");
    }

PoC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import {Test, console} from "forge-std/Test.sol";
import {InheritanceManager} from "../src/InheritanceManager.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
​
contract BeneficiaryGapPoC is Test {
InheritanceManager im;
ERC20Mock usdc;
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address attacker = makeAddr("attacker");
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
// Set up beneficiaries
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
vm.stopPrank();
// Fund the inheritance manager
usdc.mint(address(im), 9e18);
vm.deal(address(im), 9e18);
}
function test_beneficiaryGapExploit() public {
// Step 1: Owner removes the middle beneficiary, creating a gap
vm.prank(owner);
im.removeBeneficiary(user2);
// Fast forward past the timelock period
vm.warp(block.timestamp + 91 days);
// Step 2: Trigger inheritance from any address
vm.prank(attacker);
im.inherit();
// Record balances before
uint256 user1BalanceBefore = user1.balance;
uint256 user3BalanceBefore = user3.balance;
// Step 3: Have a beneficiary withdraw the ETH funds
vm.prank(user1);
im.withdrawInheritedFunds(address(0)); // ETH only
// Step 4: Verify the incorrect distribution
// Since there are 3 slots in the array but only 2 valid beneficiaries,
// funds get distributed incorrectly
console.log("User1 ETH balance:", user1.balance);
console.log("User2 ETH balance:", user2.balance);
console.log("User3 ETH balance:", user3.balance);
// Expected distribution if array was properly compacted: 4.5e18 each (9e18 / 2)
// Actual distribution with array gap: 3e18 each (9e18 / 3)
assertEq(user1.balance - user1BalanceBefore, 3e18, "User1 should receive 3e18");
assertEq(user2.balance, 0, "User2 should receive nothing as it was removed");
assertEq(user3.balance - user3BalanceBefore, 3e18, "User3 should receive 3e18");
// IMPORTANT: The remaining 3e18 is lost due to the gap in the array!
// 3e18 of ETH is sent to the zero address or lost in the contract
assertEq(address(im).balance, 0, "Contract should have 0 balance");
// Demonstrate that funds were not distributed correctly
// If properly distributed, each valid beneficiary should get 4.5e18
assertFalse(user1.balance - user1BalanceBefore == 4.5e18, "User1 should have received more ETH");
assertFalse(user3.balance - user3BalanceBefore == 4.5e18, "User3 should have received more ETH");
}
function test_beneficiaryModifierExploit() public {
// Step 1: Owner removes the last beneficiary, creating a gap
vm.prank(owner);
im.removeBeneficiary(user3);
// Fast forward past the timelock period
vm.warp(block.timestamp + 91 days);
// Step 2: Trigger inheritance
vm.prank(user1);
im.inherit();
// Step 3: The attacker should not be able to call functions with onlyBeneficiaryWithIsInherited
// but the modifier is vulnerable to out-of-bounds array access
vm.startPrank(attacker);
// This should normally revert, but may not because of the array bound issue
// in the onlyBeneficiaryWithIsInherited modifier
try im.appointTrustee(attacker) {
console.log("VULNERABILITY: Attacker bypassed beneficiary check!");
assertTrue(im.getTrustee() == attacker, "Attacker became trustee");
} catch {
console.log("Attacker failed to bypass beneficiary check");
}
vm.stopPrank();
}
}

PoC Result:

forge test --match-test test_beneficiaryGapExploit -vvv
[⠰] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/BeneficiaryArrayGapVulnerabilityPoC.t.sol:BeneficiaryGapPoC
[PASS] test_beneficiaryGapExploit() (gas: 177712)
Logs:
User1 ETH balance: 3000000000000000000
User2 ETH balance: 0
User3 ETH balance: 3000000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.44ms (989.43µs CPU time)
​
Ran 1 test suite in 37.72ms (6.44ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

  1. Direct Loss of Funds: When beneficiaries are removed, a portion of inheritance funds equal to totalAssets / beneficiaries.length will be sent to address(0) for each removed beneficiary, permanently locking those assets.

  2. Incorrect Asset Distribution: Legitimate beneficiaries receive less than their fair share. For example, if 1 of 3 beneficiaries is removed, each remaining beneficiary receives only 33% of assets instead of the correct 50%.

  3. Gas Waste: Transfer attempts to address(0) waste gas, and for ERC20 tokens will revert the entire transaction due to SafeERC20's receiver validation.

Tools Used

foundry

manual code review

Recommendations

Replace the removeBeneficiary() function with one that maintains a compact array:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
uint256 lastIndex = beneficiaries.length - 1;
// Move the last element to the position being deleted
// (unless we're deleting the last element)
if (indexToRemove != lastIndex) {
beneficiaries[indexToRemove] = beneficiaries[lastIndex];
}
// Remove the last element
beneficiaries.pop();
// Reset the deadline
_setDeadline();
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect removal from beneficiary list causes funds to be send to 0 address

Support

FAQs

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