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 10 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!