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

Array Integrity Vulnerability in `removeBeneficiary` Function

tiannah

Summary

The removeBeneficiary function in the InheritanceManager contract contains a critical vulnerability where removing a beneficiary leaves a "hole" (zero address) in the beneficiaries array. This impacts the inheritance distribution logic, potentially causing funds to be sent to invalid addresses or resulting in incorrect distribution amounts to remaining beneficiaries.

Vulnerability Details

The removeBeneficiary function uses the Solidity delete keyword to remove an element from the beneficiaries array. However, this operation doesn't actually remove the element from the array - it only resets the value at that index to the default value (address(0)). The array length remains unchanged, and the array structure is compromised with a zero address in place of the removed beneficiary.

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

This creates several issues:

  1. The contract calculates divisions based on beneficiaries.length, which now includes invalid zero addresses

  2. The withdrawInheritedFunds function attempts to send funds to all addresses in the array, including the zero address

  3. The contract's inheritance logic is based on the number of beneficiaries, but this count is incorrect after removals

Impact

This vulnerability has significant impacts:

  1. Locked Funds: When withdrawInheritedFunds attempts to distribute assets to address(0), the transaction may revert, potentially locking all funds in the contract.

  2. Incorrect Distribution: If transfers to address(0) don't revert, remaining beneficiaries will receive less than their fair share because the divisor used includes removed beneficiaries.

  3. Inheritance Logic Errors: The inherit function behaves differently based on the number of beneficiaries; including removed addresses leads to incorrect control flow.

  4. Gas Wastage: Iterating through arrays with deleted elements wastes gas and could potentially cause functions to hit the gas limit.

The impact is especially severe because this contract manages inheritance assets, where correctness of fund distribution is critical.

Proof of Code

To properly test this, you would need to add a simple view function to the InheritanceManager contract that allows you to check the array contents. Without seeing inside the array, it's hard to prove the bug exists definitively.

The minimum function you need to add to InheritanceManager is:

function getBeneficiaryAt(uint256 index) external view returns (address) {
return beneficiaries[index];
}

Paste these lines of code in the test folder: InheritanceManagerTest.t.sol

function test_removeBeneficiaryVulnerability() public {
// Setup
vm.startPrank(owner);
// Add three beneficiaries
address ben1 = makeAddr("ben1");
address ben2 = makeAddr("ben2");
address ben3 = makeAddr("ben3");
im.addBeneficiery(ben1);
im.addBeneficiery(ben2);
im.addBeneficiery(ben3);
// Log beneficiaries before removal
address beforeRemoval0 = im.getBeneficiaryAt(0);
address beforeRemoval1 = im.getBeneficiaryAt(1);
address beforeRemoval2 = im.getBeneficiaryAt(2);
console.log("Beneficiaries BEFORE removal:");
console.log("Index 0:", beforeRemoval0);
console.log("Index 1:", beforeRemoval1);
console.log("Index 2:", beforeRemoval2);
// Remove the middle beneficiary
im.removeBeneficiary(ben2);
// Log beneficiaries after removal
address afterRemoval0 = im.getBeneficiaryAt(0);
address afterRemoval1 = im.getBeneficiaryAt(1);
address afterRemoval2 = im.getBeneficiaryAt(2);
console.log("Beneficiaries AFTER removal:");
console.log("Index 0:", afterRemoval0);
console.log("Index 1:", afterRemoval1);
console.log("Index 2:", afterRemoval2);
// Verify that after removal, index 1 contains address(0)
assertEq(afterRemoval1, address(0), "Second position should be zero address after removal");
// Verify positions 0 and 2 are unchanged
assertEq(afterRemoval0, ben1, "First beneficiary should still be ben1");
assertEq(afterRemoval2, ben3, "Third beneficiary should still be ben3");
vm.stopPrank();
console.log("VULNERABILITY CONFIRMED: 'removeBeneficiary' leaves a hole (zero address) in the array");
console.log("This will cause funds to be incorrectly distributed during inheritance");
}

Tools Used

  • Manual code review

  • Foundry for testing and verification

Recommendations

Replace the vulnerable implementation with a secure array management pattern that maintains array integrity. Here's a corrected version:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
// Move the last element to the position of the removed element
if (indexToRemove < beneficiaries.length - 1) {
beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
}
// Remove the last element
beneficiaries.pop();
// Reset the deadline
_setDeadline();
}

This approach:

  1. Replaces the removed element with the last element in the array

  2. Reduces the array length by removing the last element

  3. Preserves array integrity with no gaps or zero addresses

  4. Maintains O(1) complexity for removal operations

Additionally, consider adding events to track beneficiary changes and implementing additional validations to ensure the integrity of the beneficiaries array.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 5 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.