Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

In `InheritanceManager::removeBeneficiary`, beneficiaries are not properly removed, potentially causing fund loss and a DoS in withdrawal functions.

jfornells

Description: In the InheritanceManager::removeBeneficiary function, a beneficiary is intended to be removed from the list, but the removal is not performed correctly. Using delete beneficiaries[indexToRemove] only clears the value at that position but does not shift or remove the index itself. Since beneficiaries is an array of addresses, this results in an address(0) entry at that position, keeping the array size unchanged but introducing inconsistencies in the list of valid beneficiaries.

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
@> delete beneficiaries[indexToRemove];
}

Impact: In the InheritanceManager::withdrawInheritedFunds function, this issue affects the contract in two different ways, potentially causing a significant loss of funds if large amounts are managed and a denial of service (DoS) that, while not resulting in fund loss, partially blocks the function’s execution.

Since the beneficiary is not correctly removed, the total number of beneficiaries does not decrease, causing the funds to be divided into more parts than they should be. As a result, each beneficiary receives less than their actual entitlement.

In the ETH transfer section, when a beneficiary is removed, their position in the array is replaced with address(0). The call{value:} function does not revert when sending funds to 0x0, meaning the funds will be transferred and permanently lost with no possibility of recovery.

For ERC-20 tokens, most token contracts do not allow transfers to address(0), causing the transaction to revert. While this prevents fund loss, it triggers a DoS in this part of the function, blocking the asset distribution until the issue is manually resolved.

function test_removeBeneficiariesETH() public {
// Define addresses for testing
address emergencyWallet = makeAddr("emergencyWallet");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address charlie = makeAddr("charlie");
​
// Set initial ETH balance in the contract
uint256 ethAmount = 1_000_000e18;
vm.deal(address(im), ethAmount);
​
// Ensure the InheritanceManager contract has the expected ETH balance
uint256 ethBalanceImBefore = address(im).balance;
assertEq(ethBalanceImBefore, ethAmount, "Incorrect initial ETH balance");
​
// Simulate owner transactions
vm.startPrank(owner);
​
// Add beneficiaries sequentially
im.addBeneficiery(emergencyWallet);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.addBeneficiery(charlie);
​
// Attempt to remove Bob from the beneficiaries list
im.removeBeneficiary(bob); //
​
// Stop owner simulation
vm.stopPrank();
​
// Simulate 90 more days of inactivity
vm.warp(block.timestamp + 90 days);
​
// Simulate charlie transactions
vm.startPrank(charlie);
// Trigger inheritance execution
im.inherit();
​
// Withdraw ETH funds, which should be equally divided among beneficiaries
im.withdrawInheritedFunds(address(0));
​
// Retrieve updated balances after withdrawal
uint256 emergencyWalletBal = emergencyWallet.balance;
uint256 aliceBal = alice.balance;
uint256 bobBal = bob.balance;
uint256 charlieBal = charlie.balance;
​
// Display balances for debugging
console.log("EmergencyWallet ETH Bal...", emergencyWalletBal);
console.log("Alice ETH Bal.............", aliceBal);
console.log("Bob ETH Bal...............", bobBal);
console.log("Charlie ETH Bal...........", charlieBal);
​
// Expected correct amount per beneficiary (should be divided among 3, NOT 4)
uint256 amountXBeneficiary = ethAmount / 3;
console.log("---------------------------");
console.log("Correct Amount............", amountXBeneficiary);
​
vm.stopPrank();
}
​
Logs:
EmergencyWallet ETH Bal... 250000000000000000000000
Alice ETH Bal............. 250000000000000000000000
Bob ETH Bal............... 0
Charlie ETH Bal........... 250000000000000000000000
---------------------------
Correct Amount............ 333333333333333333333333

Proof of Concept 2: In this second test, we demonstrate how the ERC-20 token transfer section reverts. Since the transfers are executed within a for loop, if one of the transactions fails, none of the beneficiaries will receive their share of the funds.

im.addBeneficiery(emergencyWallet);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.addBeneficiery(charlie);
​
im.removeBeneficiary(bob);
vm.stopPrank();
​
vm.warp(block.timestamp + 90 days);
​
vm.startPrank(charlie);
im.inherit();
​
// Attempt to withdraw ERC-20 funds (USDC)
im.withdrawInheritedFunds(address(usdc));
​
vm.stopPrank();

Log:

│ ├─ [1076] ERC20Mock::transfer(0x0000000000000000000000000000000000000000, 250000000000 [2.5e11])
│ │ └─ ← [Revert] ERC20InvalidReceiver(0x0000000000000000000000000000000000000000)

Tools Used

  • Manual review

  • Foundry for Testing

Recommended Mitigation: We recommend making the following changes to the InheritanceManager::removeBeneficiary function.

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
​
+ uint256 lastIdx = beneficiaries.length - 1;
+ if (indexToRemove < lastIdx) {
+ beneficiaries[indexToRemove] = beneficiaries[lastIdx];
+ }
+ beneficiaries.pop();
​
- delete beneficiaries[indexToRemove];
}
​
Updates

Lead Judging Commences

0xtimefliez Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect removal from beneficiary list causes funds to be send to 0 address

functions do not reset the deadline

Support

FAQs

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