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.
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.
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();
Sample contract without receive or fallback function:
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.
Manual Code Review
Foundry
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:
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.