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

Incorrect Fund Distribution Due to Array Length Including Deleted Beneficiaries

0xsixeyes

Summary

The withdrawInheritedFunds function uses the full array length to calculate shares, including deleted (address(0)) beneficiaries. This leads to incorrect fund distribution, violating the core invariant of equal distribution among active beneficiaries.

Vulnerability Details

The current implementation:

function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length; // Includes deleted beneficiaries!
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor; // Incorrect division
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
(bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
require(success, "something went wrong");
}
}
// ... similar issue in ERC20 logic
}

The issue:

  1. divisor = beneficiaries.length includes deleted beneficiaries (address(0))

  2. amountPerBeneficiary is calculated using this incorrect divisor

  3. Transfers to address(0) will revert

  4. The reverted amounts are never redistributed to active beneficiaries

  5. This violates the core invariant from README: "After the 90 days only the beneficiaries get access to the funds, entirely equally divided"

Impact

HIGH

  • Direct loss of funds that should go to beneficiaries

  • Violates core invariant of equal fund distribution

  • Portion of inheritance becomes permanently locked

  • Affects both ETH and token distributions

Likelihood: High

  • Occurs whenever there are deleted beneficiaries

  • Part of normal contract operation

  • No special conditions needed

  • Affects core functionality

Exploit Scenario:

  1. Contract has 100 ETH to distribute

  2. Initially 4 beneficiaries: Alice, Bob, Charlie, and Dave

  3. Owner removes Bob and Dave using removeBeneficiary

  4. Array becomes: [Alice, address(0), Charlie, address(0)]

  5. When withdrawInheritedFunds is called:

    ethAmountAvailable = 100 ETH
    divisor = 4 (incorrect length including deleted)
    amountPerBeneficiary = 100 / 4 = 25 ETH

  6. Result:

    • Alice receives 25 ETH

    • Second transfer reverts (address(0))

    • Charlie receives 25 ETH

    • Fourth transfer reverts (address(0))

    • 50 ETH remains locked in contract

  7. Correct distribution should have been:

    • Alice: 50 ETH

    • Charlie: 50 ETH

Tools Used

  • Manual review

  • Code inspection

  • Foundry tests

Recommendations

Maintain a separate counter for active beneficiaries:

uint256 public activeBeneficiaryCount;
​
constructor() {
owner = msg.sender;
nft = new NFTFactory(address(this));
activeBeneficiaryCount = 0; // Initialize counter
}
​
function addBeneficiery(address _beneficiary) external onlyOwner {
beneficiaries.push(_beneficiary);
activeBeneficiaryCount++; // Increment counter
_setDeadline();
}
​
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
require(beneficiaries[indexToRemove] != address(0), "Already removed");
delete beneficiaries[indexToRemove];
activeBeneficiaryCount--; // Decrement counter
}
​
function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
require(activeBeneficiaryCount > 0, "No active beneficiaries");
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / activeBeneficiaryCount; // Use counter instead of length
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (beneficiaries[i] != address(0)) { // Only distribute to active beneficiaries
(bool success,) = payable(beneficiaries[i]).call{value: amountPerBeneficiary}("");
require(success, "Transfer failed");
}
}
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = assetAmountAvailable / activeBeneficiaryCount; // Use counter instead of length
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (beneficiaries[i] != address(0)) { // Only distribute to active beneficiaries
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}
}
}

These changes would:

  • Ensure correct fund distribution

  • Maintain equal shares among active beneficiaries

  • Prevent funds from being locked

  • Uphold the contract's core invariants

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

Support

FAQs

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