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

Improper Array Handling in removeBeneficiary

dsbex

Summary - The InheritanceManager.sol::removeBeneficiary function incorrectly uses delete to remove elements from the beneficiaries array, leaving stale address(0) entries and breaking critical logic that relies on the integrity of the array. This leads to incorrect fund distribution, failed transactions, and potential loss of funds.

Vulnerability Details - The current implementation of removeBeneficiary uses delete beneficiaries[indexToRemove], which nullifies the entry at the specified index but does not reduce the array length. This results in:

  1. Stale Entries: The array retains address(0) after deletion.

  2. Incorrect Indexing: Functions like _getBeneficiaryIndex return invalid indices for remaining entries.

  3. Broken Assumptions: Functions iterating over beneficiaries (e.g., withdrawInheritedFunds) process null addresses, leading to failed transactions or ETH sent to address(0).

function test_removeBeneficiaryDestroysArray() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
vm.stopPrank();
// Verify initial state
assertEq(im._getBeneficiaryIndex(user1), 0);
assertEq(im._getBeneficiaryIndex(user2), 1);
assertEq(im._getBeneficiaryIndex(user3), 2);
// Remove second beneficiary
vm.prank(owner);
im.removeBeneficiary(user2);
// Now user3 should be at index 1
assertEq(im._getBeneficiaryIndex(user3), 1);
console.log("user3 index: ", im._getBeneficiaryIndex(user3));
}

Output:

Failing tests:
Encountered 1 failing test in test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[FAIL: assertion failed: 2 != 1] test_removeBeneficiaryDestroysArray() (gas: 144571)

Which means after we deleted user2 who was at index1, user3 didn't go to index1, because when we use delete in array, we don't remove the element it is just set to address(0).

Impact

  • Fund Loss: withdrawInheritedFunds may send assets to address(0).

  • Incorrect Calculations: buyOutEstateNFT divides by the original array length, underpaying beneficiaries.

  • Failed Transactions: Functions expecting valid addresses will revert when encountering address(0).

Tools Used

  • Manual Review

Recommendations

Use pop with Swap: Properly remove elements while maintaining array integrity:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 index = _getBeneficiaryIndex(_beneficiary);
// Swap with last element
beneficiaries[index] = beneficiaries[beneficiaries.length - 1];
beneficiaries.pop(); // Reduce array length
}
Updates

Lead Judging Commences

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