Beginner FriendlySolidity
100 EXP
View results
Submission Details
Severity: high
Invalid

Denial of Service in `InheritanceManager::withdrawInheritedFunds` Due to Potential External Low Level Call Failures

Summary

The withdrawInheritedFunds function in the InheritanceManager contract is susceptible to a Denial of Service (DoS) vulnerability. This occurs because the function attempts to transfer ETH to each beneficiary using a low-level call. If even one beneficiary is a smart contract without a payable receive() or fallback() function, the call will fail and the entire transaction will revert. This prevents any beneficiary from receiving their share of the funds and locks up all assets in the contract.

Vulnerability Details

https://github.com/CodeHawks-Contests/2025-03-inheritable-smart-contract-wallet/blob/main/src/InheritanceManager.sol#L236C1-L256C6

The withdrawInheritedFunds function aims to distribute the contract's ETH or ERC20 token balance among the designated beneficiaries after the inheritance has been activated. When distributing ETH, it iterates through the beneficiaries array. For each beneficiary, it utilizes beneficiary.call{value: amountPerBeneficiary}("") to send ETH. This low-level call is problematic because it requires the receiving address to be able to receive ETH, which means it must be either an EOA or a smart contract with a payable receive() or fallback() function. If a beneficiary is a contract that lacks these functions, the call will inevitably revert. The function uses the require(success, "something went wrong"); to make sure the transfer was successful. This will cause the entire withdrawInheritedFunds transaction to revert, causing a DoS and preventing any beneficiary, even those who could accept ETH, from receiving their share. This scenario also allows a malicious actor to add a contract that can not receive eth as a beneficiary, which will grief the other beneficiaries. The distribution is supposed to be an atomic operation, with all beneficiaries being paid or no one. This reverts this property and is not expected.

Proof of Concept

  • Create a contract without payable receive() or fallback() function.

  • Import it to the test suite.

  • Add this test to the test suite InheritanceManagerTest at path test/InheritanceManagerTest.t.sol and run the command forge test --mt test_DoS:

  • The test will pass due to the expected revert by using vm.expectRevert();

function test_DoS() public {
VulnerableContract vulnerableContract = new VulnerableContract();
vm.startPrank(owner);
im.addBeneficiery(address(vulnerableContract));
im.addBeneficiery(user1);
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 20e18);
vm.warp(1 + 90 days);
vm.startPrank(address(vulnerableContract));
im.inherit();
vm.expectRevert(); //Expect revert for calling `withdrawInheritedFunds`
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
}
  • Sample contract without receive or fallback function:

contract VulnerableContract {
}

Impact

  • Denial of Service: If a single beneficiary cannot receive ETH, the entire withdrawInheritedFunds function will fail, denying all beneficiaries access to their funds, rendering them inaccessible to any of the intended beneficiaries.

Tools Used

  • Manual Code Review

  • Foundry

Recommendations

  1. Allow Individual Beneficiary Withdrawals:

    • Modify the contract to remove the dependency that all beneficiaries must withdraw at the same time.

    • Instead of distributing all funds at once, create a withdrawMyInheritedFunds function that allows each beneficiary to claim their share individually.

    • The withdrawMyInheritedFunds function should be an external function and should take _asset as a parameter.

    • The user should be able to withdraw only if it's his turn.

    • The contract should have a storage variable to keep track of the total amount withdrawn for each user.

    • Remove the for loop and beneficiary.call{value: amountPerBeneficiary}("") logic.

    • Make sure to set isWithdrawnToken[msg.sender][_asset] and isWithdrawnETH[msg.sender] flag to true for this beneficiary to not be able to withdraw again.

    • This removes the DoS as each beneficiary can withdraw whenever they want.

    • Example:

mapping(address => bool) public isWithdrawnETH;
mapping(address user => mapping(address token => bool)) public isWithdrawnToken;
bool hasWithdrawalStarted;
uint256 amountETHPerBeneficiary;
mapping(address => uint256) public amountTokenPerBeneficiary;
function withdrawMyInheritedFunds(address _asset) external onlyBeneficiaryWithIsInherited {
uint256 divisor = beneficiaries.length;
if(!hasWithdrawalStarted) {
hasWithdrawalStarted = true;
amountETHPerBeneficiary = address(this).balance/divisor;
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
amountTokenPerBeneficiary[_asset] = assetAmountAvailable/divisor;
}
if (_asset == address(0)) {
if (isWithdrawnETH[msg.sender]) {
revert AlreadyWithdrawn();
}
isWithdrawnETH[msg.sender] = true;
(bool success,) = msg.sender.call{value: amountETHPerBeneficiary}("");
require(success, "Transfer Failed");
} else {
if (isWithdrawnToken[msg.sender][_asset]) {
revert AlreadyWithdrawn();
}
isWithdrawnToken[msg.sender][_asset] = true;
uint256 amountPerBeneficiary = amountTokenPerBeneficiary[_asset];
IERC20(_asset).safeTransfer(msg.sender, amountPerBeneficiary);
}
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.