Summary
If the owner removes a beneficiary from the InheritanceManager::beneficiaries[] array by calling InheritanceManager::removeBeneficiary(), a susequent call to InheritanceManager::inherit() and InheritanceManager::withdrawInheritedFunds() after the 90 day lock, results in a share of the funds being burned.
Vulnerability Details
This is due to the incorrect use of the delete keyword in the following function (lines 163-166):
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
@> delete beneficiaries[indexToRemove];
}
Taken from the Solidity docs:
delete a assigns the initial value for the type to a. I.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements set to their initial value. delete a[x] deletes the item at index x of the array and leaves all other elements and the length of the array untouched.
As the array is not resized, the logic to determine the remaining beneficiaries share misallocates the funds when InheritanceManager::withdrawInheritedFunds() is called (lines 236-256):
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);
}
}
}
Critically uint256 divisor = beneficiaries.length; remains the same regardless of the number of beneficiaries removed.
Impact
Not only does this allocate the incorrect share to the beneficiaries but also, the address held at the index of the beneficiary removed via delete is set to 0. This results in a share of the funds being burned. The amount of funds lost scales with the number of beneficiaries removed.
Proof Of Concept
function test_beneficiaryLostFunds() public {
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.removeBeneficiary(user1);
vm.stopPrank();
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
uint256 initialBalance = address(im).balance;
console.log("initial contract balance:\t%s", initialBalance);
vm.startPrank(user2);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
console.log("new contract balance:\t\t%s", address(im).balance);
console.log("user1 balance:\t\t%s", address(user1).balance);
console.log("user2 balance:\t\t%s", address(user2).balance);
console.log("user3 balance:\t\t%s", address(user3).balance);
}
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_beneficiaryLostFunds() (gas: 259543)
Logs:
initial contract balance: 100000000000
new contract balance: 1
user1 balance: 0
user2 balance: 33333333333
user3 balance: 33333333333
Tools Used
Foundry Forge:
forge test --mt test_beneficiaryLostFunds -vv
Recommendations
Change the InheritanceManager::removeBeneficiary() so that it replaces the element of the array and resizes:
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
for(uint256 _index = indexToRemove; _index < beneficiaries.length - 1; _index++){
beneficiaries[_index] = beneficiaries[_index + 1];
}
beneficiaries.pop();
}
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_beneficiaryLostFunds() (gas: 229473)
Logs:
initial contract balance: 100000000000
new contract balance: 0
user1 balance: 0
user2 balance: 50000000000
user3 balance: 50000000000
