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

Zero address disrupts the inherit and withdraw mechanisms

gautr

Summary

The project expects that removed beneficiaries do not impact the inheritance and withdrawal mechanisms. However, the length of the beneficiaries array does not decrease after a removal, causing an incorrect count that disrupts fund distribution. Additionally, this issue makes it impossible to revert to the personal backup scenario.

Vulnerability Details

The InheritanceManager::removeBeneficiary function replaces the beneficiary's address with a zero address but does not decrease the beneficiaries.length count.

For example, if three users are added and the first user is removed, beneficiaries.length will still be equal to 3 and beneficiaries[0] will be a zero address.

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

Since beneficiaries.length never decreases, it becomes impossible to reach the condition beneficiaries.length == 1 again. As a result, the owner cannot revert to the personal backup scenario.

function inherit() external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
// @done-audit - outsider can take ownership of the contract
if (beneficiaries.length == 1) {
owner = msg.sender;
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}

Since withdrawInheritedFunds uses beneficiaries.length as a divisor, the calculated amountPerBeneficiary will be incorrect, and funds will also be sent to the zero address, because there is no address verification inside the for loops.

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);
}
}
}

POC

Paste the following test into InheritanceManagerTest.t.sol. This test will pass, but user2 and user3 should have received 3e18.

function test_waste_money_on_zero_address() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
​
im.removeBeneficiary(user1);
vm.stopPrank();
​
vm.warp(1);
vm.deal(address(im), 6e18);
vm.warp(1 + 90 days);
​
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
​
assertEq(0, user1.balance);
// user2 and user3 should receive 3e18, but the following assertions pass
assertEq(2e18, user2.balance);
assertEq(2e18, user3.balance);
}

This test can be pasted into the same document and will demonstrate that it is impossible to revert to the personal backup scenario again.

function test_impossible_react_backup_again() public {
address user2 = makeAddr("user2");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.removeBeneficiary(user1);
vm.stopPrank();
​
vm.warp(1);
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
​
vm.startPrank(user2);
im.inherit();
vm.stopPrank();
​
// isInherited is activated even if there is one valid beneficiary
assertNotEq(user2, im.getOwner());
assertEq(owner, im.getOwner());
assertEq(im.getIsInherited(), true);
}

Impact

  • Funds can be lost due to being sent to an unintended address, with no possibility of recovery.

  • The owner can never revert to the personal backup scenario if multiple beneficiaries are added, even after removing all but one.

Tools Used

  • Foundry

Recommendations

Introduce a counter variable for valid beneficiaries. When InheritanceManager::addBeneficiery is called, the counter should increase by 1; when InheritanceManager::removeBeneficiary is called, the counter should decrease by 1.

This counter should:

  • Be used as the divisor in InheritanceManager::withdrawInheritedFunds.
    Determine conditions in InheritanceManager::inherit.

  • To prevent funds from being sent to zero addresses, implement a zero-address check inside the for loops before transferring any funds.

A better approach would be to store beneficiaries in a mapping while maintaining a counter to track the number of valid beneficiaries.

Using mappings allows:

  • Each beneficiary to withdraw their share independently.

  • Improved performance and gas efficiency.

  • Avoiding a scenario where a single beneficiary bears the high gas cost of the loops alone.

Updates

Lead Judging Commences

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

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

Support

FAQs

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

Give us feedback!