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

Improper Empty Check in removeBeneficiary

0x68

Summary

The removeBeneficiary function in the InheritanceManager contract improperly removes beneficiaries by simply deleting the entry at the specified index without reordering the array. This creates holes (zero addresses) in the beneficiaries array which causes multiple serious issues throughout the contract's functionality.

Vulnerability Details

The current implementation of removeBeneficiary uses the delete operator:

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

This simply sets the value at indexToRemove to address(0), leaving a gap in the array rather than properly removing the element. Additionally, there's no check that _beneficiary actually exists in the array before attempting removal.

The related _getBeneficiaryIndex function has its own issues:

function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
_index = i;
break;
}
}
}

This function will return 0 for beneficiary in the array that have been remove.

Impact

This vulnerability leads to several critical issues:

  1. Ineffective Access Control: Functions that use the onlyBeneficiaryWithIsInherited modifier will incorrectly allow access to anyone when there are gaps in the array, as the modifier will trigger an out-of-bounds error and execution will halt before the protected code can run.

  2. Fund Distribution Problems: The withdrawInheritedFunds function distributes funds equally among all elements in the beneficiaries array. If some elements are address(0) due to improper removal, funds will be sent to the zero address and permanently lost.

  3. Incorrect Accounting: Functions like buyOutEstateNFT calculate payments based on the length of the beneficiaries array, not the actual number of valid beneficiaries, leading to incorrect payment calculations.

  4. Silent Failures: Attempting to remove a non-existent beneficiary will silently set element 0 to address(0), potentially removing a legitimate beneficiary without warning.

Tools Used

Manual code review

Recommendations

  1. Implement a proper array removal algorithm that maintains array integrity:

function removeBeneficiary(address _beneficiary) external onlyOwner {
bool found = false;
uint256 indexToRemove;
// Find the beneficiary
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
indexToRemove = i;
found = true;
break;
}
}
// Revert if beneficiary not found
require(found, "Beneficiary not found");
// Shift elements to remove the gap
for (uint256 i = indexToRemove; i < beneficiaries.length - 1; i++) {
beneficiaries[i] = beneficiaries[i + 1];
}
// Remove the last element
beneficiaries.pop();
// Reset the deadline
_setDeadline();
}

  1. Fix the _getBeneficiaryIndex function to properly handle non-existent beneficiaries:

function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
return i;
}
}
revert("Beneficiary not found");
}

  1. Add checks in functions like withdrawInheritedFunds to skip zero addresses:

if (beneficiaries[i] != address(0)) {
// Process only valid beneficiaries
}

  1. Consider adding events to track beneficiary management:

event BeneficiaryAdded(address indexed beneficiary);
event BeneficiaryRemoved(address indexed beneficiary);
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.