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

Beneficiaries array management issues lead to incorrect fund distribution

0xWithdrawers

Description

There are two issues with the management of beneficiaries in the InheritanceManager contract:

  1. Duplicate Beneficiaries: The addBeneficiery() function does not check if a beneficiary has already been added, allowing the same address to be added multiple times:

function addBeneficiery(address _beneficiary) external onlyOwner {
beneficiaries.push(_beneficiary);
_setDeadline();
}

  1. Incomplete Removal: The removeBeneficiary() function replaces an entry with address(0) but does not reduce the array length:

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

These issues directly impact functions that use beneficiaries.length for calculations, particularly:

  • withdrawInheritedFunds(): Divides funds by beneficiaries.length for distribution

  • buyOutEstateNFT(): Uses beneficiaries.length to calculate payment amounts

Impact

1. Unequal Fund Distribution

Duplicate entries cause a beneficiary to receive multiple shares. For example, if an address appears twice in an array of three beneficiaries, they would receive 2/3 of the funds instead of the intended 1/3.

function withdrawInheritedFunds(address _asset) external {
// ...
uint256 divisor = beneficiaries.length; // Incorrect divisor if duplicates exist
// ...
for (uint256 i = 0; i < divisor; i++) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}

2. Funds Sent to address(0)

When a beneficiary is removed, address(0) remains in the array. When distributing funds:

  • For ETH: Transfers to address(0) may succeed but the funds are irretrievable

  • For ERC20 tokens: Transfers to address(0) will either revert or burn the tokens, depending on the token implementation

3. Contract Dysfunction

