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

Incorrect Beneficiary Removal Leads to Permanent Fund Disbursement Failure

howiecht

Summary

The InheritanceManager::removeBeneficiary() function incorrectly removes beneficiaries by simply setting their corresponding array element to its default value (address 0x0). This does not reduce the array's length, leading to an incorrect divisor in withdrawInheritedFunds(). Consequently, withdrawInheritedFunds() fails to distribute funds properly and becomes permanently unusable. While the owner can still manually send funds using sendERC20(), the intended beneficiary withdrawal mechanism is broken.

Vulnerability Details

The removeBeneficiary() function in InheritanceManager is intended to remove a beneficiary from the list. However, it achieves this by using delete beneficiaries[indexToRemove], which only resets the array element at the specified index to its default value (address 0x0). The array's length remains unchanged. This leads to the following issues:

  • Incorrect Divisor in withdrawInheritedFunds(): The withdrawInheritedFunds() function calculates the share of each beneficiary based on the length of the beneficiaries array. Since the length is not reduced after removal, the divisor is incorrect, resulting in incorrect fund distribution calculations.

  • Permanent Failure of withdrawInheritedFunds(): The function iterates through the beneficiaries array to send funds. Due to the presence of the default address (0x0), the loop's logic is disrupted, and the function will always revert, making it unusable.

  • Owner's Manual Fund Distribution: The owner retains the ability to manually send funds using sendERC20(), bypassing the broken withdrawInheritedFunds() function.

The provided Foundry test demonstrates this issue:

function test_removeOneBeneficieryAndWithdrawInheritedFundsERC20() public {
address user2 = makeAddr("user2");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
​
im.removeBeneficiary(user2); // remove user2 from beneficiaries
​
vm.stopPrank();
vm.warp(1);
usdc.mint(address(im), 10e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
vm.expectRevert();
im.withdrawInheritedFunds(address(usdc)); // function will never work
vm.stopPrank();
vm.startPrank(owner);
im.sendERC20(address(usdc), 5e18, user1); // owner still can send
im.sendERC20(address(usdc), 5e18, user1);
vm.stopPrank();
​
assertEq(10e18, usdc.balanceOf(user1));
assertEq(0, usdc.balanceOf(user2));
​
assertEq(0, usdc.balanceOf(address(im)));
}

Impact

The core functionality of the InheritanceManager contract, specifically the automatic distribution of inherited funds via withdrawInheritedFunds(), is permanently broken. This significantly impacts the contract's intended purpose and user experience.

Tools Used

Manual Review and Foundry Test

Recommendations

The removeBeneficiary() function should be redesigned to properly remove beneficiaries by shifting the remaining elements and reducing the array's length. Here is the suggested correction:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
// Shift elements to the left (overwrite the element to be removed)
+ for (uint256 i = indexToRemove; i < beneficiaries.length - 1; i++) {
+ beneficiaries[i] = beneficiaries[i + 1];
+ }
​
// Remove the last element (which is now a duplicate)
+ beneficiaries.pop();
}
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.