Summary
the use of delete on the beneficiaries array replaces the removed address with the zero address (0x00), causing incorrect inheritance fund distribution.
Vulnerability Details
The removeBeneficiary function uses delete on an array index, which replaces the removed beneficiary with the zero address instead of properly removing the entry. Since the withdrawInheritedFunds function calculates distributions using the array's length, the zero address gets counted in the divisor, leading to incorrect fund division.
Affected Code:
function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
(bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
require(success, "something went wrong");
}
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = assetAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}
}
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}
Impact
The contract incorrectly divides the total inheritance among all beneficiaries.length entries, including zero addresses.
This results in each actual beneficiary receiving less than their intended share.
If the removeBeneficiary function is misused, a beneficiary may receive an incorrect inheritance while a zero-address entry consumes a portion of the funds.
Tools Used
Foundry framework for testing
Solidity static analysis
Manual code review
function test_removingCorrectBeneficiaryCausesUnequalDistribution() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.removeBeneficiary(user3);
vm.stopPrank();
im.getBeneficiaries();
vm.warp(1);
vm.deal(address(im), 10e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
assertEq(0, user3.balance);
assertEq(5e18, user1.balance);
assertEq(5e18, user2.balance);
}
function test_removeBeneficiaryCausesUnequalDistribution() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address userNotInArray = makeAddr("userNotInArray");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.removeBeneficiary(userNotInArray);
vm.stopPrank();
im.getBeneficiaries();
vm.warp(1);
vm.deal(address(im), 9e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
assertEq(3e18, user1.balance);
assertEq(3e18, user2.balance);
assertEq(3e18, user3.balance);
}
Recommendations
Instead of using delete, implement a proper removal mechanism:
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
require(indexToRemove != type(uint256).max, "Beneficiary not found");
beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
beneficiaries.pop();
}