For buyOutEstateNFT(), if a beneficiary has been removed, the function may revert when trying to transfer tokens to address(0), making it impossible to buy out NFTs:

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
// ...
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return;
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
// ...
}

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
import "forge-std/Test.sol";
import "../src/InheritanceManager.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
// @audit-note - [VULN 07]
// This PoC demonstrates inconsistencies with beneficiaries.length:
// 1. Duplicate beneficiaries can be added, creating imbalanced distributions
// 2. Removed beneficiaries leave address(0) entries, causing miscalculations
// 3. Duplicate beneficiaries receive multiple shares during fund distribution
​
// Mock ERC20 token for testing
contract MockToken is ERC20 {
constructor() ERC20("Mock Token", "MOCK") {
_mint(msg.sender, 1000000 * 10**18);
}
}
​
contract PoCTest is Test {
InheritanceManager public inheritanceManager;
address public owner;
address public beneficiary1;
address public beneficiary2;
address public beneficiary3;
MockToken public mockToken;
function setUp() public {
// Deploy the InheritanceManager contract and token
inheritanceManager = new InheritanceManager();
mockToken = new MockToken();
owner = inheritanceManager.getOwner();
// Create beneficiary addresses
beneficiary1 = address(0x1);
beneficiary2 = address(0x2);
beneficiary3 = address(0x3);
// Give each beneficiary an initial balance for testing
vm.deal(beneficiary1, 10 ether);
vm.deal(beneficiary2, 10 ether);
vm.deal(beneficiary3, 10 ether);
// Transfer tokens to the InheritanceManager for distribution tests
mockToken.transfer(address(inheritanceManager), 1000 * 10**18);
}
​
// Helper function to get beneficiaries.length
function getBeneficiariesLength() public view returns (uint256) {
// In InheritanceManager contract, beneficiaries array is at storage slot 5
bytes32 data = vm.load(address(inheritanceManager), bytes32(uint256(5)));
return uint256(data);
}
function test_BeneficiariesLengthInconsistencies() public {
// Step 1: Add beneficiaries - including duplicates
console.log("----- INITIAL SETUP -----");
vm.startPrank(owner);
inheritanceManager.addBeneficiery(beneficiary1);
console.log("Added beneficiary1 (0x1)");
inheritanceManager.addBeneficiery(beneficiary1); // DUPLICATE
console.log("Added beneficiary1 (0x1) again - DUPLICATE");
inheritanceManager.addBeneficiery(beneficiary2);
console.log("Added beneficiary2 (0x2)");
inheritanceManager.addBeneficiery(beneficiary3);
console.log("Added beneficiary3 (0x3)");
console.log("Size of beneficiaries array :", getBeneficiariesLength());
vm.stopPrank();
// Step 2: Verify duplicate entries using _getBeneficiaryIndex
console.log("\n----- VERIFYING DUPLICATE ENTRIES -----");
uint256 firstIndexOfB1 = inheritanceManager._getBeneficiaryIndex(beneficiary1);
console.log("First occurrence of beneficiary1 at index:", firstIndexOfB1);
​
// Add duplicate check by finding the second occurrence manually
bool foundDuplicate = false;
uint256 secondIndexOfB1 = 0;
// We know there's a duplicate, so we'll check each index after the first occurrence
for (uint256 i = firstIndexOfB1 + 1; i < 4; i++) {
inheritanceManager.removeBeneficiary(address(uint160(i))); // Remove temporarily to check
// If this removes beneficiary1, we found the duplicate
uint256 checkIndex = inheritanceManager._getBeneficiaryIndex(beneficiary1);
if (checkIndex != firstIndexOfB1) {
foundDuplicate = true;
secondIndexOfB1 = i;
break;
}
}
assertTrue(foundDuplicate, "...");
console.log("Found duplicate of beneficiary1 at index:", secondIndexOfB1);
console.log("Size of beneficiaries array :", getBeneficiariesLength());
// Step 3: Remove beneficiary and check remaining array
console.log("\n----- DELETING AN ENTRY -----");
console.log("Removing beneficiary3 (0x3)");
inheritanceManager.removeBeneficiary(beneficiary3);
// Verify if beneficiary3 is actually gone by checking its index
uint256 indexAfterRemoval = inheritanceManager._getBeneficiaryIndex(beneficiary3);
console.log("After removal, _getBeneficiaryIndex for beneficiary3 returns:", indexAfterRemoval);
console.log("Size of beneficiaries array :", getBeneficiariesLength());
}
​
function test_DuplicateBeneficiaryReceivesMultipleShares() public {
console.log("\n----- FUND DISTRIBUTION TO DUPLICATE BENEFICIARIES -----");
// Step 1: Setup with duplicate beneficiary
vm.startPrank(owner);
inheritanceManager.addBeneficiery(beneficiary1); // First occurrence
inheritanceManager.addBeneficiery(beneficiary1); // DUPLICATE - will receive funds twice
inheritanceManager.addBeneficiery(beneficiary2);
vm.stopPrank();
console.log("Added beneficiary1 twice and beneficiary2 once");
console.log("Size of beneficiaries array:", getBeneficiariesLength());
// Record initial token balances
uint256 initialContractBalance = mockToken.balanceOf(address(inheritanceManager));
uint256 initialBeneficiary1Balance = mockToken.balanceOf(beneficiary1);
uint256 initialBeneficiary2Balance = mockToken.balanceOf(beneficiary2);
console.log("Initial contract token balance:", initialContractBalance);
console.log("Initial beneficiary1 balance:", initialBeneficiary1Balance);
console.log("Initial beneficiary2 balance:", initialBeneficiary2Balance);
// Step 2: Trigger inheritance and fund distribution
console.log("\n----- TRIGGERING INHERITANCE -----");
// Jump ahead in time to pass the deadline
vm.warp(block.timestamp + 100 days);
// Inherit and distribute funds
vm.prank(beneficiary1);
inheritanceManager.inherit(); // Set isInherited to true
console.log("After inherit(), isInherited =", inheritanceManager.getIsInherited());
vm.prank(beneficiary1);
inheritanceManager.withdrawInheritedFunds(address(mockToken));
// Step 3: Check final balances
uint256 finalContractBalance = mockToken.balanceOf(address(inheritanceManager));
uint256 finalBeneficiary1Balance = mockToken.balanceOf(beneficiary1);
uint256 finalBeneficiary2Balance = mockToken.balanceOf(beneficiary2);
console.log("\n----- FINAL BALANCES AFTER DISTRIBUTION -----");
console.log("Final contract token balance:", finalContractBalance);
console.log("Final beneficiary1 balance:", finalBeneficiary1Balance);
console.log("Final beneficiary2 balance:", finalBeneficiary2Balance);
// Calculate and display what each beneficiary received
uint256 beneficiary1Received = finalBeneficiary1Balance - initialBeneficiary1Balance;
uint256 beneficiary2Received = finalBeneficiary2Balance - initialBeneficiary2Balance;
console.log("\n----- DISTRIBUTION ANALYSIS -----");
console.log("Beneficiary1 received:", beneficiary1Received);
console.log("Beneficiary2 received:", beneficiary2Received);
// We expect beneficiary1 to receive 2 shares (since they appear twice in the array)
// beneficiary2 receives only 1 share
assertTrue(beneficiary1Received > beneficiary2Received, "Beneficiary1 should receive more funds due to duplicate entries");
// The ratio should be approximately 2:1
uint256 ratio = beneficiary1Received / beneficiary2Received;
assertEq(ratio, 2, "Distribution ratio should be 2:1 for duplicate vs single beneficiary");
console.log("Distribution ratio (beneficiary1:beneficiary2):", ratio, ":1");
console.log("VULNERABILITY CONFIRMED: Duplicate beneficiaries receive multiple shares!");
}
}

Place the test in the test folder and run it with the following command:

forge test --match-test test_BeneficiariesLengthInconsistencies -vv
forge test --match-test test_DuplicateBeneficiaryReceivesMultipleShares -vv

This test demonstrates:

  1. Duplicate beneficiaries can be added (beneficiary1 appears twice)

  2. Removing a beneficiary doesn't change the array length

  3. Duplicate beneficiaries receive multiple shares during fund distribution

Recommendation

To fix these issues:

1. Prevent duplicate beneficiaries:

function addBeneficiery(address _beneficiary) external onlyOwner {
// Check if beneficiary already exists
for (uint256 i = 0; i < beneficiaries.length; i++) {
require(beneficiaries[i] != _beneficiary, "Beneficiary already exists");
}
beneficiaries.push(_beneficiary);
_setDeadline();
}

2. Properly remove beneficiaries by rearranging the array:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
// Verify the beneficiary exists
require(beneficiaries[indexToRemove] == _beneficiary, "Beneficiary not found");
// Move the last element to the position of the removed element
beneficiaries[indexToRemove] = beneficiaries[beneficiaries.length - 1];
// Remove the last element
beneficiaries.pop();
_setDeadline();
}

These changes will ensure fair and accurate fund distribution and prevent both duplicate beneficiaries and issues with removed entries.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 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.