Summary
There is a issue in the InheritanceManager::removeBeneficiaryfunction when using deleteto remove beneficiaries from the beneficiariesarray where the array gets populated with address(0). When calling the InheritanceManager::withdrawInheritedFundsfunction the transaction will revert locking funds in contract. This funds will be locked indefinatly if ownercannot access their wallet.
Vulnerability Details
Add this test to test suite:
function test_withdrawInheritedFundsERC20AfterRemovingBeneficiary() public {
address user2 = makeAddr("user2");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.removeBeneficiary(user2);
vm.stopPrank();
usdc.mint(address(im), 9e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
vm.expectRevert();
im.withdrawInheritedFunds(address(usdc));
vm.stopPrank();
}
Because user2has been removed from beneficiariesarray it has been substituted with an address(0)so when the withdrawInheritedFundsfunction tries to distribute funds the transaction reverts because it is trying to send funds to the address(0).
├─ [31483] InheritanceManager::withdrawInheritedFunds(ERC20Mock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ ├─ [851] ERC20Mock::balanceOf(InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA]) [staticcall]
│ │ └─ ← [Return] 9000000000000000000 [9e18]
│ ├─ [25772] ERC20Mock::transfer(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 4500000000000000000 [4.5e18])
│ │ ├─ emit Transfer(from: InheritanceManager: [0x88F59F8826af5e695B13cA934d6c7999875A9EeA], to: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], value: 4500000000000000000 [4.5e18])
│ │ ├─ storage changes:
│ │ │ @ 0x266f7372a76c4363ea04e26dfd6a2a19d44ec1cadb0d91060c1fb98ef6adc19b: 0 → 0x0000000000000000000000000000000000000000000000003e73362871420000
│ │ │ @ 0x046b8fe1df5e368589a8e0acb67ff926456dfd1af6a8b6b77fa4a7c8dea1c3fa: 0x0000000000000000000000000000000000000000000000007ce66c50e2840000 → 0x0000000000000000000000000000000000000000000000003e73362871420000
│ │ └─ ← [Return] true
@> │ ├─ [1076] ERC20Mock::transfer(0x0000000000000000000000000000000000000000, 4500000000000000000 [4.5e18])
│ │ └─ ← [Revert] ERC20InvalidReceiver(0x0000000000000000000000000000000000000000)
Impact
Funds could be locked in contract indefinatly if owner has lost access control over contract.
Tools Used
Manual code review
Recommendations
Adjust the removeBeneficiary function:
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
+ uint256 lastIndex = beneficiaries.length - 1;
// If not the last element, swap with the last element
+ if (indexToRemove != lastIndex) {
+ beneficiaries[indexToRemove] = beneficiaries[lastIndex];
+ }
// Remove the last element
+ beneficiaries.pop();
}
Then add this if statement to withdrawInheritedFunds:
function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
// q does funds get stuck in the contract because of rounding errors?
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++) {
+ if (beneficiaries[i] != address(0)) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
+ }
}
}
}
