Summary
Function removeBeneficiary is not removing addreses from beneficiaries[] array - it's only replacing it with default value(address(0)). This couses several issues in functions that distribute funds.
Vulnerability Details
Function removeBeneficiary() for removing addresses by "delete" keyword, which only replace addresses in array with address(0), and it doesn't reduce length of beneficiaries[] array.
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}
Functions withdrawInheritedFunds() and buyOutEstateNFT() use beneficiaries.length as divisor for deviding the funds. When funds are distributed, they're divided by the total number of beneficiaries (including deleted ones), resulting in each legitimate beneficiary receiving less than their fair share.
Also funds sent to address(0) will be essencially burned without change to recover them.
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 buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return;
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}
It also unnecessarily consume more gas that is needed when going through empty addresses in while loop from onlyBeneficiaryWithIsInherited modifier.
modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}
Test case for foundry that shows bad calculations when calling withdrawInheritedFunds(address(0)) after one of the beneficiaries was removed:
function test_badWithdrawInheritedFunds() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address user4 = makeAddr("user4");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.addBeneficiery(user4);
console.log("address on index 3(user4)", im.getAddresOnIndexBeneficiary(3));
im.removeBeneficiary(user4);
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 12e10);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
assertEq(user3.balance, 4e10, "user 3 should get 4e10 ");
}
│ └─ ← [Stop]
│ ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 30000000000}()
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::assertEq(30000000000 [3e10], 40000000000 [4e10], "user 3 should get 4e10 ") [staticcall]
│ └─ ← [Revert] user 3 should get 4e10 : 30000000000 != 40000000000
└─ ← [Revert] user 3 should get 4e10 : 30000000000 != 40000000000
Impact:
\
Funds sent to address(0) are effectively burned and cannot be recovered
When funds are distributed, they're divided by the total number of beneficiaries (including deleted ones), resulting in each legitimate beneficiary receiving less than their fair share
Tools Used
Foundry, for better visualization i have imported Console to test file:
import {Test, console} from "forge-std/Test.sol";
and I created getter function:
function getAddresOnIndexBeneficiary(
uint256 _index
) public returns (address) {
return beneficiaries[_index];
}
Recommendations
Implement removing by Swap and Pop pattern
