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

Improper array element removal in removeBeneficiary creates "holes" leading to fund loss and broken inheritance logic

anchabadze

Description:

The InheritanceManager::removeBeneficiary() function removes elements from the beneficiaries array by using the delete operator, which sets the value at that index to address(0) but does not resize the array or reorganize its elements:

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

This approach creates "holes" in the array with address(0) values. The contract continues to use beneficiaries.length in calculations and iterations, treating these "holes" as valid beneficiaries throughout the codebase, affecting critical inheritance functionality

Impact:

In InheritanceManager::inherit(): A scenario with two beneficiaries where one is removed will still execute the beneficiaries.length > 1 logic path, setting isInherited = true even though there's effectively only one beneficiary. This prevents the remaining beneficiary from becoming the owner as intended.

function inherit() external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
owner = msg.sender;
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}

In InheritanceManager::withdrawInheritedFunds(): The function attempts to send assets to all addresses in the array, including address(0). This results in permanent fund loss as tokens sent to the zero address are irretrievable.

function withdrawInheritedFunds(address _asset) external {
// ...
uint256 divisor = beneficiaries.length;
// ...
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
(bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
require(success, "something went wrong");
}
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = assetAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}
}

In InheritanceManager::buyOutEstateNFT(): The buyout amount calculation includes removed beneficiaries, forcing the buyer to pay more than necessary. Tokens distributed to address(0) during the compensation phase are permanently lost.

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return;
} else {
IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
}
}
nft.burnEstate(_nftID);
}

Recommended Mitigation:

Implement a proper array element removal technique that preserves array integrity:

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

Lead Judging Commences

0xtimefliez Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect removal from beneficiary list causes funds to be send to 0 address

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