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

The `_getBeneficiaryIndex` function returns incorrect indices for non-existent beneficiaries, which can lead to removal of wrong beneficiaries and incorrect fund distribution.

alekseyts

Summary

A critical vulnerability has been identified in the InheritanceManager contract's beneficiary management system. The _getBeneficiaryIndex function returns incorrect indices for non-existent beneficiaries, which can lead to removal of wrong beneficiaries and incorrect fund distribution.

Vulnerability Details

The vulnerability exists in the beneficiary management system:

  1. Index Return Issue:

  • When a beneficiary is not found, the function returns index 0

  • This causes confusion with the actual beneficiary at index 0

  • Allows removal of wrong beneficiaries through removeBeneficiary

  1. Test Demonstration Shows:

  • Three beneficiaries are added (user1, user2, user3)

  • Querying index for non-existent beneficiary returns same index as an existing beneficiary

  • This allows removing wrong beneficiary

  • Results in incorrect fund distribution

From the test:

//SPDX-License-Identifier: MIT
​
pragma solidity 0.8.26;
​
import {Test, console} from "forge-std/Test.sol";
import {InheritanceManager} from "../src/InheritanceManager.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
​
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
contract InheritanceManagerAuditTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
​
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
​
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
weth = new ERC20Mock();
}
​
// test _getBeneficiaryIndex: if beneciciary not found and there is only one added it will return index 0 and return wrong index and leave owner without backup wallet
// removeBeneficiary can remove incorrect beneficiary
// fix: start adding beneficiary from index 1, if not found return 0 or revert.
function test_wrongIndex_getBeneficiaryIndex() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address notBeneficiary = makeAddr("notBeneficiary");
​
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
​
uint256 indexOfCorrectBeneficiary = im._getBeneficiaryIndex(user1);
uint256 indexOfIncorrectBeneficiary = im._getBeneficiaryIndex(
notBeneficiary
);
uint256 indexOfuser2Beneficiary = im._getBeneficiaryIndex(user2);
vm.stopPrank();
​
// user2 is beneficiary too...at same index as correct beneficiary user1
assertEq(indexOfCorrectBeneficiary, indexOfIncorrectBeneficiary);
assertEq(1, indexOfuser2Beneficiary);
​
vm.startPrank(owner);
im.removeBeneficiary(notBeneficiary);
vm.stopPrank();
​
vm.warp(1 + 90 days);
​
uint256 amount = 10e10;
vm.deal(address(im), amount);
​
vm.startPrank(owner);
im.inherit();
​
vm.warp(1 + 90 days);
​
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
​
assertEq(0, user1.balance);
assertEq(amount / 3, user2.balance);
assertEq(amount / 3, user3.balance);
}
}
​

The test proves this by:

  1. Adding three beneficiaries

  2. Showing that _getBeneficiaryIndex(notBeneficiary) returns same index as valid beneficiary

  3. Demonstrating fund distribution is affected:

    • user1 receives 0 funds

    • user2 and user3 receive equal shares

    • Funds are incorrectly distributed due to wrong beneficiary management

Impact

Critical severity. The vulnerability allows:

  • Removal of wrong beneficiaries

  • Loss of backup wallet functionality

  • Incorrect inheritance fund distribution

  • Potential complete loss of access to funds if wrong beneficiary is removed

Tools Used

  • Manual code review

  • Foundry test framework

  • Custom test cases demonstrating the index collision

  • Function return value analysis

Recommendations

  1. Modify beneficiary index management:

contract InheritanceManager {
// Start beneficiary indices from 1
uint256 private constant NOT_FOUND = 0;
function _getBeneficiaryIndex(address beneficiary) internal view returns (uint256) {
// Return 0 (NOT_FOUND) if beneficiary doesn't exist
// Start actual indices from 1
for (uint256 i = 1; i <= beneficiaryCount; i++) {
if (beneficiaries[i] == beneficiary) {
return i;
}
}
return NOT_FOUND;
}
function removeBeneficiary(address beneficiary) external {
uint256 index = _getBeneficiaryIndex(beneficiary);
require(index != NOT_FOUND, "Beneficiary not found");
// Continue with removal...
}
}

  1. Add additional safety checks:

  • Validate beneficiary existence before operations

  • Add explicit beneficiary status tracking

  • Implement beneficiary operation events for tracking

  1. Enhance error handling:

  • Add specific error messages for beneficiary operations

  • Implement proper revert messages for invalid operations

  • Add events for critical beneficiary changes

Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

alekseyts Submitter
9 months ago
0xtimefliez Lead Judge
9 months ago
0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!