Summary
The removeBeneficiary function does not properly update the beneficiaries array after removing an address. Instead, it leaves a address(0) in the array. Consequently, when calling withdrawInheritedFunds, a portion of the assets intended for valid beneficiaries may be sent to address(0), resulting in an irreversible loss of funds.
Vulnerability Details
The current implementation of removeBeneficiary only deletes the entry in the array without adjusting the array size:
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}
withdrawInheritedFunds() will disperse the assets to all the addresses in beneficiaries
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);
}
}
Impact
User lost assets
PoC
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 NonReentrantTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
vm.stopPrank();
}
function test_removeBeneficiaryBug() public {
address user2 = makeAddr("user2");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
vm.stopPrank();
assertEq(0, im._getBeneficiaryIndex(user1));
assertEq(1, im._getBeneficiaryIndex(user2));
vm.startPrank(owner);
im.removeBeneficiary(user2);
vm.stopPrank();
assert(1 != im._getBeneficiaryIndex(user2));
vm.warp(1 + 90 days);
im.inherit();
assertEq(im.getIsInherited(), true);
assertEq(user1.balance, 0);
assertEq(address(0).balance, 0);
vm.deal(address(im), 1 ether);
vm.startPrank(user1);
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
assertEq(user1.balance, 0.5 ether);
assertEq(address(0).balance, 0.5 ether);
}
}
Tools Used
Recommendations
update array size after deletion.
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length-1];
beneficiaries.pop();
}
